diff mbox series

arm: Remove unused ldr _dl_start_user

Message ID 20240205161808.1316432-1-adhemerval.zanella@linaro.org
State Committed
Commit 1e25112dc0cb2515d27d8d178b1ecce778a9d37a
Headers
Series arm: Remove unused ldr _dl_start_user |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
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 warning Patch is already merged

Commit Message

Adhemerval Zanella Netto Feb. 5, 2024, 4:18 p.m. UTC 
  The commit 49d877a80b29d3002887b084eec6676d9f5fec18 (arm: Remove
_dl_skip_args usage) removed the _SKIP_ARGS literal, which was
previously loader to r4 on loader _start.  However, the cleanup did not
remove the following 'ldr r4, [sl, r4]' on _dl_start_user, used to check
to skip the arguments after ld self-relocations.

In my testing, the kernel initially set r4 to 0, which makes the
ldr instruction just read the _GLOBAL_OFFSET_TABLE_.  However, since r4
is a caller-saved register; a different runtime might not zero
initialize it and thus trigger an invalid memory access.

Checked on arm-linux-gnu.

Reported-by: Adrian Ratiu <adrian.ratiu@collabora.com>
---
 sysdeps/arm/dl-machine.h | 1 -
 1 file changed, 1 deletion(-)

Comments

Szabolcs Nagy Feb. 5, 2024, 5:06 p.m. UTC | #1 
The 02/05/2024 16:18, Adhemerval Zanella wrote:
> The commit 49d877a80b29d3002887b084eec6676d9f5fec18 (arm: Remove
> _dl_skip_args usage) removed the _SKIP_ARGS literal, which was
> previously loader to r4 on loader _start.  However, the cleanup did not
> remove the following 'ldr r4, [sl, r4]' on _dl_start_user, used to check
> to skip the arguments after ld self-relocations.
> 
> In my testing, the kernel initially set r4 to 0, which makes the
> ldr instruction just read the _GLOBAL_OFFSET_TABLE_.  However, since r4
> is a caller-saved register; a different runtime might not zero
       ^^^^^^

it's callee-saved (preserved by calls)

> initialize it and thus trigger an invalid memory access.
> 
> Checked on arm-linux-gnu.

patch looks good.

Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>

> 
> Reported-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
>  sysdeps/arm/dl-machine.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/sysdeps/arm/dl-machine.h b/sysdeps/arm/dl-machine.h
> index b857bbc868..dd1a0f6b6e 100644
> --- a/sysdeps/arm/dl-machine.h
> +++ b/sysdeps/arm/dl-machine.h
> @@ -139,7 +139,6 @@ _start:\n\
>  _dl_start_user:\n\
>  	adr	r6, .L_GET_GOT\n\
>  	add	sl, sl, r6\n\
> -	ldr	r4, [sl, r4]\n\
>  	@ save the entry point in another register\n\
>  	mov	r6, r0\n\
>  	@ get the original arg count\n\
> -- 
> 2.34.1
>
Adhemerval Zanella Netto Feb. 5, 2024, 5:08 p.m. UTC | #2 
On 05/02/24 14:06, Szabolcs Nagy wrote:
> The 02/05/2024 16:18, Adhemerval Zanella wrote:
>> The commit 49d877a80b29d3002887b084eec6676d9f5fec18 (arm: Remove
>> _dl_skip_args usage) removed the _SKIP_ARGS literal, which was
>> previously loader to r4 on loader _start.  However, the cleanup did not
>> remove the following 'ldr r4, [sl, r4]' on _dl_start_user, used to check
>> to skip the arguments after ld self-relocations.
>>
>> In my testing, the kernel initially set r4 to 0, which makes the
>> ldr instruction just read the _GLOBAL_OFFSET_TABLE_.  However, since r4
>> is a caller-saved register; a different runtime might not zero
>        ^^^^^^
> 
> it's callee-saved (preserved by calls)

Yeah, I meant calee-saved indeed.  I will fix the commit message.

> 
>> initialize it and thus trigger an invalid memory access.
>>
>> Checked on arm-linux-gnu.
> 
> patch looks good.
> 
> Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>
> 
>>
>> Reported-by: Adrian Ratiu <adrian.ratiu@collabora.com>
>> ---
>>  sysdeps/arm/dl-machine.h | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/sysdeps/arm/dl-machine.h b/sysdeps/arm/dl-machine.h
>> index b857bbc868..dd1a0f6b6e 100644
>> --- a/sysdeps/arm/dl-machine.h
>> +++ b/sysdeps/arm/dl-machine.h
>> @@ -139,7 +139,6 @@ _start:\n\
>>  _dl_start_user:\n\
>>  	adr	r6, .L_GET_GOT\n\
>>  	add	sl, sl, r6\n\
>> -	ldr	r4, [sl, r4]\n\
>>  	@ save the entry point in another register\n\
>>  	mov	r6, r0\n\
>>  	@ get the original arg count\n\
>> -- 
>> 2.34.1
>>
Sam James Feb. 5, 2024, 5:13 p.m. UTC | #3 
Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> The commit 49d877a80b29d3002887b084eec6676d9f5fec18 (arm: Remove
> _dl_skip_args usage) removed the _SKIP_ARGS literal, which was
> previously loader to r4 on loader _start.  However, the cleanup did not
> remove the following 'ldr r4, [sl, r4]' on _dl_start_user, used to check
> to skip the arguments after ld self-relocations.
>
> In my testing, the kernel initially set r4 to 0, which makes the
> ldr instruction just read the _GLOBAL_OFFSET_TABLE_.  However, since r4
> is a caller-saved register; a different runtime might not zero
> initialize it and thus trigger an invalid memory access.

