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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

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.

>
> > +             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;
> > +             }
> > +             if (sym->st_shndx < SHN_LORESERVE && sym->st_shndx >= obj->sec_cnt) {
> >                       pr_warn("ELF sym #%d in section #%zu points to missing section #%zu in %s\n",
> >                               i, sec->sec_idx, (size_t)sym->st_shndx, obj->filename);
> >                       return -EINVAL;
> >               }
> > -             if (ELF64_ST_TYPE(sym->st_info) == STT_SECTION) {
> > +             if (sym_type == STT_SECTION) {
> >                       if (sym->st_value != 0)
> >                               return -EINVAL;
> >                       continue;
> > @@ -1135,16 +1160,16 @@ static int linker_append_elf_syms(struct bpf_linker *linker, struct src_obj *obj
> >               size_t dst_sym_idx;
> >               int name_off;
> >
> > -             /* we already have all-zero initial symbol */
> > -             if (sym->st_name == 0 && sym->st_info == 0 &&
> > -                 sym->st_other == 0 && sym->st_shndx == SHN_UNDEF &&
> > -                 sym->st_value == 0 && sym->st_size ==0)
> > +             /* We already validated all-zero symbol #0 and we already
> > +              * appended it preventively to the final SYMTAB, so skip it.
> > +              */
> > +             if (i == 0)
> >                       continue;
> >
> >               sym_name = elf_strptr(obj->elf, str_sec_idx, sym->st_name);
> >               if (!sym_name) {
> >                       pr_warn("can't fetch symbol name for symbol #%d in '%s'\n", i, obj->filename);
> > -                     return -1;
> > +                     return -EINVAL;
> >               }
> >
> >               if (sym->st_shndx && sym->st_shndx < SHN_LORESERVE) {
> >