[v2] elf: Avoid pointer-arithmetic underflow in ldconfig
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Testing passed
|
| redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
| redhat-pt-bot/TryBot-32bit |
success
|
Build for i686
|
| linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_glibc_check--master-arm |
success
|
Testing passed
|
Commit Message
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(-)
Comments
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];
@@ -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;