* [PATCH v7 bpf-next 01/10] lib/buildid: harden build ID parsing logic
[not found] <20240829174232.3133883-1-andrii@kernel.org>
@ 2024-08-29 17:42 ` Andrii Nakryiko
2024-11-01 13:54 ` Jiri Olsa
0 siblings, 1 reply; 3+ messages in thread
From: Andrii Nakryiko @ 2024-08-29 17:42 UTC (permalink / raw)
To: bpf
Cc: linux-mm, akpm, adobriyan, shakeel.butt, hannes, ak, osandov,
song, jannh, linux-fsdevel, willy, Andrii Nakryiko, stable,
Eduard Zingerman
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@vger.kernel.org
Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>
Reviewed-by: Jann Horn <jannh@google.com>
Suggested-by: Andi Kleen <ak@linux.intel.com>
Fixes: bd7525dacd7e ("bpf: Move stack_map_get_build_id into lib")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
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;
}
- new_offs = note_offs + sizeof(Elf32_Nhdr) +
- ALIGN(nhdr->n_namesz, 4) + ALIGN(nhdr->n_descsz, 4);
- if (new_offs <= note_offs) /* overflow */
- break;
- note_offs = new_offs;
+
+ note_off = new_off;
}
return -EINVAL;
@@ -71,7 +77,7 @@ static int get_build_id_32(const void *page_addr, unsigned char *build_id,
{
Elf32_Ehdr *ehdr = (Elf32_Ehdr *)page_addr;
Elf32_Phdr *phdr;
- int i;
+ __u32 i, phnum;
/*
* FIXME
@@ -80,18 +86,19 @@ static int get_build_id_32(const void *page_addr, unsigned char *build_id,
*/
if (ehdr->e_phoff != sizeof(Elf32_Ehdr))
return -EINVAL;
+
+ phnum = READ_ONCE(ehdr->e_phnum);
/* only supports phdr that fits in one page */
- if (ehdr->e_phnum >
- (PAGE_SIZE - sizeof(Elf32_Ehdr)) / sizeof(Elf32_Phdr))
+ if (phnum > (PAGE_SIZE - sizeof(Elf32_Ehdr)) / sizeof(Elf32_Phdr))
return -EINVAL;
phdr = (Elf32_Phdr *)(page_addr + sizeof(Elf32_Ehdr));
- for (i = 0; i < ehdr->e_phnum; ++i) {
+ for (i = 0; i < phnum; ++i) {
if (phdr[i].p_type == PT_NOTE &&
!parse_build_id(page_addr, build_id, size,
- page_addr + phdr[i].p_offset,
- phdr[i].p_filesz))
+ page_addr + READ_ONCE(phdr[i].p_offset),
+ READ_ONCE(phdr[i].p_filesz)))
return 0;
}
return -EINVAL;
@@ -103,7 +110,7 @@ static int get_build_id_64(const void *page_addr, unsigned char *build_id,
{
Elf64_Ehdr *ehdr = (Elf64_Ehdr *)page_addr;
Elf64_Phdr *phdr;
- int i;
+ __u32 i, phnum;
/*
* FIXME
@@ -112,18 +119,19 @@ static int get_build_id_64(const void *page_addr, unsigned char *build_id,
*/
if (ehdr->e_phoff != sizeof(Elf64_Ehdr))
return -EINVAL;
+
+ phnum = READ_ONCE(ehdr->e_phnum);
/* only supports phdr that fits in one page */
- if (ehdr->e_phnum >
- (PAGE_SIZE - sizeof(Elf64_Ehdr)) / sizeof(Elf64_Phdr))
+ if (phnum > (PAGE_SIZE - sizeof(Elf64_Ehdr)) / sizeof(Elf64_Phdr))
return -EINVAL;
phdr = (Elf64_Phdr *)(page_addr + sizeof(Elf64_Ehdr));
- for (i = 0; i < ehdr->e_phnum; ++i) {
+ for (i = 0; i < phnum; ++i) {
if (phdr[i].p_type == PT_NOTE &&
!parse_build_id(page_addr, build_id, size,
- page_addr + phdr[i].p_offset,
- phdr[i].p_filesz))
+ page_addr + READ_ONCE(phdr[i].p_offset),
+ READ_ONCE(phdr[i].p_filesz)))
return 0;
}
return -EINVAL;
@@ -152,6 +160,10 @@ int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
page = find_get_page(vma->vm_file->f_mapping, 0);
if (!page)
return -EFAULT; /* page not mapped */
+ if (!PageUptodate(page)) {
+ put_page(page);
+ return -EFAULT;
+ }
ret = -EINVAL;
page_addr = kmap_local_page(page);
--
2.43.5
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v7 bpf-next 01/10] lib/buildid: harden build ID parsing logic
2024-08-29 17:42 ` [PATCH v7 bpf-next 01/10] lib/buildid: harden build ID parsing logic Andrii Nakryiko
@ 2024-11-01 13:54 ` Jiri Olsa
2024-11-01 18:12 ` Andrii Nakryiko
0 siblings, 1 reply; 3+ messages in thread
From: Jiri Olsa @ 2024-11-01 13:54 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, linux-mm, akpm, adobriyan, shakeel.butt, hannes, ak, osandov,
song, jannh, linux-fsdevel, willy, stable, Eduard Zingerman
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@vger.kernel.org
> Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>
> Reviewed-by: Jann Horn <jannh@google.com>
> Suggested-by: Andi Kleen <ak@linux.intel.com>
> Fixes: bd7525dacd7e ("bpf: Move stack_map_get_build_id into lib")
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
> 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@kernel.org>
---
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);
memcpy(build_id, data, desc_sz);
memset(build_id + desc_sz, 0, BUILD_ID_SIZE_MAX - desc_sz);
if (size)
--
2.47.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v7 bpf-next 01/10] lib/buildid: harden build ID parsing logic
2024-11-01 13:54 ` Jiri Olsa
@ 2024-11-01 18:12 ` Andrii Nakryiko
0 siblings, 0 replies; 3+ messages in thread
From: Andrii Nakryiko @ 2024-11-01 18:12 UTC (permalink / raw)
To: Jiri Olsa
Cc: Andrii Nakryiko, bpf, linux-mm, akpm, adobriyan, shakeel.butt,
hannes, ak, osandov, song, jannh, linux-fsdevel, willy, stable,
Eduard Zingerman
On Fri, Nov 1, 2024 at 6:54 AM Jiri Olsa <olsajiri@gmail.com> 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@vger.kernel.org
> > Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>
> > Reviewed-by: Jann Horn <jannh@google.com>
> > Suggested-by: Andi Kleen <ak@linux.intel.com>
> > Fixes: bd7525dacd7e ("bpf: Move stack_map_get_build_id into lib")
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> > 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@kernel.org>
> ---
> 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@kernel.org>
> memcpy(build_id, data, desc_sz);
> memset(build_id + desc_sz, 0, BUILD_ID_SIZE_MAX - desc_sz);
> if (size)
> --
> 2.47.0
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-11-01 18:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20240829174232.3133883-1-andrii@kernel.org>
2024-08-29 17:42 ` [PATCH v7 bpf-next 01/10] lib/buildid: harden build ID parsing logic Andrii Nakryiko
2024-11-01 13:54 ` Jiri Olsa
2024-11-01 18:12 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox