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