Message ID | 20230904170332.398424-1-peadar@arista.com |
---|---|
State | New |
Headers | show |
Series | [v2] elf: Avoid pointer-arithmetic underflow in ldconfig | expand |
On 2023-09-04 10:03, Peter Edwards via Libc-alpha wrote: > + loadoff = (intptr_t)segment->p_vaddr - > + (intptr_t)segment->p_offset; This could suffer from intptr_t overflow, couldn't it? Signed integer overflow has undefined behavior. > Also, given negative values are possible, use INTPTR_MAX instead of -1 > as a better sentinel When scanning arbitrary data even INTPTR_MAX is problematic at least in theory. Let's redo things to not make assumptions about data values. Looking into the code a bit, it appears that elf/readelflib.c has a distressingly large number of pointer- and integer-overflow bugs with unusual or corrupt ELF data. And there's even a switch case with a missing 'break' at the end; ouch. Attached is a proposed patch to fix the bugs I saw, including the bug you noticed. I'd appreciate your looking over it since you're familiar with this code.
On Sep 04 2023, Paul Eggert wrote: > +/* A single bad_offset implementation is valid for both 32- and 64-bit code, > + so define it just once. */ > +#ifndef bad_offset_defined > +# define bad_offset_defined > + > +/* Check that OFFSET, ..., OFFSET + NITEMS * ITEMSIZE all fit within > + (or just at the end of) a file of length FILE_LENGTH. Also check > + that OFFSET is a multiple of ALIGNMENT. Return true (diagnosing > + with FILE_NAME) if the offsets are invalid. */ > +static bool > +bad_offset (ElfW(Xword) offset, ElfW(Xword) nitems, size_t itemsize, Please don't make it look like it depends on ElfW.
On Tue, 5 Sept 2023 at 00:51, Paul Eggert <eggert@cs.ucla.edu> wrote: > Attached is a proposed patch to fix the bugs I saw, including the bug > you noticed. I'd appreciate your looking over it since you're familiar > with this code. I am a novice here - I first saw this code the other day, but for what my opinion is worth, the patch looks fine to me, modulo the same issue Andreas has - if we're doing pointer arithmetic in the host environment, I think it's more appropriate to use offset types natural to the host environment, rather than the ElfXX_ types for the elf class we're looking at. The unused PT_INTERP and I assume accidental lack of a break was a nice catch.
On Sep 04 2023, Paul Eggert wrote: > + while (sizeof (ElfW(Nhdr)) + 4 < note_offset_lim - note_offset) while (note_offset_lim - note_offset > sizeof (ElfW(Nhdr)) + 4) > + 0 < stringbytes && dynamic_strings[stringbytes - 1]; stringbytes > 0 && dynamic_strings[stringbytes - 1];
diff --git a/elf/readelflib.c b/elf/readelflib.c index f5b8c80e38..efab08ce3c 100644 --- a/elf/readelflib.c +++ b/elf/readelflib.c @@ -203,7 +203,7 @@ done: { /* Find the file offset of the segment containing the dynamic string table. */ - ElfW(Off) loadoff = -1; + intptr_t loadoff = INTPTR_MAX; for (i = 0, segment = elf_pheader; i < elf_header->e_phnum; i++, segment++) { @@ -212,11 +212,15 @@ done: && (dyn_entry->d_un.d_val - segment->p_vaddr < segment->p_filesz)) { - loadoff = segment->p_vaddr - segment->p_offset; + /* Note loadoff may be negative - the ELF headers may not be + in a loadable segment, and the first loadable segment + may be at a p_offset > 0, but p_vaddr == 0 */ + loadoff = (intptr_t)segment->p_vaddr - + (intptr_t)segment->p_offset; break; } } - if (loadoff == (ElfW(Off)) -1) + if (loadoff == INTPTR_MAX) { /* Very strange. */ loadoff = 0;
For a 64-bit ldconfig, running on a 32-bit library, if the p_vaddr field of the segment containing the dynamic strings is less than it's p_offset, then using ElfW(Off) for the arithmetic leads to a truncated unsigned value for pointer arithmetic. Instead, use intptr_t for loadoff, and cast the p_vaddr and p_offset fields to same. Also, given negative values are possible, use INTPTR_MAX instead of -1 as a better sentinel to indicate the value is unset. Expected behaviour: 64-bit `ldconfig` runs silently, updating cache Observed behaviour: `ldconfig` reports ``` ldconfig: file <filename> is truncated ``` ... for any 32-bit ELF libs with dynamic strings in a segment with p_vaddr > p_offset Signed-off-by: Peter Edwards <peadar@arista.com> --- elf/readelflib.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)