* [PATCH] elf: Avoid pointer-arithmetic underflow in ldconfig
@ 2023-09-04 10:54 Peter Edwards
2023-09-04 11:18 ` Andreas Schwab
0 siblings, 1 reply; 7+ messages in thread
From: Peter Edwards @ 2023-09-04 10:54 UTC (permalink / raw)
To: libc-alpha; +Cc: 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] elf: Avoid pointer-arithmetic underflow in ldconfig
2023-09-04 10:54 [PATCH] elf: Avoid pointer-arithmetic underflow in ldconfig Peter Edwards
@ 2023-09-04 11:18 ` Andreas Schwab
2023-09-04 12:32 ` Peter Edwards
0 siblings, 1 reply; 7+ messages in thread
From: Andreas Schwab @ 2023-09-04 11:18 UTC (permalink / raw)
To: Peter Edwards via Libc-alpha; +Cc: Peter Edwards
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."
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] elf: Avoid pointer-arithmetic underflow in ldconfig
2023-09-04 11:18 ` Andreas Schwab
@ 2023-09-04 12:32 ` Peter Edwards
2023-09-04 14:27 ` Andreas Schwab
0 siblings, 1 reply; 7+ messages in thread
From: Peter Edwards @ 2023-09-04 12:32 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Peter Edwards via Libc-alpha
[-- 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).
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] elf: Avoid pointer-arithmetic underflow in ldconfig
2023-09-04 12:32 ` Peter Edwards
@ 2023-09-04 14:27 ` Andreas Schwab
2023-09-04 15:34 ` Peter Edwards
0 siblings, 1 reply; 7+ messages in thread
From: Andreas Schwab @ 2023-09-04 14:27 UTC (permalink / raw)
To: Peter Edwards; +Cc: Peter Edwards via Libc-alpha
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."
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] elf: Avoid pointer-arithmetic underflow in ldconfig
2023-09-04 14:27 ` Andreas Schwab
@ 2023-09-04 15:34 ` Peter Edwards
2023-09-04 16:26 ` Andreas Schwab
0 siblings, 1 reply; 7+ messages in thread
From: Peter Edwards @ 2023-09-04 15:34 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Peter Edwards via Libc-alpha
[-- 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] elf: Avoid pointer-arithmetic underflow in ldconfig
2023-09-04 15:34 ` Peter Edwards
@ 2023-09-04 16:26 ` Andreas Schwab
2023-09-04 16:28 ` Peter Edwards
0 siblings, 1 reply; 7+ messages in thread
From: Andreas Schwab @ 2023-09-04 16:26 UTC (permalink / raw)
To: Peter Edwards; +Cc: Peter Edwards via Libc-alpha
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."
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] elf: Avoid pointer-arithmetic underflow in ldconfig
2023-09-04 16:26 ` Andreas Schwab
@ 2023-09-04 16:28 ` Peter Edwards
0 siblings, 0 replies; 7+ messages in thread
From: Peter Edwards @ 2023-09-04 16:28 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Peter Edwards via Libc-alpha
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-09-04 16:28 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-04 10:54 [PATCH] elf: Avoid pointer-arithmetic underflow in ldconfig Peter Edwards
2023-09-04 11:18 ` Andreas Schwab
2023-09-04 12:32 ` Peter Edwards
2023-09-04 14:27 ` Andreas Schwab
2023-09-04 15:34 ` Peter Edwards
2023-09-04 16:26 ` Andreas Schwab
2023-09-04 16:28 ` Peter Edwards
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).