netdev - Re: [PATCH bpf] libbpf: Sanitise internal map names so they are not rejected by the kernel

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:   Wed, 19 Feb 2020 08:45:44 -0800
From:   Yonghong Song <yhs@...com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>,
        Daniel Borkmann <daniel@...earbox.net>, <ast@...com>
CC:     <bpf@...r.kernel.org>, <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf] libbpf: Sanitise internal map names so they are not
 rejected by the kernel



On 2/19/20 2:28 AM, Toke Høiland-Jørgensen wrote:
> Daniel Borkmann <daniel@...earbox.net> writes:
> 
>> On 2/18/20 5:42 PM, Toke Høiland-Jørgensen wrote:
>>> Yonghong Song <yhs@...com> writes:
>>>> On 2/18/20 6:40 AM, Daniel Borkmann wrote:
>>>>> On 2/17/20 6:17 PM, Toke Høiland-Jørgensen wrote:
>>>>>> The kernel only accepts map names with alphanumeric characters,
>>>>>> underscores
>>>>>> and periods in their name. However, the auto-generated internal map names
>>>>>> used by libbpf takes their prefix from the user-supplied BPF object name,
>>>>>> which has no such restriction. This can lead to "Invalid argument" errors
>>>>>> when trying to load a BPF program using global variables.
>>>>>>
>>>>>> Fix this by sanitising the map names, replacing any non-allowed
>>>>>> characters
>>>>>> with underscores.
>>>>>>
>>>>>> Fixes: d859900c4c56 ("bpf, libbpf: support global data/bss/rodata
>>>>>> sections")
>>>>>> Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.com>
>>>>>
>>>>> Makes sense to me, applied, thanks! I presume you had something like '-'
>>>>> in the
>>>>> global var leading to rejection?
>>>>
>>>> The C global variable cannot have '-'. I saw a complain in bcc mailing
>>>> list sometimes back like: if an object file is a-b.o, then we will
>>>> generate a map name like a-b.bss for the bss ELF section data. The
>>>> map name "a-b.bss" name will be rejected by the kernel. The workaround
>>>> is to change object file name. Not sure whether this is the only
>>>> issue which may introduce non [a-zA-Z0-9_] or not. But this patch indeed
>>>> should fix the issue I just described.
>>
>> Yep, meant object file name, just realized too late after sending. :/
>>
>>> Yes, this was exactly my problem; my object file is called
>>> 'xdp-dispatcher.o'. Fun error to track down :P
>>>
>>> Why doesn't the kernel allow dashes in the name anyway?
>>
>> Commit cb4d2b3f03d8 ("bpf: Add name, load_time, uid and map_ids to bpf_prog_info")
>> doesn't state a specific reason, and we did later extend it via 3e0ddc4f3ff1 ("bpf:
>> allow . char as part of the object name"). My best guess right now is potentially
>> not to confuse BPF's kallsyms handling with dashes etc.
> 
> Right, OK, fair enough I suppose. I was just wondering since this is
> the second time I've run into hard-to-debug problems because of the
> naming restrictions.
> 
> Really, it would be nice to have something like the netlink extack
> mechanism so the kernel can return something more than just an error
> code when a bpf() call fails. Is there any way to do something similar
> for a syscall? Could we invent something?

Currently, BPF_PROG_LOAD and BPF_BTF_LOAD has log_buf as part of syscall 
interface. Esp. for BPF_PROG_LOAD, maybe we could put some non-verifier 
logs here?

Maybe we could introduce log_buf to other syscall commands if there is
a great need in user space to get more details about the error code?