Re: [PATCH v2 bpf-next 09/17] libbpf: extend sanity checking ELF symbols with externs validation

[Yonghong Song <yhs@fb.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;
>>> +             }
[...]