On Fri, Nov 1, 2024 at 6:54 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote: > > On Thu, Aug 29, 2024 at 10:42:23AM -0700, Andrii Nakryiko wrote: > > Harden build ID parsing logic, adding explicit READ_ONCE() where it's > > important to have a consistent value read and validated just once. > > > > Also, as pointed out by Andi Kleen, we need to make sure that entire ELF > > note is within a page bounds, so move the overflow check up and add an > > extra note_size boundaries validation. > > > > Fixes tag below points to the code that moved this code into > > lib/buildid.c, and then subsequently was used in perf subsystem, making > > this code exposed to perf_event_open() users in v5.12+. > > > > Cc: stable@xxxxxxxxxxxxxxx > > Reviewed-by: Eduard Zingerman <eddyz87@xxxxxxxxx> > > Reviewed-by: Jann Horn <jannh@xxxxxxxxxx> > > Suggested-by: Andi Kleen <ak@xxxxxxxxxxxxxxx> > > Fixes: bd7525dacd7e ("bpf: Move stack_map_get_build_id into lib") > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > > --- > > lib/buildid.c | 76 +++++++++++++++++++++++++++++---------------------- > > 1 file changed, 44 insertions(+), 32 deletions(-) > > > > diff --git a/lib/buildid.c b/lib/buildid.c > > index e02b5507418b..26007cc99a38 100644 > > --- a/lib/buildid.c > > +++ b/lib/buildid.c > > @@ -18,31 +18,37 @@ static int parse_build_id_buf(unsigned char *build_id, > > const void *note_start, > > Elf32_Word note_size) > > { > > - Elf32_Word note_offs = 0, new_offs; > > - > > - while (note_offs + sizeof(Elf32_Nhdr) < note_size) { > > - Elf32_Nhdr *nhdr = (Elf32_Nhdr *)(note_start + note_offs); > > + const char note_name[] = "GNU"; > > + const size_t note_name_sz = sizeof(note_name); > > + u64 note_off = 0, new_off, name_sz, desc_sz; > > + const char *data; > > + > > + while (note_off + sizeof(Elf32_Nhdr) < note_size && > > + note_off + sizeof(Elf32_Nhdr) > note_off /* overflow */) { > > + Elf32_Nhdr *nhdr = (Elf32_Nhdr *)(note_start + note_off); > > + > > + name_sz = READ_ONCE(nhdr->n_namesz); > > + desc_sz = READ_ONCE(nhdr->n_descsz); > > + > > + new_off = note_off + sizeof(Elf32_Nhdr); > > + if (check_add_overflow(new_off, ALIGN(name_sz, 4), &new_off) || > > + check_add_overflow(new_off, ALIGN(desc_sz, 4), &new_off) || > > + new_off > note_size) > > + break; > > > > if (nhdr->n_type == BUILD_ID && > > - nhdr->n_namesz == sizeof("GNU") && > > - !strcmp((char *)(nhdr + 1), "GNU") && > > - nhdr->n_descsz > 0 && > > - nhdr->n_descsz <= BUILD_ID_SIZE_MAX) { > > - memcpy(build_id, > > - note_start + note_offs + > > - ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_Nhdr), > > - nhdr->n_descsz); > > - memset(build_id + nhdr->n_descsz, 0, > > - BUILD_ID_SIZE_MAX - nhdr->n_descsz); > > + name_sz == note_name_sz && > > + memcmp(nhdr + 1, note_name, note_name_sz) == 0 && > > + desc_sz > 0 && desc_sz <= BUILD_ID_SIZE_MAX) { > > + data = note_start + note_off + ALIGN(note_name_sz, 4); > > + memcpy(build_id, data, desc_sz); > > + memset(build_id + desc_sz, 0, BUILD_ID_SIZE_MAX - desc_sz); > > if (size) > > - *size = nhdr->n_descsz; > > + *size = desc_sz; > > return 0; > > } > > hi, > this fix is causing stable kernels to return wrong build id, > the change below seems to fix that (based on 6.6 stable) > > if we agree on the fix I'll send it to all affected stable trees > > jirka > > > --- > The parse_build_id_buf does not account Elf32_Nhdr header size > when getting the build id data pointer and returns wrong build > id data as result. > > This is problem only stable trees that merged c83a80d8b84f fix, > the upstream build id code was refactored and returns proper > build id. > > Fixes: c83a80d8b84f ("lib/buildid: harden build ID parsing logic") > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx> > --- > lib/buildid.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/buildid.c b/lib/buildid.c > index d3bc3d0528d5..9fc46366597e 100644 > --- a/lib/buildid.c > +++ b/lib/buildid.c > @@ -40,7 +40,7 @@ static int parse_build_id_buf(unsigned char *build_id, > name_sz == note_name_sz && > memcmp(nhdr + 1, note_name, note_name_sz) == 0 && > desc_sz > 0 && desc_sz <= BUILD_ID_SIZE_MAX) { > - data = note_start + note_off + ALIGN(note_name_sz, 4); > + data = note_start + note_off + sizeof(Elf32_Nhdr) + ALIGN(note_name_sz, 4); ah, my screw up, sorry. LGTM Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > memcpy(build_id, data, desc_sz); > memset(build_id + desc_sz, 0, BUILD_ID_SIZE_MAX - desc_sz); > if (size) > -- > 2.47.0 >