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]

Date:   Mon, 11 Jan 2021 22:41:19 -0800
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>,
        Christopher William Snowhill <chris@...e54.net>
Subject: Re: [PATCH bpf 2/2] libbpf: allow loading empty BTFs

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")
> >>> 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.

>
> >
> > In practice, no BPF program's BTF should be empty, but if it is, the
> > kernel will rightfully stop you. I don't think it's a common enough
> > case for libbpf to handle.
>
> In general, libbpf should catch errors earlier if possible without going
> to kernel. This way, we can have better error messages for user.
> But I won't insist in this case as it is indeed really rare.

I wouldn't say in general. Rather in cases that commonly would cause
confusion. I don't think libbpf should grow into a massive "let's
double check everything before kernel" thing.

>
> >
> >>
> >>> -
> >>>        if (meta_left < hdr->str_off + hdr->str_len) {
> >>>                pr_debug("Invalid BTF total size:%u\n", btf->raw_size);
> >>>                return -EINVAL;
> >>>