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

["Sami Mujawar" <sami.mujawar@arm.com>]

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.

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.

[/SAMI]

>
>
>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>> ---
>>   ArmVirtPkg/ArmVirtKvmTool.dsc | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
>> index 4541d03d23e0d98915b3d3ada688c48d979b75d2..664a624fd2a30bb466a3df2103482e3e6c1f303a 100644
>> --- a/ArmVirtPkg/ArmVirtKvmTool.dsc
>> +++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
>> @@ -126,7 +126,7 @@ [PcdsFixedAtBuild.common]
>>     gArmTokenSpaceGuid.PcdVFPEnabled|1
>>   !endif
>>
>> -  gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x4000
>> +  gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x8000
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>>
>> --
>> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>>
>
> 
>
>