netdev - Re: [PATCH bpf-next 07/11] libbpf: fix BTF data layout checks and allow empty BTF

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]

Date:   Mon, 2 Nov 2020 21:18:48 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Song Liu <songliubraving@...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 bpf-next 07/11] libbpf: fix BTF data layout checks and
 allow empty BTF

On Mon, Nov 2, 2020 at 4:51 PM Song Liu <songliubraving@...com> wrote:
>
>
>
> > On Oct 28, 2020, at 5:58 PM, Andrii Nakryiko <andrii@...nel.org> wrote:
> >
> > Make data section layout checks stricter, disallowing overlap of types and
> > strings data.
> >
> > Additionally, allow BTFs with no type data. There is nothing inherently wrong
> > with having BTF with no types (put potentially with some strings). This could
> > be a situation with kernel module BTFs, if module doesn't introduce any new
> > type information.
> >
> > Also fix invalid offset alignment check for btf->hdr->type_off.
> >
> > Fixes: 8a138aed4a80 ("bpf: btf: Add BTF support to libbpf")
> > Signed-off-by: Andrii Nakryiko <andrii@...nel.org>
> > ---
> > tools/lib/bpf/btf.c | 16 ++++++----------
> > 1 file changed, 6 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > index 20c64a8441a8..9b0ef71a03d0 100644
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -245,22 +245,18 @@ static int btf_parse_hdr(struct btf *btf)
> >               return -EINVAL;
> >       }
> >
> > -     if (meta_left < hdr->type_off) {
> > -             pr_debug("Invalid BTF type section offset:%u\n", hdr->type_off);
> > +     if (meta_left < hdr->str_off + hdr->str_len) {
> > +             pr_debug("Invalid BTF total size:%u\n", btf->raw_size);
> >               return -EINVAL;
> >       }
>
> Can we make this one as
>         if (meta_left != hdr->str_off + hdr->str_len) {

That would be not forward-compatible. I.e., old libbpf loading new BTF
with extra stuff after the string section. Kernel is necessarily more
strict, but I'd like to keep libbpf more permissive with this.

>
> >
> > -     if (meta_left < hdr->str_off) {
> > -             pr_debug("Invalid BTF string section offset:%u\n", hdr->str_off);
> > +     if (hdr->type_off + hdr->type_len > hdr->str_off) {
> > +             pr_debug("Invalid BTF data sections layout: type data at %u + %u, strings data at %u + %u\n",
> > +                      hdr->type_off, hdr->type_len, hdr->str_off, hdr->str_len);
> >               return -EINVAL;
> >       }
>
> And this one
>         if (hdr->type_off + hdr->type_len != hdr->str_off) {
>
> ?

Similarly, libbpf could be a bit more permissive here without
sacrificing correctness (at least for read-only BTF, when rewriting
BTF extra data will be discarded, of course).

>
> [...]