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: <717c1c8c-4194-0605-3aa3-eb33fdc17711@iogearbox.net>
Date:   Tue, 12 Jan 2021 21:17:28 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Yonghong Song <yhs@...com>
Cc:     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 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. ;-)

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