All of lore.kernel.org
 help / color / mirror / Atom feed
From: Yonghong Song <yhs@fb.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Andrii Nakryiko <andrii@kernel.org>, bpf <bpf@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v2 bpf-next 09/17] libbpf: extend sanity checking ELF symbols with externs validation
Date: Thu, 22 Apr 2021 16:13:25 -0700	[thread overview]
Message-ID: <f2e46f09-775f-5ad0-f62b-c12e67902670@fb.com> (raw)
In-Reply-To: <CAEf4BzZjry6=1tu=1Msx9JNGSwS8LOyCbZFioO-1CxWc-AzW6g@mail.gmail.com>



On 4/22/21 11:20 AM, Andrii Nakryiko wrote:
> On Thu, Apr 22, 2021 at 9:35 AM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 4/16/21 1:23 PM, Andrii Nakryiko wrote:
>>> Add logic to validate extern symbols, plus some other minor extra checks, like
>>> ELF symbol #0 validation, general symbol visibility and binding validations.
>>>
>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>>> ---
>>>    tools/lib/bpf/linker.c | 43 +++++++++++++++++++++++++++++++++---------
>>>    1 file changed, 34 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
>>> index 1263641e8b97..283249df9831 100644
>>> --- a/tools/lib/bpf/linker.c
>>> +++ b/tools/lib/bpf/linker.c
>>> @@ -750,14 +750,39 @@ static int linker_sanity_check_elf_symtab(struct src_obj *obj, struct src_sec *s
>>>        n = sec->shdr->sh_size / sec->shdr->sh_entsize;
>>>        sym = sec->data->d_buf;
>>>        for (i = 0; i < n; i++, sym++) {
>>> -             if (sym->st_shndx
>>> -                 && sym->st_shndx < SHN_LORESERVE
>>> -                 && sym->st_shndx >= obj->sec_cnt) {
>>> +             int sym_type = ELF64_ST_TYPE(sym->st_info);
>>> +             int sym_bind = ELF64_ST_BIND(sym->st_info);
>>> +
>>> +             if (i == 0) {
>>> +                     if (sym->st_name != 0 || sym->st_info != 0
>>> +                         || sym->st_other != 0 || sym->st_shndx != 0
>>> +                         || sym->st_value != 0 || sym->st_size != 0) {
>>> +                             pr_warn("ELF sym #0 is invalid in %s\n", obj->filename);
>>> +                             return -EINVAL;
>>> +                     }
>>> +                     continue;
>>> +             }
>>
>> In ELF file, the first entry of symbol table and section table (index 0)
>> is invalid/undefined.
>>
>> Symbol table '.symtab' contains 9 entries:
>>      Num:    Value          Size Type    Bind   Vis       Ndx Name
>>        0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT   UND
>>
>> Section Headers:
>>
>>     [Nr] Name              Type            Address          Off    Size
>>    ES Flg Lk Inf Al
>>     [ 0]                   NULL            0000000000000000 000000 000000
>> 00      0   0  0
>>
>> Instead of validating them, I think we can skip traversal of the index =
>> 0 entry for symbol table and section header table. What do you think?
> 
> In valid ELF yes. But then entire sanity check logic is not needed if
> we just assume correct ELF. But I don't want to make that potentially
> dangerous assumption :) Here I'm validating that ELF is sane with
> minimal efforts. I do skip symbol #0 later because I validated that
> it's all-zero one, as expected by ELF standard.

I just feel we can ignore entry 0 regardless of its contents. But I 
guess this is still useful since libbpf linker generates its own
ELF file and it is worthwhile to do an in-depth check for that.
So I am okay with this.

> 
>>
>>> +             if (sym_bind != STB_LOCAL && sym_bind != STB_GLOBAL && sym_bind != STB_WEAK) {
>>> +                     pr_warn("ELF sym #%d is section #%zu has unsupported symbol binding %d\n",
>>> +                             i, sec->sec_idx, sym_bind);
>>> +                     return -EINVAL;
>>> +             }
>>> +             if (sym->st_shndx == 0) {
>>> +                     if (sym_type != STT_NOTYPE || sym_bind == STB_LOCAL
>>> +                         || sym->st_value != 0 || sym->st_size != 0) {
>>> +                             pr_warn("ELF sym #%d is invalid extern symbol in %s\n",
>>> +                                     i, obj->filename);
>>> +
>>> +                             return -EINVAL;
>>> +                     }
>>> +                     continue;
>>> +             }
[...]

  reply	other threads:[~2021-04-22 23:13 UTC|newest]

Thread overview: 67+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-16 20:23 [PATCH v2 bpf-next 00/17] BPF static linker: support externs Andrii Nakryiko
2021-04-16 20:23 ` [PATCH v2 bpf-next 01/17] bpftool: support dumping BTF VAR's "extern" linkage Andrii Nakryiko
2021-04-22  3:02   ` Yonghong Song
2021-04-16 20:23 ` [PATCH v2 bpf-next 02/17] bpftool: dump more info about DATASEC members Andrii Nakryiko
2021-04-22  3:09   ` Yonghong Song
2021-04-16 20:23 ` [PATCH v2 bpf-next 03/17] libbpf: suppress compiler warning when using SEC() macro with externs Andrii Nakryiko
2021-04-22  3:47   ` Yonghong Song
2021-04-22  3:59     ` Andrii Nakryiko
2021-04-16 20:23 ` [PATCH v2 bpf-next 04/17] libbpf: mark BPF subprogs with hidden visibility as static for BPF verifier Andrii Nakryiko
2021-04-22  5:43   ` Yonghong Song
2021-04-22 18:09     ` Andrii Nakryiko
2021-04-22 23:00       ` Yonghong Song
2021-04-22 23:28         ` Andrii Nakryiko
2021-04-16 20:23 ` [PATCH v2 bpf-next 05/17] libbpf: allow gaps in BPF program sections to support overriden weak functions Andrii Nakryiko
2021-04-22  6:25   ` Yonghong Song
2021-04-22 18:10     ` Andrii Nakryiko
2021-04-16 20:23 ` [PATCH v2 bpf-next 06/17] libbpf: refactor BTF map definition parsing Andrii Nakryiko
2021-04-22 15:33   ` Yonghong Song
2021-04-22 18:40     ` Andrii Nakryiko
2021-04-16 20:23 ` [PATCH v2 bpf-next 07/17] libbpf: factor out symtab and relos sanity checks Andrii Nakryiko
2021-04-22 16:06   ` Yonghong Song
2021-04-22 18:16     ` Andrii Nakryiko
2021-04-16 20:23 ` [PATCH v2 bpf-next 08/17] libbpf: make few internal helpers available outside of libbpf.c Andrii Nakryiko
2021-04-22 16:19   ` Yonghong Song
2021-04-22 18:18     ` Andrii Nakryiko
2021-04-16 20:23 ` [PATCH v2 bpf-next 09/17] libbpf: extend sanity checking ELF symbols with externs validation Andrii Nakryiko
2021-04-22 16:35   ` Yonghong Song
2021-04-22 18:20     ` Andrii Nakryiko
2021-04-22 23:13       ` Yonghong Song [this message]
2021-04-16 20:23 ` [PATCH v2 bpf-next 10/17] libbpf: tighten BTF type ID rewriting with error checking Andrii Nakryiko
2021-04-22 16:50   ` Yonghong Song
2021-04-22 18:24     ` Andrii Nakryiko
2021-04-23  2:54       ` Alexei Starovoitov
2021-04-23  4:25         ` Andrii Nakryiko
2021-04-23  5:11           ` Alexei Starovoitov
2021-04-23 16:09             ` Andrii Nakryiko
2021-04-23 16:18               ` Alexei Starovoitov
2021-04-23 16:30                 ` Andrii Nakryiko
2021-04-23 16:34                   ` Alexei Starovoitov
2021-04-23 17:02                     ` Andrii Nakryiko
2021-04-16 20:23 ` [PATCH v2 bpf-next 11/17] libbpf: add linker extern resolution support for functions and global variables Andrii Nakryiko
2021-04-22 21:27   ` Yonghong Song
2021-04-22 22:12     ` Andrii Nakryiko
2021-04-22 23:57       ` Yonghong Song
2021-04-23  2:36         ` Yonghong Song
2021-04-23  4:28           ` Andrii Nakryiko
2021-04-23  4:27         ` Andrii Nakryiko
2021-04-16 20:23 ` [PATCH v2 bpf-next 12/17] libbpf: support extern resolution for BTF-defined maps in .maps section Andrii Nakryiko
2021-04-22 22:56   ` Yonghong Song
2021-04-22 23:32     ` Andrii Nakryiko
2021-04-16 20:24 ` [PATCH v2 bpf-next 13/17] selftests/bpf: use -O0 instead of -Og in selftests builds Andrii Nakryiko
2021-04-23  0:05   ` Yonghong Song
2021-04-16 20:24 ` [PATCH v2 bpf-next 14/17] selftests/bpf: omit skeleton generation for multi-linked BPF object files Andrii Nakryiko
2021-04-23  0:13   ` Yonghong Song
2021-04-16 20:24 ` [PATCH v2 bpf-next 15/17] selftests/bpf: add function linking selftest Andrii Nakryiko
2021-04-23  0:50   ` Yonghong Song
2021-04-23  2:34     ` Alexei Starovoitov
2021-04-23  4:29       ` Andrii Nakryiko
2021-04-23 17:18     ` Andrii Nakryiko
2021-04-23 17:35       ` Yonghong Song
2021-04-23 17:55         ` Andrii Nakryiko
2021-04-23 17:58           ` Yonghong Song
2021-04-23 17:59             ` Andrii Nakryiko
2021-04-16 20:24 ` [PATCH v2 bpf-next 16/17] selftests/bpf: add global variables " Andrii Nakryiko
2021-04-23  1:01   ` Yonghong Song
2021-04-16 20:24 ` [PATCH v2 bpf-next 17/17] sleftests/bpf: add map " Andrii Nakryiko
2021-04-23  1:20   ` Yonghong Song

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f2e46f09-775f-5ad0-f62b-c12e67902670@fb.com \
    --to=yhs@fb.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@fb.com \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=kernel-team@fb.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.