Tag the bug?

Also, I feel like the title perhaps makes the change sound more cosmetic
than it is.

>
> Checked on arm-linux-gnu.
>
> Reported-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
>  sysdeps/arm/dl-machine.h | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/sysdeps/arm/dl-machine.h b/sysdeps/arm/dl-machine.h
> index b857bbc868..dd1a0f6b6e 100644
> --- a/sysdeps/arm/dl-machine.h
> +++ b/sysdeps/arm/dl-machine.h
> @@ -139,7 +139,6 @@ _start:\n\
>  _dl_start_user:\n\
>  	adr	r6, .L_GET_GOT\n\
>  	add	sl, sl, r6\n\
> -	ldr	r4, [sl, r4]\n\
>  	@ save the entry point in another register\n\
>  	mov	r6, r0\n\
>  	@ get the original arg count\n\
Adhemerval Zanella Netto Feb. 5, 2024, 6:09 p.m. UTC | #4 
On 05/02/24 14:13, Sam James wrote:
> 
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> 
>> The commit 49d877a80b29d3002887b084eec6676d9f5fec18 (arm: Remove
>> _dl_skip_args usage) removed the _SKIP_ARGS literal, which was
>> previously loader to r4 on loader _start.  However, the cleanup did not
>> remove the following 'ldr r4, [sl, r4]' on _dl_start_user, used to check
>> to skip the arguments after ld self-relocations.
>>
>> In my testing, the kernel initially set r4 to 0, which makes the
>> ldr instruction just read the _GLOBAL_OFFSET_TABLE_.  However, since r4
>> is a caller-saved register; a different runtime might not zero
>> initialize it and thus trigger an invalid memory access.
> 
> Tag the bug?
> 
> Also, I feel like the title perhaps makes the change sound more cosmetic
> than it is.

Right, I will change to 'arm: Remove wrong ldr _dl_start_user (BZ 31339)'

> 
>>
>> Checked on arm-linux-gnu.
>>
>> Reported-by: Adrian Ratiu <adrian.ratiu@collabora.com>
>> ---
>>  sysdeps/arm/dl-machine.h | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/sysdeps/arm/dl-machine.h b/sysdeps/arm/dl-machine.h
>> index b857bbc868..dd1a0f6b6e 100644
>> --- a/sysdeps/arm/dl-machine.h
>> +++ b/sysdeps/arm/dl-machine.h
>> @@ -139,7 +139,6 @@ _start:\n\
>>  _dl_start_user:\n\
>>  	adr	r6, .L_GET_GOT\n\
>>  	add	sl, sl, r6\n\
>> -	ldr	r4, [sl, r4]\n\
>>  	@ save the entry point in another register\n\
>>  	mov	r6, r0\n\
>>  	@ get the original arg count\n\
>
Sam James Feb. 5, 2024, 6:19 p.m. UTC | #5 
Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> writes:

> On 05/02/24 14:13, Sam James wrote:
>> 
>> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>> 
>>> The commit 49d877a80b29d3002887b084eec6676d9f5fec18 (arm: Remove
>>> _dl_skip_args usage) removed the _SKIP_ARGS literal, which was
>>> previously loader to r4 on loader _start.  However, the cleanup did not
>>> remove the following 'ldr r4, [sl, r4]' on _dl_start_user, used to check
>>> to skip the arguments after ld self-relocations.
>>>
>>> In my testing, the kernel initially set r4 to 0, which makes the
>>> ldr instruction just read the _GLOBAL_OFFSET_TABLE_.  However, since r4
>>> is a caller-saved register; a different runtime might not zero
>>> initialize it and thus trigger an invalid memory access.
>> 
>> Tag the bug?
>> 
>> Also, I feel like the title perhaps makes the change sound more cosmetic
>> than it is.
>
> Right, I will change to 'arm: Remove wrong ldr _dl_start_user (BZ 31339)'

wfm, thanks!

>
>> 
>>>
>>> Checked on arm-linux-gnu.
>>>
>>> Reported-by: Adrian Ratiu <adrian.ratiu@collabora.com>
>>> ---
>>>  sysdeps/arm/dl-machine.h | 1 -
>>>  1 file changed, 1 deletion(-)
>>>
>>> diff --git a/sysdeps/arm/dl-machine.h b/sysdeps/arm/dl-machine.h
>>> index b857bbc868..dd1a0f6b6e 100644
>>> --- a/sysdeps/arm/dl-machine.h
>>> +++ b/sysdeps/arm/dl-machine.h
>>> @@ -139,7 +139,6 @@ _start:\n\
>>>  _dl_start_user:\n\
>>>  	adr	r6, .L_GET_GOT\n\
>>>  	add	sl, sl, r6\n\
>>> -	ldr	r4, [sl, r4]\n\
>>>  	@ save the entry point in another register\n\
>>>  	mov	r6, r0\n\
>>>  	@ get the original arg count\n\
>>
diff mbox series

Patch

diff --git a/sysdeps/arm/dl-machine.h b/sysdeps/arm/dl-machine.h
index b857bbc868..dd1a0f6b6e 100644
--- a/sysdeps/arm/dl-machine.h
+++ b/sysdeps/arm/dl-machine.h
@@ -139,7 +139,6 @@  _start:\n\
 _dl_start_user:\n\
 	adr	r6, .L_GET_GOT\n\
 	add	sl, sl, r6\n\
-	ldr	r4, [sl, r4]\n\
 	@ save the entry point in another register\n\
 	mov	r6, r0\n\
 	@ get the original arg count\n\