netdev - Re: [PATCH bpf 2/2] libbpf: allow loading empty BTFs

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: <CAEf4BzZMg9Ur=412xi4mQ7xkojLrUYszPF6xLmyJMRz4X+rbtg@mail.gmail.com>
Date:   Tue, 12 Jan 2021 12:26:56 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Yonghong Song <yhs@...com>, Andrii Nakryiko <andrii@...nel.org>,
        bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Kernel Team <kernel-team@...com>,
        Christopher William Snowhill <chris@...e54.net>
Subject: Re: [PATCH bpf 2/2] libbpf: allow loading empty BTFs

On Tue, Jan 12, 2021 at 12:17 PM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 1/12/21 7:41 AM, Andrii Nakryiko wrote:
> > On Mon, Jan 11, 2021 at 5:16 PM Yonghong Song <yhs@...com> wrote:
> >> On 1/11/21 12:51 PM, Andrii Nakryiko wrote:
> >>> On Mon, Jan 11, 2021 at 10:13 AM Yonghong Song <yhs@...com> wrote:
> >>>> On 1/9/21 11:03 PM, Andrii Nakryiko wrote:
> >>>>> Empty BTFs do come up (e.g., simple kernel modules with no new types and
> >>>>> strings, compared to the vmlinux BTF) and there is nothing technically wrong
> >>>>> with them. So remove unnecessary check preventing loading empty BTFs.
> >>>>>
> >>>>> Reported-by: Christopher William Snowhill <chris@...e54.net>
> >>>>> Fixes: ("d8123624506c libbpf: Fix BTF data layout checks and allow empty BTF")
>
> Fixed up Fixes tag ^^^^^ while applying. ;-)

Oh the irony, eh? :) Thanks, Daniel!

>
> >>>>> Signed-off-by: Andrii Nakryiko <andrii@...nel.org>
> >>>>> ---
> >>>>>     tools/lib/bpf/btf.c | 5 -----
> >>>>>     1 file changed, 5 deletions(-)
> >>>>>
> >>>>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> >>>>> index 3c3f2bc6c652..9970a288dda5 100644
> >>>>> --- a/tools/lib/bpf/btf.c
> >>>>> +++ b/tools/lib/bpf/btf.c
> >>>>> @@ -240,11 +240,6 @@ static int btf_parse_hdr(struct btf *btf)
> >>>>>         }
> >>>>>
> >>>>>         meta_left = btf->raw_size - sizeof(*hdr);
> >>>>> -     if (!meta_left) {
> >>>>> -             pr_debug("BTF has no data\n");
> >>>>> -             return -EINVAL;
> >>>>> -     }
> >>>>
> >>>> Previous kernel patch allows empty btf only if that btf is module (not
> >>>> base/vmlinux) btf. Here it seems we allow any empty non-module btf to be
> >>>> loaded into the kernel. In such cases, loading may fail? Maybe we should
> >>>> detect such cases in libbpf and error out instead of going to kernel and
> >>>> get error back?
> >>>
> >>> I did this consciously. Kernel is more strict, because there is no
> >>> reasonable case when vmlinux BTF or BPF program's BTF can be empty (at
> >>> least not that now we have FUNCs in BTF). But allowing libbpf to load
> >>> empty BTF generically is helpful for bpftool, as one example, for
> >>> inspection. If you do `bpftool btf dump` on empty BTF, it will just
> >>> print nothing and you'll know that it's a valid (from BTF header
> >>> perspective) BTF, just doesn't have any types (besides VOID). If we
> >>> don't allow it, then we'll just get an error and then you'll have to
> >>> do painful hex dumping and decoding to see what's wrong.
> >>
> >> It is totally okay to allow empty btf in libbpf. I just want to check
> >> if this btf is going to be loaded into the kernel, right before it is
> >> loading whether libbpf could check whether it is a non-module empty btf
> >> or not, if it is, do not go to kernel.
> >
> > Ok, I see what you are proposing. We can do that, but it's definitely
> > separate from these bug fixes. But, to be honest, I wouldn't bother
> > because libbpf will return BTF verification log with a very readable
> > "No data" message in it.
>
> Right, seems okay to me for this particular case given the user will be
> able to make some sense of it from the log.
>
> Thanks,
> Daniel