On Thu, 18 May 2023 at 17:12, Sami Mujawar <sami.muja...@arm.com> wrote: > > Hi Ard, > > Thank you for the feedback and for the pointers. > > Please see my response inline marked [SAMI]. > > Regards, > > Sami Mujawar > > On 18/05/2023 12:01 pm, Ard Biesheuvel via groups.io wrote: > > On Thu, 18 May 2023 at 11:10, Sami Mujawar <sami.muja...@arm.com> wrote: > >> The patch "f07a9df9af60 ArmVirtPkg: Enable stack guard" > >> enabled stack overflow detection for ArmVirtPkg. Following > >> this patch, running UEFI shell command 'dmpstore' resulted > >> in a crash indicating a stack overflow. Invoking 'dmpstore' > >> results in recursive calls to CascadeProcessVariables () > >> which apparently consumes the available stack space and > >> overflows. > >> > >> Therefore, increase the primary core stack size. > >> > > Thanks for the fix. I imagine diagnosing this may not have been trivial. > > > > However, I don't think this is the right fix tbh. Normally, SEC and > > PEI run off this initial stack, and the DxeIpl PEIM is in charging of > > launching the DxeCore with a full sized stack, and remapping it > > non-executable as well. > > > > These PrePi platforms take some shortcuts and apparently, one of the > > consequences is that DXE and BDS run off the initial stack, which > > points into the firmware image IIRC. > > > > IOW, it would be better to explicitly allocate 128 KiB worth of > > bootservices data memory and let the DxeCore run off of that. > > [SAMI] If the stack size is passed in LoadDxeCoreFromFv() at > > https://github.com/tianocore/edk2/blob/master/ArmVirtPkg/PrePi/PrePi.c#L104, > the code at > > https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/Library/PrePiLib/PrePiLib.c#L158-L182 > > allocates the stack and switches it. I have set the stack size to 128KB > in the call to > > LoadDxeCoreFromFv (NULL, SIZE_128KB) and it fixes the issue. >
Fantastic, so all the code we need is already there. > However, the PrePiLib implementation lacks the code to remap the stack > as NonExecutable as > > done by the DxeIplPeim code at > > https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c#L42-L45). > > I have added a call to ArmSetMemoryRegionNoExec () in PrePiLib and it > works. However, this code would > > need to go in a separate Arch specific file. I am not sure what would be > required for other architectures but > > I can submit a patch that adds an arch hook function 'ArchSetStackNx > (UINTN StackBase, UINTN StackSize)' to > > set the stack as NonExecutable and provide an implementation for Arm. > Other architectures can similarly > > implement this function. > > Please let me know if this approach is ok. > Given the wider discussion we had the other day about tightening memory protections, I think it is important that we get this fixed but I don't think there is any urgency to it. I sent some patches a couple of months ago to map DxeCore code and data with tightened permissions as well, and I think we can revisit this in that context the next time around. So for now, just passing the stack size as you suggested above is sufficient IMO. Thanks, Ard. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105034): https://edk2.groups.io/g/devel/message/105034 Mute This Topic: https://groups.io/mt/98987538/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
