[PATCH] elf: Avoid pointer-arithmetic underflow in ldconfig

[PATCH] elf: Avoid pointer-arithmetic underflow in ldconfig

[Peter Edwards]

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 ptrdiff_t for loadoff, and cast the p_vaddr and p_offset
fields to uintptr_t.

Also, given negative values are possible, use PTRDIFF_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(-)

diff --git a/elf/readelflib.c b/elf/readelflib.c
index f5b8c80e38..edff57f183 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;
+	  ptrdiff_t loadoff = PTRDIFF_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 = (uintptr_t)segment->p_vaddr -
+		      (uintptr_t)segment->p_offset;
 		  break;
 		}
 	    }
-	  if (loadoff == (ElfW(Off)) -1)
+	  if (loadoff == PTRDIFF_MAX)
 	    {
 	      /* Very strange. */
 	      loadoff = 0;
-- 
2.34.1

Re: [PATCH] elf: Avoid pointer-arithmetic underflow in ldconfig

[Andreas Schwab]

On Sep 04 2023, Peter Edwards via Libc-alpha wrote:

> @@ -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 = (uintptr_t)segment->p_vaddr -
> +		      (uintptr_t)segment->p_offset;

Shouldn't that use a signed difference, then?  Since loadoff is now
ptrdiff_t, I would expect the use of ptrdiff_t for the casts.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] elf: Avoid pointer-arithmetic underflow in ldconfig

[Peter Edwards]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

On Mon, 4 Sept 2023 at 12:18, Andreas Schwab <schwab@suse.de> wrote:

>
> > +               loadoff = (uintptr_t)segment->p_vaddr -
> > +                   (uintptr_t)segment->p_offset;
>
> Shouldn't that use a signed difference, then?  Since loadoff is now
> ptrdiff_t, I would expect the use of ptrdiff_t for the casts.
>
Hi,
For the 32-bit code, we were previously relying on the behaviour of
wrap-around on the unsigned type, this just extends that reliance to a
pointer-sized integer on the 64 bit platform.
I guess intptr_t might be a better option than uintptr_t?  I would think
ptrdiff_t would really only be useful as the difference between two
pointers (and I guess if you want to be pedantic, might not always be the
same as the size of a pointer).

Re: [PATCH] elf: Avoid pointer-arithmetic underflow in ldconfig

[Andreas Schwab]

On Sep 04 2023, Peter Edwards wrote:

> On Mon, 4 Sept 2023 at 12:18, Andreas Schwab <schwab@suse.de> wrote:
>
>>
>> > +               loadoff = (uintptr_t)segment->p_vaddr -
>> > +                   (uintptr_t)segment->p_offset;
>>
>> Shouldn't that use a signed difference, then?  Since loadoff is now
>> ptrdiff_t, I would expect the use of ptrdiff_t for the casts.
>>
> Hi,
> For the 32-bit code, we were previously relying on the behaviour of
> wrap-around on the unsigned type, this just extends that reliance to a
> pointer-sized integer on the 64 bit platform.

By using a wider (unsigned) type you are changing the wrap-around
semantics.  Wrap-around only works as expected when the type matches the
value domain.

> I guess intptr_t might be a better option than uintptr_t?

I think a wide enough signed type would be better, since the result is
supposed to be signed.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] elf: Avoid pointer-arithmetic underflow in ldconfig

[Peter Edwards]

[-- Attachment #1: Type: text/plain, Size: 267 bytes --]

>
>
>
> > I guess intptr_t might be a better option than uintptr_t?
>
> I think a wide enough signed type would be better, since the result is
> supposed to be signed.
>
That's what I'm suggesting, right? intptr_t is signed, and big enough to
hold any pointer value.

Re: [PATCH] elf: Avoid pointer-arithmetic underflow in ldconfig

[Andreas Schwab]

On Sep 04 2023, Peter Edwards wrote:

>>
>>
>>
>> > I guess intptr_t might be a better option than uintptr_t?
>>
>> I think a wide enough signed type would be better, since the result is
>> supposed to be signed.
>>
> That's what I'm suggesting, right?

If you are suggesting to change loadoff to intptr_t, then it's ok.  But
we should not mix ptrdiff_t with intptr_t.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] elf: Avoid pointer-arithmetic underflow in ldconfig

[Peter Edwards]

[-- Attachment #1: Type: text/plain, Size: 212 bytes --]

On Mon, 4 Sept 2023 at 17:26, Andreas Schwab <schwab@suse.de> wrote:

>
> If you are suggesting to change loadoff to intptr_t, then it's ok.  But
> we should not mix ptrdiff_t with intptr_t.
>

 Ok - Will update