Re: [edk2-devel] [PATCH] BaseTools/GenFw: Change opcode when converting ADR to ADRP

["Ard Biesheuvel" <ardb@kernel.org>]

On Wed, 20 Dec 2023 at 00:29, Jake Garver <jake@nvidia.com> wrote:
>
> Ard, Pedro,
>
> It's all connecting for me:
>
> I was able to recreate this problem with a crosstool-ng built toolchain.  That confirms it's not specific to Ubuntu and is indeed related to "--enable-fix-cortex-a53-843419".
> I also verified that this issue is specific to our StandaloneMm-based images because of the use of -fpie.
>

This does not surprise me tbh. GCC implements the stack canary load
using a special, opaque open coded sequence so that the value never
gets spilled to the stack inadvertently (which would defeat the
purpose of the canary check) and so it is not subjected to the same
kinds of optimizations that other loads are, wrt -mcmodel range and
linker relaxations.

I would argue that, even if stack protections are important, building
the StandaloneMm core with -fno-stack-protector would be a reasonable
workaround here, as the input it deals with is several API levels deep
and across an exception level boundary.

> Next up, I'll put together a patch to match Ard's below and test it.
>

Yes, please


>
> ________________________________
> From: Jake Garver <jake@nvidia.com>
> Sent: Wednesday, December 13, 2023 2:47 PM
> To: Pedro Falcato <pedro.falcato@gmail.com>; Ard Biesheuvel <ardb@kernel.org>
> Cc: devel@edk2.groups.io <devel@edk2.groups.io>; rebecca@bsdio.com <rebecca@bsdio.com>; gaoliming@byosoft.com.cn <gaoliming@byosoft.com.cn>; bob.c.feng@intel.com <bob.c.feng@intel.com>; yuwei.chen@intel.com <yuwei.chen@intel.com>
> Subject: Re: [edk2-devel] [PATCH] BaseTools/GenFw: Change opcode when converting ADR to ADRP
>
> Fantastic!
>
> When we hit this in GCC 12.3 toolchain, the ADR was indeed at a 0xffc page offset.  So, that connects.
>
> This also explains why the issue seemed to be specific to stack protection: Because it's comparing values right away.  If we hit this with other loads, we might not notice until later or at all.
>
> I have one more dot to connect:  When I built crosstool-ng, I was using "--enable-fix-cortex-a53-843419".  However, I'm guessing I wasn't lucky enough to end up with an ADRP at the end of a page.  I'll try to manufacture that situation as further confirmation.
>
> Thanks,
> Jake
> ________________________________
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Wednesday, December 13, 2023 1:01 PM
> To: Ard Biesheuvel <ardb@kernel.org>
> Cc: Jake Garver <jake@nvidia.com>; devel@edk2.groups.io <devel@edk2.groups.io>; rebecca@bsdio.com <rebecca@bsdio.com>; gaoliming@byosoft.com.cn <gaoliming@byosoft.com.cn>; bob.c.feng@intel.com <bob.c.feng@intel.com>; yuwei.chen@intel.com <yuwei.chen@intel.com>
> Subject: Re: [edk2-devel] [PATCH] BaseTools/GenFw: Change opcode when converting ADR to ADRP
>
> External email: Use caution opening links or attachments
>
>
> On Wed, Dec 13, 2023 at 5:31 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 13 Dec 2023 at 15:58, Jake Garver <jake@nvidia.com> wrote:
> > >
> > > Totally understand and agree, Ard.
> > >
> > > In the meantime, I've now experienced the issue with Ubuntu22's GCC 12.3.  Originally, we didn't see the issue on this toolchain, but a developer ran into when preparing a change.  Even more concerning, when I instrumented that change, it went away.  So, it seems to be very sensitive to the input, which will make it hard to reproduce.
> > >
> > > Specifically, like the Ubuntu20 10.5 toolchain, the Ubuntu 12.3 toolchain generated an R_AARCH64_ADR_GOT_PAGE relocation against an ADR instruction.  Further, it was when loading the value of __stack_chk_guard.
> > >
> > > I was again unable to reproduce this using a crosstool-ng build of GCC 12.3, even when matching the ./configure arguments.
> > >
> > > Since it's now reproducible in a toolchain we're actively using, I'll continue looking at it.  I'll let you know what I find.
> >
> > OK, mystery solved.
> >
> >     # Load to set the stack canary
> >     2ffc:       10000480        adr     x0, 0x308c
> >     3008:       912ec000        add     x0, x0, #0xbb0
> >
> > The location of the ADRP instruction is at the end of a 4k page
> > (0xffc), which could trigger erratum #843419 on Cortex-A53, and is
> > therefore converted into ADR.
>
> Ha! Great deduction! And because GCC builds don't turn on the a53 ADRP
> errata by default, the toolchains Jake built weren't catching this
> issue.
>
> --
> Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112748): https://edk2.groups.io/g/devel/message/112748
Mute This Topic: https://groups.io/mt/102202314/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-