On Tue, Jul 30, 2024 at 9:05 PM Andi Kleen <ak@xxxxxxxxxxxxxxx> wrote:
>
> > while (note_offs + sizeof(Elf32_Nhdr) < note_size) {
> > Elf32_Nhdr *nhdr = (Elf32_Nhdr *)(note_start + note_offs);
> >
> > + name_sz = READ_ONCE(nhdr->n_namesz);
> > + desc_sz = READ_ONCE(nhdr->n_descsz);
> > 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 &&
> > + strcmp((char *)(nhdr + 1), note_name) == 0 &&
>
> Doesn't the strcmp need a boundary check to be inside note_size too?
>
> Other it may read into the next page, which could be unmapped, causing a fault.
> Given it's unlikely that this happen, and the end has guard pages,
> but there are some users of set_memory_np.
>
> You could just move the later checks earlier.
Yep, good catch! I'll move the overflow check and will add a note_size
check to it, thanks!
>
> The rest looks good to me.
>
> -Andi
>
