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.
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.muja...@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)'
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105030): https://edk2.groups.io/g/devel/message/105030
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]
-=-=-=-=-=-=-=-=-=-=-=-
