diff mbox series

[v2,bpf-next,02/10] lib/buildid: take into account e_phoff when fetching program headers

Message ID 20240724225210.545423-3-andrii@kernel.org (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Harden and extend ELF build ID parsing logic | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 661 this patch: 661
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 2 of 2 maintainers
netdev/build_clang success Errors and warnings before: 662 this patch: 662
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 662 this patch: 662
netdev/checkpatch warning WARNING: line length of 92 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-15 fail Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc

Commit Message

Andrii Nakryiko July 24, 2024, 10:52 p.m. UTC 
Current code assumption is that program (segment) headers are following
ELF header immediately. This is a common case, but is not guaranteed. So
take into account e_phoff field of the ELF header when accessing program
headers.

Reported-by: Alexey Dobriyan <adobriyan@gmail.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 lib/buildid.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Jiri Olsa July 25, 2024, 12:03 p.m. UTC | #1 
On Wed, Jul 24, 2024 at 03:52:02PM -0700, Andrii Nakryiko wrote:
> Current code assumption is that program (segment) headers are following
> ELF header immediately. This is a common case, but is not guaranteed. So
> take into account e_phoff field of the ELF header when accessing program
> headers.
> 
> Reported-by: Alexey Dobriyan <adobriyan@gmail.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

looks like this one never got in right?
  https://lore.kernel.org/bpf/CAEf4BzaAKAwO=-=0qZQfkHhBodN0MQUHpL-RY7tCHdcFidjv-Q@mail.gmail.com/

I couldn't find the place where you remove that check ;-)

jirka

> ---
>  lib/buildid.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/buildid.c b/lib/buildid.c
> index 1442a2483a8b..ce48ffab4111 100644
> --- a/lib/buildid.c
> +++ b/lib/buildid.c
> @@ -206,7 +206,7 @@ static int get_build_id_32(struct freader *r, unsigned char *build_id, __u32 *si
>  {
>  	const Elf32_Ehdr *ehdr;
>  	const Elf32_Phdr *phdr;
> -	__u32 phnum, i;
> +	__u32 phnum, phoff, i;
>  
>  	ehdr = freader_fetch(r, 0, sizeof(Elf32_Ehdr));
>  	if (!ehdr)
> @@ -214,13 +214,14 @@ static int get_build_id_32(struct freader *r, unsigned char *build_id, __u32 *si
>  
>  	/* subsequent freader_fetch() calls invalidate pointers, so remember locally */
>  	phnum = ehdr->e_phnum;
> +	phoff = READ_ONCE(ehdr->e_phoff);
>  
>  	/* only supports phdr that fits in one page */
>  	if (phnum > (PAGE_SIZE - sizeof(Elf32_Ehdr)) / sizeof(Elf32_Phdr))
>  		return -EINVAL;
>  
>  	for (i = 0; i < phnum; ++i) {
> -		phdr = freader_fetch(r, i * sizeof(Elf32_Phdr), sizeof(Elf32_Phdr));
> +		phdr = freader_fetch(r, phoff + i * sizeof(Elf32_Phdr), sizeof(Elf32_Phdr));
>  		if (!phdr)
>  			return r->err;
>  
> @@ -237,6 +238,7 @@ static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si
>  	const Elf64_Ehdr *ehdr;
>  	const Elf64_Phdr *phdr;
>  	__u32 phnum, i;
> +	__u64 phoff;
>  
>  	ehdr = freader_fetch(r, 0, sizeof(Elf64_Ehdr));
>  	if (!ehdr)
> @@ -244,13 +246,14 @@ static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si
>  
>  	/* subsequent freader_fetch() calls invalidate pointers, so remember locally */
>  	phnum = ehdr->e_phnum;
> +	phoff = READ_ONCE(ehdr->e_phoff);
>  
>  	/* only supports phdr that fits in one page */
>  	if (phnum > (PAGE_SIZE - sizeof(Elf64_Ehdr)) / sizeof(Elf64_Phdr))
>  		return -EINVAL;
>  
>  	for (i = 0; i < phnum; ++i) {
> -		phdr = freader_fetch(r, i * sizeof(Elf64_Phdr), sizeof(Elf64_Phdr));
> +		phdr = freader_fetch(r, phoff + i * sizeof(Elf64_Phdr), sizeof(Elf64_Phdr));
>  		if (!phdr)
>  			return r->err;
>  
> -- 
> 2.43.0
> 
>
Andrii Nakryiko July 25, 2024, 7:59 p.m. UTC | #2 
On Thu, Jul 25, 2024 at 5:03 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Wed, Jul 24, 2024 at 03:52:02PM -0700, Andrii Nakryiko wrote:
> > Current code assumption is that program (segment) headers are following
> > ELF header immediately. This is a common case, but is not guaranteed. So
> > take into account e_phoff field of the ELF header when accessing program
> > headers.
> >
> > Reported-by: Alexey Dobriyan <adobriyan@gmail.com>
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>
> looks like this one never got in right?
>   https://lore.kernel.org/bpf/CAEf4BzaAKAwO=-=0qZQfkHhBodN0MQUHpL-RY7tCHdcFidjv-Q@mail.gmail.com/
>
> I couldn't find the place where you remove that check ;-)
>

yeah, I don't think that landed. And this patch set will supersede
that change with a proper support.

> jirka
>
> > ---
> >  lib/buildid.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/buildid.c b/lib/buildid.c
> > index 1442a2483a8b..ce48ffab4111 100644
> > --- a/lib/buildid.c
> > +++ b/lib/buildid.c
> > @@ -206,7 +206,7 @@ static int get_build_id_32(struct freader *r, unsigned char *build_id, __u32 *si
> >  {
> >       const Elf32_Ehdr *ehdr;
> >       const Elf32_Phdr *phdr;
> > -     __u32 phnum, i;
> > +     __u32 phnum, phoff, i;
> >
> >       ehdr = freader_fetch(r, 0, sizeof(Elf32_Ehdr));
> >       if (!ehdr)
> > @@ -214,13 +214,14 @@ static int get_build_id_32(struct freader *r, unsigned char *build_id, __u32 *si
> >
> >       /* subsequent freader_fetch() calls invalidate pointers, so remember locally */
> >       phnum = ehdr->e_phnum;
> > +     phoff = READ_ONCE(ehdr->e_phoff);
> >
> >       /* only supports phdr that fits in one page */
> >       if (phnum > (PAGE_SIZE - sizeof(Elf32_Ehdr)) / sizeof(Elf32_Phdr))
> >               return -EINVAL;
> >
> >       for (i = 0; i < phnum; ++i) {
> > -             phdr = freader_fetch(r, i * sizeof(Elf32_Phdr), sizeof(Elf32_Phdr));
> > +             phdr = freader_fetch(r, phoff + i * sizeof(Elf32_Phdr), sizeof(Elf32_Phdr));
> >               if (!phdr)
> >                       return r->err;
> >
> > @@ -237,6 +238,7 @@ static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si
> >       const Elf64_Ehdr *ehdr;
> >       const Elf64_Phdr *phdr;
> >       __u32 phnum, i;
> > +     __u64 phoff;
> >
> >       ehdr = freader_fetch(r, 0, sizeof(Elf64_Ehdr));
> >       if (!ehdr)
> > @@ -244,13 +246,14 @@ static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si
> >
> >       /* subsequent freader_fetch() calls invalidate pointers, so remember locally */
> >       phnum = ehdr->e_phnum;
> > +     phoff = READ_ONCE(ehdr->e_phoff);
> >
> >       /* only supports phdr that fits in one page */
> >       if (phnum > (PAGE_SIZE - sizeof(Elf64_Ehdr)) / sizeof(Elf64_Phdr))
> >               return -EINVAL;
> >
> >       for (i = 0; i < phnum; ++i) {
> > -             phdr = freader_fetch(r, i * sizeof(Elf64_Phdr), sizeof(Elf64_Phdr));
> > +             phdr = freader_fetch(r, phoff + i * sizeof(Elf64_Phdr), sizeof(Elf64_Phdr));
> >               if (!phdr)
> >                       return r->err;
> >
> > --
> > 2.43.0
> >
> >
Andi Kleen July 25, 2024, 10:45 p.m. UTC | #3 
> @@ -214,13 +214,14 @@ static int get_build_id_32(struct freader *r, unsigned char *build_id, __u32 *si
>  
>  	/* subsequent freader_fetch() calls invalidate pointers, so remember locally */
>  	phnum = ehdr->e_phnum;
> +	phoff = READ_ONCE(ehdr->e_phoff);
>  
>  	/* only supports phdr that fits in one page */
>  	if (phnum > (PAGE_SIZE - sizeof(Elf32_Ehdr)) / sizeof(Elf32_Phdr))
>  		return -EINVAL;
>  
>  	for (i = 0; i < phnum; ++i) {
> -		phdr = freader_fetch(r, i * sizeof(Elf32_Phdr), sizeof(Elf32_Phdr));
> +		phdr = freader_fetch(r, phoff + i * sizeof(Elf32_Phdr), sizeof(Elf32_Phdr));

What happens if phoff is big enough that this computation wraps?
Andrii Nakryiko July 27, 2024, 12:30 a.m. UTC | #4 
On Thu, Jul 25, 2024 at 3:45 PM Andi Kleen <ak@linux.intel.com> wrote:
>
> > @@ -214,13 +214,14 @@ static int get_build_id_32(struct freader *r, unsigned char *build_id, __u32 *si
> >
> >       /* subsequent freader_fetch() calls invalidate pointers, so remember locally */
> >       phnum = ehdr->e_phnum;
> > +     phoff = READ_ONCE(ehdr->e_phoff);
> >
> >       /* only supports phdr that fits in one page */
> >       if (phnum > (PAGE_SIZE - sizeof(Elf32_Ehdr)) / sizeof(Elf32_Phdr))
> >               return -EINVAL;
> >
> >       for (i = 0; i < phnum; ++i) {
> > -             phdr = freader_fetch(r, i * sizeof(Elf32_Phdr), sizeof(Elf32_Phdr));
> > +             phdr = freader_fetch(r, phoff + i * sizeof(Elf32_Phdr), sizeof(Elf32_Phdr));
>
> What happens if phoff is big enough that this computation wraps?
>

phoff is u32, phoff + i * sizeof(Elf32_Phdr) will be casted to u64 as
it's passed into freader_fetch (which expects u64), and so it will be
an offset slightly bigger than 4GB into the file. If that happens to
be a valid file offset, so be it, we'll fetch the page at that file
offset. If not, freader_fetch() will return NULL.
diff mbox series

Patch

diff --git a/lib/buildid.c b/lib/buildid.c
index 1442a2483a8b..ce48ffab4111 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -206,7 +206,7 @@  static int get_build_id_32(struct freader *r, unsigned char *build_id, __u32 *si
 {
 	const Elf32_Ehdr *ehdr;
 	const Elf32_Phdr *phdr;
-	__u32 phnum, i;
+	__u32 phnum, phoff, i;
 
 	ehdr = freader_fetch(r, 0, sizeof(Elf32_Ehdr));
 	if (!ehdr)
@@ -214,13 +214,14 @@  static int get_build_id_32(struct freader *r, unsigned char *build_id, __u32 *si
 
 	/* subsequent freader_fetch() calls invalidate pointers, so remember locally */
 	phnum = ehdr->e_phnum;
+	phoff = READ_ONCE(ehdr->e_phoff);
 
 	/* only supports phdr that fits in one page */
 	if (phnum > (PAGE_SIZE - sizeof(Elf32_Ehdr)) / sizeof(Elf32_Phdr))
 		return -EINVAL;
 
 	for (i = 0; i < phnum; ++i) {
-		phdr = freader_fetch(r, i * sizeof(Elf32_Phdr), sizeof(Elf32_Phdr));
+		phdr = freader_fetch(r, phoff + i * sizeof(Elf32_Phdr), sizeof(Elf32_Phdr));
 		if (!phdr)
 			return r->err;
 
@@ -237,6 +238,7 @@  static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si
 	const Elf64_Ehdr *ehdr;
 	const Elf64_Phdr *phdr;
 	__u32 phnum, i;
+	__u64 phoff;
 
 	ehdr = freader_fetch(r, 0, sizeof(Elf64_Ehdr));
 	if (!ehdr)
@@ -244,13 +246,14 @@  static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si
 
 	/* subsequent freader_fetch() calls invalidate pointers, so remember locally */
 	phnum = ehdr->e_phnum;
+	phoff = READ_ONCE(ehdr->e_phoff);
 
 	/* only supports phdr that fits in one page */
 	if (phnum > (PAGE_SIZE - sizeof(Elf64_Ehdr)) / sizeof(Elf64_Phdr))
 		return -EINVAL;
 
 	for (i = 0; i < phnum; ++i) {
-		phdr = freader_fetch(r, i * sizeof(Elf64_Phdr), sizeof(Elf64_Phdr));
+		phdr = freader_fetch(r, phoff + i * sizeof(Elf64_Phdr), sizeof(Elf64_Phdr));
 		if (!phdr)
 			return r->err;