On 4/11/25 18:22, Andrii Nakryiko wrote:
> On Thu, Apr 10, 2025 at 2:55 AM Viktor Malik <vmalik@xxxxxxxxxx> wrote:
>>
>> As reported by CVE-2025-29481 [1], it is possible to corrupt a BPF ELF
>> file such that arbitrary BPF instructions are loaded by libbpf. This can
>> be done by setting a symbol (BPF program) section offset to a large
>> (unsigned) number such that <section start + symbol offset> overflows
>> and points before the section data in the memory.
>>
>> Consider the situation below where:
>> - prog_start = sec_start + symbol_offset <-- size_t overflow here
>> - prog_end = prog_start + prog_size
>>
>> prog_start sec_start prog_end sec_end
>> | | | |
>> v v v v
>> .....................|################################|............
>>
>> The CVE report in [1] also provides a corrupted BPF ELF which can be
>> used as a reproducer:
>>
>> $ readelf -S crash
>> Section Headers:
>> [Nr] Name Type Address Offset
>> Size EntSize Flags Link Info Align
>> ...
>> [ 2] uretprobe.mu[...] PROGBITS 0000000000000000 00000040
>> 0000000000000068 0000000000000000 AX 0 0 8
>>
>> $ readelf -s crash
>> Symbol table '.symtab' contains 8 entries:
>> Num: Value Size Type Bind Vis Ndx Name
>> ...
>> 6: ffffffffffffffb8 104 FUNC GLOBAL DEFAULT 2 handle_tp
>>
>> Here, the handle_tp prog has section offset ffffffffffffffb8, i.e. will
>> point before the actual memory where section 2 is allocated.
>>
>> This is also reported by AddressSanitizer:
>>
>> =================================================================
>> ==1232==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7c7302fe0000 at pc 0x7fc3046e4b77 bp 0x7ffe64677cd0 sp 0x7ffe64677490
>> READ of size 104 at 0x7c7302fe0000 thread T0
>> #0 0x7fc3046e4b76 in memcpy (/lib64/libasan.so.8+0xe4b76)
>> #1 0x00000040df3e in bpf_object__init_prog /src/libbpf/src/libbpf.c:856
>> #2 0x00000040df3e in bpf_object__add_programs /src/libbpf/src/libbpf.c:928
>> #3 0x00000040df3e in bpf_object__elf_collect /src/libbpf/src/libbpf.c:3930
>> #4 0x00000040df3e in bpf_object_open /src/libbpf/src/libbpf.c:8067
>> #5 0x00000040f176 in bpf_object__open_file /src/libbpf/src/libbpf.c:8090
>> #6 0x000000400c16 in main /poc/poc.c:8
>> #7 0x7fc3043d25b4 in __libc_start_call_main (/lib64/libc.so.6+0x35b4)
>> #8 0x7fc3043d2667 in __libc_start_main@@GLIBC_2.34 (/lib64/libc.so.6+0x3667)
>> #9 0x000000400b34 in _start (/poc/poc+0x400b34)
>>
>> 0x7c7302fe0000 is located 64 bytes before 104-byte region [0x7c7302fe0040,0x7c7302fe00a8)
>> allocated by thread T0 here:
>> #0 0x7fc3046e716b in malloc (/lib64/libasan.so.8+0xe716b)
>> #1 0x7fc3045ee600 in __libelf_set_rawdata_wrlock (/lib64/libelf.so.1+0xb600)
>> #2 0x7fc3045ef018 in __elf_getdata_rdlock (/lib64/libelf.so.1+0xc018)
>> #3 0x00000040642f in elf_sec_data /src/libbpf/src/libbpf.c:3740
>>
>> The problem here is that currently, libbpf only checks that the program
>> end is within the section bounds. There used to be a check
>> `while (sec_off < sec_sz)` in bpf_object__add_programs, however, it was
>> removed by commit 6245947c1b3c ("libbpf: Allow gaps in BPF program
>> sections to support overriden weak functions").
>>
>> Put the above condition back to bpf_object__init_prog to make sure that
>> the program start is also within the bounds of the section to avoid the
>> potential buffer overflow.
>>
>> [1] https://github.com/lmarch2/poc/blob/main/libbpf/libbpf.md
>>
>> Reported-by: lmarch2 <2524158037@xxxxxx>
>> Cc: stable@xxxxxxxxxxxxxxx
>
> Libbpf is packaged and consumed from Github mirror, which is produced
> from latest bpf-next and bpf trees, so there is no point in
> backporting fixes like this to stable kernel branches. Please drop the
> CC: stable in the next revision.
Ack, will drop it.
>
>> Fixes: 6245947c1b3c ("libbpf: Allow gaps in BPF program sections to support overriden weak functions")
>> Link: https://github.com/lmarch2/poc/blob/main/libbpf/libbpf.md
>> Link: https://www.cve.org/CVERecord?id=CVE-2025-29481
>
> libbpf is meant to load BPF programs under root. It's a
> highly-privileged operation, and libbpf is not meant, designed, and
> actually explicitly discouraged from loading untrusted ELF files. As
> such, this is just a normal bug fix, like lots of others. So let's
> drop the CVE link as well.
>
> Again, no one in their sane mind should be passing untrusted ELF files
> into libbpf while running under root. Period.
>
> All production use cases load ELF that they generated and control
> (usually embedded into their memory through BPF skeleton header). And
> if that ELF file is corrupted, you have problems somewhere else,
> libbpf is not a culprit.
While I couldn't agree more, I'm a bit on the fence here. On one hand,
unless the CVE is revoked (see the other thread), people may still run
across it and, without sufficient knowledge of libbpf, think that they
are vulnerable. Having a CVE reference in the patch could help them
recognize that they are using a patched version of libbpf or at least
read an explanation why the vulnerability is not real.
On the other hand, since it's just a bug, I agree that it doesn't make
much sense to reference a CVE from it. So, I'm ok both ways. I can
reference the CVE and provide some better explanation why this should
not be considered a vulnerability.
>> Signed-off-by: Viktor Malik <vmalik@xxxxxxxxxx>
>> Reviewed-by: Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx>
>> ---
>> tools/lib/bpf/libbpf.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 6b85060f07b3..d0ece3c9618e 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -896,7 +896,7 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
>> return -LIBBPF_ERRNO__FORMAT;
>> }
>>
>> - if (sec_off + prog_sz > sec_sz) {
>> + if (sec_off >= sec_sz || sec_off + prog_sz > sec_sz) {
>
> So the thing we are protecting against is that sec_off + prog_sz can
> overflow and turn out to be < sec_sz (even though the sum is actually
> bigger), right?
>
> If my understanding is correct, then I'd find it much more obviously
> expressed as:
>
>
> if (sec_off + prog_sz > sec_sz || sec_off + prog_sz < sec_off)
>
> We have such an overflow detection checking pattern used in a few
> places already, I believe. WDYT?
Sure, since we're dealing with unsigned numbers, the above is an
equivalent condition. And you're right that it better expresses the
intent so let me use it.
Thanks!
Viktor
>
> pw-bot: cr
>
>> pr_warn("sec '%s': program at offset %zu crosses section boundary\n",
>> sec_name, sec_off);
>> return -LIBBPF_ERRNO__FORMAT;
>> --
>> 2.49.0
>>
>
