netdev - Re: [PATCH v2 bpf-next 07/17] libbpf: factor out symtab and relos sanity checks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzZWA+sPoHYBNK6+3yWcAUBd4KJuiUGOsMbbs0wSBvjcKw@mail.gmail.com>
Date:   Thu, 22 Apr 2021 11:16:48 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     Andrii Nakryiko <andrii@...nel.org>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 07/17] libbpf: factor out symtab and relos
 sanity checks

On Thu, Apr 22, 2021 at 9:06 AM Yonghong Song <yhs@...com> wrote:
>
>
>
> On 4/16/21 1:23 PM, Andrii Nakryiko wrote:
> > Factor out logic for sanity checking SHT_SYMTAB and SHT_REL sections into
> > separate sections. They are already quite extensive and are suffering from too
> > deep indentation. Subsequent changes will extend SYMTAB sanity checking
> > further, so it's better to factor each into a separate function.
> >
> > No functional changes are intended.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@...nel.org>
>
> Ack with a minor suggestion below.
> Acked-by: Yonghong Song <yhs@...com>
>
> > ---
> >   tools/lib/bpf/linker.c | 233 ++++++++++++++++++++++-------------------
> >   1 file changed, 127 insertions(+), 106 deletions(-)
> >

[...]

> >
> > -                     /* .rel<secname> -> <secname> pattern is followed */
> > -                     if (strncmp(sec->sec_name, ".rel", sizeof(".rel") - 1) != 0
> > -                         || strcmp(sec->sec_name + sizeof(".rel") - 1, link_sec->sec_name) != 0) {
> > -                             pr_warn("ELF relo section #%zu name has invalid name in %s\n",
> > -                                     sec->sec_idx, obj->filename);
> > +     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) {
> > +                     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->st_value != 0)
> >                               return -EINVAL;
> > -                     }
> > +                     continue;
>
> I think this "continue" is not necessary.
>

yes, but I wanted to make it clear that there should be no more logic
handling STT_SECTION afterwards, if/when we extend the logic of this
loop. We have similar patterns with goto to keep logic more modular
and easily extensible:


    if (something) {
        err = -EINVAL;
        goto err_out;
    }


err_out:
    return err;


So let's keep it as is.

> > +             }
> > +     }
> >
> > -                     /* don't further validate relocations for ignored sections */
> > -                     if (link_sec->skipped)
> > -                             break;
> > +     return 0;
> > +}
> >
> > -                     /* relocatable section is data or instructions */
> > -                     if (link_sec->shdr->sh_type != SHT_PROGBITS
> > -                         && link_sec->shdr->sh_type != SHT_NOBITS) {
> > -                             pr_warn("ELF relo section #%zu points to invalid section #%zu in %s\n",
> > -                                     sec->sec_idx, (size_t)sec->shdr->sh_info, obj->filename);
> > -                             return -EINVAL;
> > -                     }
> [...]