| Message ID | 20240205161808.1316432-1-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | arm: Remove unused ldr _dl_start_user | expand |
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 >
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 >>
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\
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\ >
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 --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\