* [edk2-devel] [PATCH v2] BaseTools/GenFw: Correct offset when relocating an ADR
@ 2023-12-20 19:31 Jake Garver via groups.io
2023-12-20 21:26 ` Rebecca Cran
0 siblings, 1 reply; 3+ messages in thread
From: Jake Garver via groups.io @ 2023-12-20 19:31 UTC (permalink / raw)
To: devel
Cc: rebecca, gaoliming, bob.c.feng, yuwei.chen, ardb+tianocore,
pedro.falcato, Jake Garver
In the R_AARCH64_ADR_GOT_PAGE case on AARCH64, we may encounter an ADR
instead of an ADRP when the toolchain is working around Cortex-A53
erratum #843419. If that's the case, be sure to calculate the offset
appropriately.
This resolves an issue experienced when building a StandaloneMm image
with stack protection enabled on GCC compiled with
"--enable-fix-cortex-a53-843419". This scenario sometimes generates an
ADR with a R_AARCH64_ADR_GOT_PAGE relocation.
In this scenario, the following code is being generated by the
toolchain:
# Load to set the stack canary
2ffc: 10028020 adr x0, 8000 <mErrorString+0x1bc>
3008: f940d400 ldr x0, [x0, #424]
# Load to check the stack canary
30cc: b0000020 adrp x0, 8000 <mErrorString+0x1bc>
30d0: f940d400 ldr x0, [x0, #424]
GenFw rewrote that to:
# Load to set the stack canary
2ffc: 10000480 adr x0, 0x308c
3008: 912ec000 add x0, x0, #0xbb0
# Load to check the stack canary
30cc: f0000460 adrp x0, 0x92000
30d0: 912ec000 add x0, x0, #0xbb0
Note that we're now setting the stack canary from the wrong address,
resulting in an erroneous stack fault.
After this fix, the offset will be calculated correctly for an ADR and
the stack canary is set correctly.
Signed-off-by: Jake Garver <jake@nvidia.com>
---
Notes:
v2: Implement approach proposed by Ard Biesheuvel.
- title changed to: Correct offset when relocating an ADR
v1: Original title: Change opcode when converting ADR to ADRP
BaseTools/Source/C/GenFw/Elf64Convert.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c b/BaseTools/Source/C/GenFw/Elf64Convert.c
index 9911db65af..9d04fc612e 100644
--- a/BaseTools/Source/C/GenFw/Elf64Convert.c
+++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
@@ -1562,7 +1562,27 @@ WriteSections64 (
// subsequent LDR instruction (covered by a R_AARCH64_LD64_GOT_LO12_NC
// relocation) into an ADD instruction - this is handled above.
//
- Offset = (Sym->st_value - (Rel->r_offset & ~0xfff)) >> 12;
+ // In order to handle Cortex-A53 erratum #843419, the GCC toolchain
+ // may convert an ADRP instruction at the end of a page (0xffc
+ // offset) into an ADR instruction. If so, be sure to calculate the
+ // offset for an ADR instead of ADRP.
+ //
+ if ((*(UINT32 *)Targ & BIT31) == 0) {
+ //
+ // Calculate the offset for an ADR.
+ //
+ Offset = (Sym->st_value & ~0xfff) - Rel->r_offset;
+ if (Offset < -0x100000 || Offset > 0xfffff) {
+ Error (NULL, 0, 3000, "Invalid", "WriteSections64(): %s due to its size (> 1 MB), unable to relocate ADR.",
+ mInImageName);
+ break;
+ }
+ } else {
+ //
+ // Calculate the offset for an ADRP.
+ //
+ Offset = (Sym->st_value - (Rel->r_offset & ~0xfff)) >> 12;
+ }
*(UINT32 *)Targ &= 0x9000001f;
*(UINT32 *)Targ |= ((Offset & 0x1ffffc) << (5 - 2)) | ((Offset & 0x3) << 29);
--
2.34.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112769): https://edk2.groups.io/g/devel/message/112769
Mute This Topic: https://groups.io/mt/103287393/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [edk2-devel] [PATCH v2] BaseTools/GenFw: Correct offset when relocating an ADR
2023-12-20 19:31 [edk2-devel] [PATCH v2] BaseTools/GenFw: Correct offset when relocating an ADR Jake Garver via groups.io
@ 2023-12-20 21:26 ` Rebecca Cran
2023-12-21 10:13 ` Ard Biesheuvel
0 siblings, 1 reply; 3+ messages in thread
From: Rebecca Cran @ 2023-12-20 21:26 UTC (permalink / raw)
To: Jake Garver, devel
Cc: gaoliming, bob.c.feng, yuwei.chen, ardb+tianocore, pedro.falcato
Reviewed-by: Rebecca Cran <rebecca@bsdio.com>
On 12/20/2023 12:31 PM, Jake Garver wrote:
> In the R_AARCH64_ADR_GOT_PAGE case on AARCH64, we may encounter an ADR
> instead of an ADRP when the toolchain is working around Cortex-A53
> erratum #843419. If that's the case, be sure to calculate the offset
> appropriately.
>
> This resolves an issue experienced when building a StandaloneMm image
> with stack protection enabled on GCC compiled with
> "--enable-fix-cortex-a53-843419". This scenario sometimes generates an
> ADR with a R_AARCH64_ADR_GOT_PAGE relocation.
>
> In this scenario, the following code is being generated by the
> toolchain:
>
> # Load to set the stack canary
> 2ffc: 10028020 adr x0, 8000 <mErrorString+0x1bc>
> 3008: f940d400 ldr x0, [x0, #424]
>
> # Load to check the stack canary
> 30cc: b0000020 adrp x0, 8000 <mErrorString+0x1bc>
> 30d0: f940d400 ldr x0, [x0, #424]
>
> GenFw rewrote that to:
>
> # Load to set the stack canary
> 2ffc: 10000480 adr x0, 0x308c
> 3008: 912ec000 add x0, x0, #0xbb0
>
> # Load to check the stack canary
> 30cc: f0000460 adrp x0, 0x92000
> 30d0: 912ec000 add x0, x0, #0xbb0
>
> Note that we're now setting the stack canary from the wrong address,
> resulting in an erroneous stack fault.
>
> After this fix, the offset will be calculated correctly for an ADR and
> the stack canary is set correctly.
>
> Signed-off-by: Jake Garver <jake@nvidia.com>
> ---
>
> Notes:
> v2: Implement approach proposed by Ard Biesheuvel.
> - title changed to: Correct offset when relocating an ADR
> v1: Original title: Change opcode when converting ADR to ADRP
>
> BaseTools/Source/C/GenFw/Elf64Convert.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c b/BaseTools/Source/C/GenFw/Elf64Convert.c
> index 9911db65af..9d04fc612e 100644
> --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
> +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
> @@ -1562,7 +1562,27 @@ WriteSections64 (
> // subsequent LDR instruction (covered by a R_AARCH64_LD64_GOT_LO12_NC
> // relocation) into an ADD instruction - this is handled above.
> //
> - Offset = (Sym->st_value - (Rel->r_offset & ~0xfff)) >> 12;
> + // In order to handle Cortex-A53 erratum #843419, the GCC toolchain
> + // may convert an ADRP instruction at the end of a page (0xffc
> + // offset) into an ADR instruction. If so, be sure to calculate the
> + // offset for an ADR instead of ADRP.
> + //
> + if ((*(UINT32 *)Targ & BIT31) == 0) {
> + //
> + // Calculate the offset for an ADR.
> + //
> + Offset = (Sym->st_value & ~0xfff) - Rel->r_offset;
> + if (Offset < -0x100000 || Offset > 0xfffff) {
> + Error (NULL, 0, 3000, "Invalid", "WriteSections64(): %s due to its size (> 1 MB), unable to relocate ADR.",
> + mInImageName);
> + break;
> + }
> + } else {
> + //
> + // Calculate the offset for an ADRP.
> + //
> + Offset = (Sym->st_value - (Rel->r_offset & ~0xfff)) >> 12;
> + }
>
> *(UINT32 *)Targ &= 0x9000001f;
> *(UINT32 *)Targ |= ((Offset & 0x1ffffc) << (5 - 2)) | ((Offset & 0x3) << 29);
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112770): https://edk2.groups.io/g/devel/message/112770
Mute This Topic: https://groups.io/mt/103287393/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [edk2-devel] [PATCH v2] BaseTools/GenFw: Correct offset when relocating an ADR
2023-12-20 21:26 ` Rebecca Cran
@ 2023-12-21 10:13 ` Ard Biesheuvel
0 siblings, 0 replies; 3+ messages in thread
From: Ard Biesheuvel @ 2023-12-21 10:13 UTC (permalink / raw)
To: devel, rebecca
Cc: Jake Garver, gaoliming, bob.c.feng, yuwei.chen, ardb+tianocore,
pedro.falcato
On Wed, 20 Dec 2023 at 22:26, Rebecca Cran <rebecca@bsdio.com> wrote:
>
> Reviewed-by: Rebecca Cran <rebecca@bsdio.com>
>
Merged as #5183
Thanks all
>
> On 12/20/2023 12:31 PM, Jake Garver wrote:
> > In the R_AARCH64_ADR_GOT_PAGE case on AARCH64, we may encounter an ADR
> > instead of an ADRP when the toolchain is working around Cortex-A53
> > erratum #843419. If that's the case, be sure to calculate the offset
> > appropriately.
> >
> > This resolves an issue experienced when building a StandaloneMm image
> > with stack protection enabled on GCC compiled with
> > "--enable-fix-cortex-a53-843419". This scenario sometimes generates an
> > ADR with a R_AARCH64_ADR_GOT_PAGE relocation.
> >
> > In this scenario, the following code is being generated by the
> > toolchain:
> >
> > # Load to set the stack canary
> > 2ffc: 10028020 adr x0, 8000 <mErrorString+0x1bc>
> > 3008: f940d400 ldr x0, [x0, #424]
> >
> > # Load to check the stack canary
> > 30cc: b0000020 adrp x0, 8000 <mErrorString+0x1bc>
> > 30d0: f940d400 ldr x0, [x0, #424]
> >
> > GenFw rewrote that to:
> >
> > # Load to set the stack canary
> > 2ffc: 10000480 adr x0, 0x308c
> > 3008: 912ec000 add x0, x0, #0xbb0
> >
> > # Load to check the stack canary
> > 30cc: f0000460 adrp x0, 0x92000
> > 30d0: 912ec000 add x0, x0, #0xbb0
> >
> > Note that we're now setting the stack canary from the wrong address,
> > resulting in an erroneous stack fault.
> >
> > After this fix, the offset will be calculated correctly for an ADR and
> > the stack canary is set correctly.
> >
> > Signed-off-by: Jake Garver <jake@nvidia.com>
> > ---
> >
> > Notes:
> > v2: Implement approach proposed by Ard Biesheuvel.
> > - title changed to: Correct offset when relocating an ADR
> > v1: Original title: Change opcode when converting ADR to ADRP
> >
> > BaseTools/Source/C/GenFw/Elf64Convert.c | 22 +++++++++++++++++++++-
> > 1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c b/BaseTools/Source/C/GenFw/Elf64Convert.c
> > index 9911db65af..9d04fc612e 100644
> > --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
> > +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
> > @@ -1562,7 +1562,27 @@ WriteSections64 (
> > // subsequent LDR instruction (covered by a R_AARCH64_LD64_GOT_LO12_NC
> > // relocation) into an ADD instruction - this is handled above.
> > //
> > - Offset = (Sym->st_value - (Rel->r_offset & ~0xfff)) >> 12;
> > + // In order to handle Cortex-A53 erratum #843419, the GCC toolchain
> > + // may convert an ADRP instruction at the end of a page (0xffc
> > + // offset) into an ADR instruction. If so, be sure to calculate the
> > + // offset for an ADR instead of ADRP.
> > + //
> > + if ((*(UINT32 *)Targ & BIT31) == 0) {
> > + //
> > + // Calculate the offset for an ADR.
> > + //
> > + Offset = (Sym->st_value & ~0xfff) - Rel->r_offset;
> > + if (Offset < -0x100000 || Offset > 0xfffff) {
> > + Error (NULL, 0, 3000, "Invalid", "WriteSections64(): %s due to its size (> 1 MB), unable to relocate ADR.",
> > + mInImageName);
> > + break;
> > + }
> > + } else {
> > + //
> > + // Calculate the offset for an ADRP.
> > + //
> > + Offset = (Sym->st_value - (Rel->r_offset & ~0xfff)) >> 12;
> > + }
> >
> > *(UINT32 *)Targ &= 0x9000001f;
> > *(UINT32 *)Targ |= ((Offset & 0x1ffffc) << (5 - 2)) | ((Offset & 0x3) << 29);
>
>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112806): https://edk2.groups.io/g/devel/message/112806
Mute This Topic: https://groups.io/mt/103287393/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-12-21 10:14 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-20 19:31 [edk2-devel] [PATCH v2] BaseTools/GenFw: Correct offset when relocating an ADR Jake Garver via groups.io
2023-12-20 21:26 ` Rebecca Cran
2023-12-21 10:13 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox