| Message ID | 20250410073407.131211-1-vmalik@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf] libbpf: Fix buffer overflow in bpf_object__init_prog | expand |
I was about to sent my fix, just to realize I got beaten by half and hour+ even with my 6 hour timezone advantage :) On Thu, Apr 10, 2025 at 09:34:07AM +0200, Viktor Malik wrote: > As reported by CVE-2025-29481 (link below), 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 > sufficiently large (unsigned) number such <section_start+symbol_offset> > overflows and points before the section. > > 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 > .....................|################################|............ > > Currently, libbpf only checks that prog_end is within the section > bounds. Add a check that prog_start is also within the bounds to avoid > the potential buffer overflow. I would add Reported-by: lmarch2 <2524158037@qq.com> Cc: stable@vger.kernel.org Fixes: 6245947c1b3c ("libbpf: Allow gaps in BPF program sections to support overriden weak functions") There used to be a 'while (sec_off < sec_sz)' check that would prevent this issue, but with commit 6245947c1b3c that was removed. --- Nit: it would be nice if some concrete values from the reproducer is included Section Headers: [Nr] Name Type Address Off Size ES Flg Lk Inf Al ... [ 2] uretprobe.multi.snter_write PROGBITS 0000000000000000 000040 000068 00 AX 0 0 8 Symbol table '.symtab' contains 8 entries: Num: Value Size Type Bind Vis Ndx Name ... 6: ffffffffffffffb8 104 FUNC GLOBAL DEFAULT 2 handle_tp As well as AddressSanitizer output: libbpf: loading object from crash-04573b0232eeaed1b2cd9f10e4fadc122c560e7a libbpf: elf: section(2) uretprobe.multi.snter_write, size 104, link 0, flags 6, type=1 libbpf: sec 'uretprobe.multi.snter_write': found program 'handle_tp' at insn offset 0 (0 bytes), code size 13 insns (104 bytes) libbpf: sec 'uretprobe.multi.snter_write': found program 'handle_tp' at insn offset 2305843009213693943 (18446744073709551544 bytes), code size 13 insns (104 bytes) ================================================================= ==169293==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7c1fc6be0000 at pc 0x7f6fc831f877 bp 0x7ffc89800a30 sp 0x7ffc898001f0 READ of size 104 at 0x7c1fc6be0000 thread T0 #0 0x7f6fc831f876 in memcpy (/lib64/libasan.so.8+0x11f876) (BuildId: 7a83eb8b5639d83795773bfac12481d6f3243469) #1 0x00000040fcbf in bpf_object__init_prog ./tools/lib/bpf/libbpf.c:856 #2 0x00000040fcbf in bpf_object__add_programs ./tools/lib/bpf/libbpf.c:928 #3 0x00000040fcbf in bpf_object__elf_collect ./tools/lib/bpf/libbpf.c:3930 #4 0x00000040fcbf in bpf_object_open ./tools/lib/bpf/libbpf.c:8067 #5 0x000000411b83 in bpf_object__open_file ./tools/lib/bpf/libbpf.c:8090 #6 0x000000403966 in main (../poc/libbpf/poc+0x403966) (BuildId: 9d80b3f3edc46b2a3684427aad5fe2bcda2b5ea4) ... > Link: https://github.com/lmarch2/poc/blob/main/libbpf/libbpf.md > Link: https://www.cve.org/CVERecord?id=CVE-2025-29481 > Signed-off-by: Viktor Malik <vmalik@redhat.com> Code-wise LGTM Reviewed-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
On 4/10/25 10:16, Shung-Hsi Yu wrote: > I was about to sent my fix, just to realize I got beaten by half and > hour+ even with my 6 hour timezone advantage :) :) > > On Thu, Apr 10, 2025 at 09:34:07AM +0200, Viktor Malik wrote: >> As reported by CVE-2025-29481 (link below), 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 >> sufficiently large (unsigned) number such <section_start+symbol_offset> >> overflows and points before the section. >> >> 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 >> .....................|################################|............ >> >> Currently, libbpf only checks that prog_end is within the section >> bounds. Add a check that prog_start is also within the bounds to avoid >> the potential buffer overflow. > > I would add > > Reported-by: lmarch2 <2524158037@qq.com> > Cc: stable@vger.kernel.org > Fixes: 6245947c1b3c ("libbpf: Allow gaps in BPF program sections to support overriden weak functions") > > There used to be a 'while (sec_off < sec_sz)' check that would prevent > this issue, but with commit 6245947c1b3c that was removed. > > --- > > Nit: it would be nice if some concrete values from the reproducer is > included > > Section Headers: > [Nr] Name Type Address Off Size ES Flg Lk Inf Al > ... > [ 2] uretprobe.multi.snter_write PROGBITS 0000000000000000 000040 000068 00 AX 0 0 8 > > Symbol table '.symtab' contains 8 entries: > Num: Value Size Type Bind Vis Ndx Name > ... > 6: ffffffffffffffb8 104 FUNC GLOBAL DEFAULT 2 handle_tp > > As well as AddressSanitizer output: > > libbpf: loading object from crash-04573b0232eeaed1b2cd9f10e4fadc122c560e7a > libbpf: elf: section(2) uretprobe.multi.snter_write, size 104, link 0, flags 6, type=1 > libbpf: sec 'uretprobe.multi.snter_write': found program 'handle_tp' at insn offset 0 (0 bytes), code size 13 insns (104 bytes) > libbpf: sec 'uretprobe.multi.snter_write': found program 'handle_tp' at insn offset 2305843009213693943 (18446744073709551544 bytes), code size 13 insns (104 bytes) > ================================================================= > ==169293==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7c1fc6be0000 at pc 0x7f6fc831f877 bp 0x7ffc89800a30 sp 0x7ffc898001f0 > READ of size 104 at 0x7c1fc6be0000 thread T0 > #0 0x7f6fc831f876 in memcpy (/lib64/libasan.so.8+0x11f876) (BuildId: 7a83eb8b5639d83795773bfac12481d6f3243469) > #1 0x00000040fcbf in bpf_object__init_prog ./tools/lib/bpf/libbpf.c:856 > #2 0x00000040fcbf in bpf_object__add_programs ./tools/lib/bpf/libbpf.c:928 > #3 0x00000040fcbf in bpf_object__elf_collect ./tools/lib/bpf/libbpf.c:3930 > #4 0x00000040fcbf in bpf_object_open ./tools/lib/bpf/libbpf.c:8067 > #5 0x000000411b83 in bpf_object__open_file ./tools/lib/bpf/libbpf.c:8090 > #6 0x000000403966 in main (../poc/libbpf/poc+0x403966) (BuildId: 9d80b3f3edc46b2a3684427aad5fe2bcda2b5ea4) > ... > These are great suggestions, I'll add them to the commit message and will send v2. Thanks! Viktor >> Link: https://github.com/lmarch2/poc/blob/main/libbpf/libbpf.md >> Link: https://www.cve.org/CVERecord?id=CVE-2025-29481 >> Signed-off-by: Viktor Malik <vmalik@redhat.com> > > Code-wise LGTM > > Reviewed-by: Shung-Hsi Yu <shung-hsi.yu@suse.com> >
On Thu, Apr 10, 2025 at 10:56:19AM +0200, Viktor Malik wrote: > On 4/10/25 10:16, Shung-Hsi Yu wrote: > > I was about to sent my fix, just to realize I got beaten by half and > > hour+ even with my 6 hour timezone advantage :) > > :) > > > On Thu, Apr 10, 2025 at 09:34:07AM +0200, Viktor Malik wrote: > >> As reported by CVE-2025-29481 (link below), 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 > >> sufficiently large (unsigned) number such <section_start+symbol_offset> > >> overflows and points before the section. > >> > >> 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 > >> .....................|################################|............ > >> > >> Currently, libbpf only checks that prog_end is within the section > >> bounds. Add a check that prog_start is also within the bounds to avoid > >> the potential buffer overflow. Looking this again, I realize the above does not exactly describe the code change. The 'sec_off >= sec_sz' check was instead a check for the following situation: sec_start sec_end prog_start | | | v v v .....|################################|............... And it was symbol_offset/sec_off + prog_size/prog_sz that overflowed when running the reproducer, not sec_start/data + symbol_offset/sec_off, though maybe that could still happen further down in the call to bpf_object__init_prog(), I'm not sure. The change does indeed address the issue surface by the reproducer though. Just different from what the above describes. Shung-Hsi > 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) { > pr_warn("sec '%s': program at offset %zu crosses section boundary\n", > sec_name, sec_off); > return -LIBBPF_ERRNO__FORMAT; ...
On 4/10/25 12:09, Shung-Hsi Yu wrote: > On Thu, Apr 10, 2025 at 10:56:19AM +0200, Viktor Malik wrote: >> On 4/10/25 10:16, Shung-Hsi Yu wrote: >>> I was about to sent my fix, just to realize I got beaten by half and >>> hour+ even with my 6 hour timezone advantage :) >> >> :) >> >>> On Thu, Apr 10, 2025 at 09:34:07AM +0200, Viktor Malik wrote: >>>> As reported by CVE-2025-29481 (link below), 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 >>>> sufficiently large (unsigned) number such <section_start+symbol_offset> >>>> overflows and points before the section. >>>> >>>> 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 >>>> .....................|################################|............ >>>> >>>> Currently, libbpf only checks that prog_end is within the section >>>> bounds. Add a check that prog_start is also within the bounds to avoid >>>> the potential buffer overflow. > > Looking this again, I realize the above does not exactly describe the > code change. The 'sec_off >= sec_sz' check was instead a check for the > following situation: > > sec_start sec_end prog_start > | | | > v v v > .....|################################|............... > > And it was symbol_offset/sec_off + prog_size/prog_sz that overflowed > when running the reproducer, not sec_start/data + symbol_offset/sec_off, Here are the relevant parts of the bpf_object__add_programs function: static int bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data, const char *sec_name, int sec_idx) { ... void *data = sec_data->d_buf; ... for (i = 0; i < nr_syms; i++) { ... prog_sz = sym->st_size; sec_off = sym->st_value; ... if (sec_off + prog_sz > sec_sz) { pr_warn("sec '%s': program at offset %zu crosses section boundary\n", sec_name, sec_off); return -LIBBPF_ERRNO__FORMAT; } ... err = bpf_object__init_prog(obj, prog, name, sec_idx, sec_name, sec_off, data + sec_off, prog_sz); ... } } You are correct that `sec_off + prog_sz` overflows and therefore bypasses the existing check. However, the actual problem is IMO elsewhere. Above, sec_data->d_buf points to a memory buffer allocated by libelf which holds the section data. Therefore, `data + sec_off` passed to bpf_object__init_prog will overflow (as sec_off is 0xffffffffffffffb8) and point before the buffer itself. This is also what AddressSanitizer reports by: 0x7c7302fe0000 is located 64 bytes before 104-byte region [0x7c7302fe0040,0x7c7302fe00a8) Next, bpf_object__init_prog does: static int bpf_object__init_prog(struct bpf_object *obj, struct bpf_program *prog, const char *name, size_t sec_idx, const char *sec_name, size_t sec_off, void *insn_data, size_t insn_data_sz) { [...] memcpy(prog->insns, insn_data, insn_data_sz); [...] } which is where the buffer overflow happens as insn_data points before the memory buffer holding the section. So, I believe that the actual problem stems from the overflow of `data + sec_off` (i.e. prog_start) and therefore my description is accurate. Viktor > though maybe that could still happen further down in the call to > bpf_object__init_prog(), I'm not sure. > > The change does indeed address the issue surface by the reproducer > though. Just different from what the above describes. > > > Shung-Hsi > >> 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) { >> pr_warn("sec '%s': program at offset %zu crosses section boundary\n", >> sec_name, sec_off); >> return -LIBBPF_ERRNO__FORMAT; > > ... >
On Thu, Apr 10, 2025 at 01:04:49PM +0200, Viktor Malik wrote: > On 4/10/25 12:09, Shung-Hsi Yu wrote: > > On Thu, Apr 10, 2025 at 10:56:19AM +0200, Viktor Malik wrote: > >> On 4/10/25 10:16, Shung-Hsi Yu wrote: > >>> I was about to sent my fix, just to realize I got beaten by half and > >>> hour+ even with my 6 hour timezone advantage :) > >> > >> :) > >> > >>> On Thu, Apr 10, 2025 at 09:34:07AM +0200, Viktor Malik wrote: > >>>> As reported by CVE-2025-29481 (link below), 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 > >>>> sufficiently large (unsigned) number such <section_start+symbol_offset> > >>>> overflows and points before the section. > >>>> > >>>> 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 > >>>> .....................|################################|............ > >>>> > >>>> Currently, libbpf only checks that prog_end is within the section > >>>> bounds. Add a check that prog_start is also within the bounds to avoid > >>>> the potential buffer overflow. > > > > Looking this again, I realize the above does not exactly describe the > > code change. The 'sec_off >= sec_sz' check was instead a check for the > > following situation: > > > > sec_start sec_end prog_start > > | | | > > v v v > > .....|################################|............... > > > > And it was symbol_offset/sec_off + prog_size/prog_sz that overflowed > > when running the reproducer, not sec_start/data + symbol_offset/sec_off, > > Here are the relevant parts of the bpf_object__add_programs function: > > static int > bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data, > const char *sec_name, int sec_idx) > { > ... > void *data = sec_data->d_buf; > ... > for (i = 0; i < nr_syms; i++) { > ... > prog_sz = sym->st_size; > sec_off = sym->st_value; > ... > if (sec_off + prog_sz > sec_sz) { > pr_warn("sec '%s': program at offset %zu crosses section boundary\n", > sec_name, sec_off); > return -LIBBPF_ERRNO__FORMAT; > } > ... > err = bpf_object__init_prog(obj, prog, name, sec_idx, sec_name, > sec_off, data + sec_off, prog_sz); > ... > } > } > > You are correct that `sec_off + prog_sz` overflows and therefore > bypasses the existing check. > > However, the actual problem is IMO elsewhere. Above, sec_data->d_buf > points to a memory buffer allocated by libelf which holds the section > data. Therefore, `data + sec_off` passed to bpf_object__init_prog will > overflow (as sec_off is 0xffffffffffffffb8) and point before the buffer > itself. This is also what AddressSanitizer reports by: > > 0x7c7302fe0000 is located 64 bytes before 104-byte region [0x7c7302fe0040,0x7c7302fe00a8) > > Next, bpf_object__init_prog does: > > static int > bpf_object__init_prog(struct bpf_object *obj, struct bpf_program *prog, > const char *name, size_t sec_idx, const char *sec_name, > size_t sec_off, void *insn_data, size_t insn_data_sz) > { > [...] > memcpy(prog->insns, insn_data, insn_data_sz); > [...] > } > > which is where the buffer overflow happens as insn_data points before > the memory buffer holding the section. > > So, I believe that the actual problem stems from the overflow of > `data + sec_off` (i.e. prog_start) and therefore my description is > accurate. Okay, I see that now, `data + sec_off` overflow is the root cause indeed. And in the bigger picture when *data/sec_data->d_buf is considered, the diagram is correct. I think I just have a hard time getting over the fact that the 'sec_off >= sec_sz' check, when taken literally and out of context, is checking whether the program's start offset goes _beyond_ the section size. No suggestion comes to mind though, and not a blocker. > Viktor > > > though maybe that could still happen further down in the call to > > bpf_object__init_prog(), I'm not sure. > > > > The change does indeed address the issue surface by the reproducer > > though. Just different from what the above describes. > > > > Shung-Hsi > > > >> 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) { > >> pr_warn("sec '%s': program at offset %zu crosses section boundary\n", > >> sec_name, sec_off); > >> return -LIBBPF_ERRNO__FORMAT; > > > > ...
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) { pr_warn("sec '%s': program at offset %zu crosses section boundary\n", sec_name, sec_off); return -LIBBPF_ERRNO__FORMAT;
As reported by CVE-2025-29481 (link below), 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 sufficiently large (unsigned) number such <section_start+symbol_offset> overflows and points before the section. 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 .....................|################################|............ Currently, libbpf only checks that prog_end is within the section bounds. Add a check that prog_start is also within the bounds to avoid the potential buffer overflow. Link: https://github.com/lmarch2/poc/blob/main/libbpf/libbpf.md Link: https://www.cve.org/CVERecord?id=CVE-2025-29481 Signed-off-by: Viktor Malik <vmalik@redhat.com> --- tools/lib/bpf/libbpf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)