diff mbox series

[bpf-next,07/10] lib/buildid: harden build ID parsing logic some more

Message ID 20240709204245.3847811-8-andrii@kernel.org (mailing list archive)
State New
Headers show
Series Harden and extend ELF build ID parsing logic | expand

Commit Message

Andrii Nakryiko July 9, 2024, 8:42 p.m. UTC 
Harden build ID parsing logic some more, adding explicit READ_ONCE()
when fetching values that we then use to check correctness and various
note iteration invariants.

Suggested-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 lib/buildid.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

Comments

Andi Kleen July 10, 2024, 8:45 p.m. UTC | #1 
On Tue, Jul 09, 2024 at 01:42:42PM -0700, Andrii Nakryiko wrote:
> Harden build ID parsing logic some more, adding explicit READ_ONCE()
> when fetching values that we then use to check correctness and various
> note iteration invariants.

Just sprinkling READ_ONCE all over doesn't necessarily fix the code.
It is only needed for values that affect a loop or reference.

You have to fix stuff like this 

static inline int parse_build_id(const void *page_addr,
                                 unsigned char *build_id,
                                 __u32 *size,
                                 const void *note_start,
                                 Elf32_Word note_size)
{
        /* check for overflow */
        if (note_start < page_addr || note_start + note_size < note_start)
            ^^^^^^^^^^^^^^^^^^^^^^
                return -EINVAL;


which is C undefined (at least without -fwrapv-pointer) and can easily
be miscompiled if it isn't already.

I suspect the code will need more work, especially since you're
unwilling to consider any defense in depth measures.

-Andi
Andrii Nakryiko July 10, 2024, 8:53 p.m. UTC | #2 
On Wed, Jul 10, 2024 at 1:45 PM Andi Kleen <ak@linux.intel.com> wrote:
>
> On Tue, Jul 09, 2024 at 01:42:42PM -0700, Andrii Nakryiko wrote:
> > Harden build ID parsing logic some more, adding explicit READ_ONCE()
> > when fetching values that we then use to check correctness and various
> > note iteration invariants.
>
> Just sprinkling READ_ONCE all over doesn't necessarily fix the code.
> It is only needed for values that affect a loop or reference.

Agreed, besides `READ_ONCE(nhdr->n_type) == BUILD_ID` and
`READ_ONCE(phdr->p_type) == PT_NOTE`, which I added mostly just for
consistency, the rest should be indeed read once and then checked, no?
Do you see any other unnecessary READ_ONCE()s in this patch?

>
> You have to fix stuff like this
>
> static inline int parse_build_id(const void *page_addr,
>                                  unsigned char *build_id,
>                                  __u32 *size,
>                                  const void *note_start,
>                                  Elf32_Word note_size)
> {
>         /* check for overflow */
>         if (note_start < page_addr || note_start + note_size < note_start)
>             ^^^^^^^^^^^^^^^^^^^^^^

this has been switched to u64-based offsets in patch #1, did you take
a look at it?

>                 return -EINVAL;
>
>
> which is C undefined (at least without -fwrapv-pointer) and can easily
> be miscompiled if it isn't already.
>
> I suspect the code will need more work, especially since you're
> unwilling to consider any defense in depth measures.
>

Can you be a bit more specific about the remaining issues? I'm happy
to fix whatever can and should be fixed (after the changes I already
did in this patch set).

If by "defense in depth" you mean allowing this functionality only for
executable VMAs, then yes, I refuse to do that, as I already
explained.

> -Andi
>
diff mbox series

Patch

diff --git a/lib/buildid.c b/lib/buildid.c
index 419966d88cd5..7e36a32fbb90 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -158,7 +158,7 @@  static int parse_build_id(struct freader *r, unsigned char *build_id, __u32 *siz
 	const char note_name[] = "GNU";
 	const size_t note_name_sz = sizeof(note_name);
 	u64 build_id_off, new_offs, note_end = note_offs + note_size;
-	u32 build_id_sz;
+	u32 build_id_sz, name_sz, desc_sz;
 	const Elf32_Nhdr *nhdr;
 	const char *data;
 
@@ -171,14 +171,15 @@  static int parse_build_id(struct freader *r, unsigned char *build_id, __u32 *siz
 		if (!nhdr)
 			return r->err;
 
-		if (nhdr->n_type == BUILD_ID &&
-		    nhdr->n_namesz == note_name_sz &&
-		    !strcmp((char *)(nhdr + 1), note_name) &&
-		    nhdr->n_descsz > 0 &&
-		    nhdr->n_descsz <= BUILD_ID_SIZE_MAX) {
+		name_sz = READ_ONCE(nhdr->n_namesz);
+		desc_sz = READ_ONCE(nhdr->n_descsz);
+		if (READ_ONCE(nhdr->n_type) == BUILD_ID &&
+		    name_sz == note_name_sz &&
+		    !strncmp((char *)(nhdr + 1), note_name, note_name_sz) &&
+		    desc_sz > 0 && desc_sz <= BUILD_ID_SIZE_MAX) {
 
 			build_id_off = note_offs + sizeof(Elf32_Nhdr) + ALIGN(note_name_sz, 4);
-			build_id_sz = nhdr->n_descsz;
+			build_id_sz = desc_sz;
 
 			/* freader_fetch() will invalidate nhdr pointer */
 			data = freader_fetch(r, build_id_off, build_id_sz);
@@ -192,8 +193,7 @@  static int parse_build_id(struct freader *r, unsigned char *build_id, __u32 *siz
 			return 0;
 		}
 
-		new_offs = note_offs + sizeof(Elf32_Nhdr) +
-			   ALIGN(nhdr->n_namesz, 4) + ALIGN(nhdr->n_descsz, 4);
+		new_offs = note_offs + sizeof(Elf32_Nhdr) + ALIGN(name_sz, 4) + ALIGN(desc_sz, 4);
 		if (new_offs <= note_offs)  /* overflow */
 			break;
 		note_offs = new_offs;
@@ -214,7 +214,7 @@  static int get_build_id_32(struct freader *r, unsigned char *build_id, __u32 *si
 		return r->err;
 
 	/* subsequent freader_fetch() calls invalidate pointers, so remember locally */
-	phnum = ehdr->e_phnum;
+	phnum = READ_ONCE(ehdr->e_phnum);
 	phoff = READ_ONCE(ehdr->e_phoff);
 
 	/* set upper bound on amount of segments (phdrs) we iterate */
@@ -226,8 +226,9 @@  static int get_build_id_32(struct freader *r, unsigned char *build_id, __u32 *si
 		if (!phdr)
 			return r->err;
 
-		if (phdr->p_type == PT_NOTE &&
-		    !parse_build_id(r, build_id, size, phdr->p_offset, phdr->p_filesz))
+		if (READ_ONCE(phdr->p_type) == PT_NOTE &&
+		    !parse_build_id(r, build_id, size,
+				    READ_ONCE(phdr->p_offset), READ_ONCE(phdr->p_filesz)))
 			return 0;
 	}
 	return -EINVAL;
@@ -246,7 +247,7 @@  static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si
 		return r->err;
 
 	/* subsequent freader_fetch() calls invalidate pointers, so remember locally */
-	phnum = ehdr->e_phnum;
+	phnum = READ_ONCE(ehdr->e_phnum);
 	phoff = READ_ONCE(ehdr->e_phoff);
 
 	/* set upper bound on amount of segments (phdrs) we iterate */
@@ -258,8 +259,9 @@  static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si
 		if (!phdr)
 			return r->err;
 
-		if (phdr->p_type == PT_NOTE &&
-		    !parse_build_id(r, build_id, size, phdr->p_offset, phdr->p_filesz))
+		if (READ_ONCE(phdr->p_type) == PT_NOTE &&
+		    !parse_build_id(r, build_id, size,
+				    READ_ONCE(phdr->p_offset), READ_ONCE(phdr->p_filesz)))
 			return 0;
 	}