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: <31ebd16f-8218-1457-b4e2-3728ab147747@fb.com>
Date:   Mon, 11 Jan 2021 17:15:54 -0800
From:   Yonghong Song <yhs@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.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 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.

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

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