Skip to content

Commit

Permalink
BaseTools/GenFw: Correct offset when relocating an ADR
Browse files Browse the repository at this point in the history
When converting ELF to PE/COFF for the AArch64 target, we may encounter
an R_AARCH64_ADR_GOT_PAGE relocation that refers to an ADR instruction
instead of an ADRP instruction. This can happen 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
(which is built with -fpie) with stack protection enabled on GCC
compiled with "--enable-fix-cortex-a53-843419". In this case, the linker
may convert an ADRP instruction appearing at an offset of 0xff8 or 0xffc
modulo 4KiB into an ADR instruction, but will leave the original
R_AARCH64_ADR_GOT_PAGE relocation in place. (This is not a bug in the
linker, given that there is no other relocation type that it could
reasonably convert it into)

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, tianocore#424]

    # Load to check the stack canary
    30cc:	b0000020 	adrp	x0, 8000 <mErrorString+0x1bc>
    30d0:	f940d400 	ldr	x0, [x0, tianocore#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. Note that there is a corner case
where this may cause the conversion to fail: if the original GOT entry
is just within -/+ 1 MiB of the reference, but the actual variable it
refers to is not, the resulting offset cannot be represented by the
immediate offset field in a ADR instruction. Given that this issue only
affects PIE executables, which are rare and usually tiny, this is
unlikely to cause problems in practice.

Ref: https://edk2.groups.io/g/devel/topic/102202314

[ardb: expand commit log, add reference]

Signed-off-by: Jake Garver <jake@nvidia.com>
Reviewed-by: Rebecca Cran <rebecca@bsdio.com>
  • Loading branch information
jgarver authored and ardbiesheuvel committed Dec 21, 2023
1 parent 9f0061a commit b60bdc3
Showing 1 changed file with 21 additions and 1 deletion.
22 changes: 21 additions & 1 deletion BaseTools/Source/C/GenFw/Elf64Convert.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit b60bdc3

Please sign in to comment.