| Message ID | 20240724225210.545423-3-andrii@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Harden and extend ELF build ID parsing logic | expand |
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 > >
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 > > > >
> @@ -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?
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 --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;
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(-)