Re: [edk2-devel] [PATCH v1 5/6] ArmVirtPkg: Kvmtool: Increase primary core stack size

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

On Thu, 18 May 2023 at 17:12, Sami Mujawar <sami.mujawar@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.mujawar@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.