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.
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)'
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105015): https://edk2.groups.io/g/devel/message/105015
Mute This Topic: https://groups.io/mt/98987538/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-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. > 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)' > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105024): https://edk2.groups.io/g/devel/message/105024 Mute This Topic: https://groups.io/mt/98987538/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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)' >> > > > > -=-=-=-=-=-=-=-=-=-=-=- 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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.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. -=-=-=-=-=-=-=-=-=-=-=- 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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Ard, On Thu, May 18, 2023 at 08:17 AM, Ard Biesheuvel wrote: > > 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. [SAMI] Thanks, I will submit a v2 series with the changes. I also observed that a similar change would be needed for ArmPlatformPkg/PrePi at https://github.com/tianocore/edk2/blob/master/ArmPlatformPkg/PrePi/PrePi.c#L164 As of now we do not enable PcdCpuStackGuard in edk2-platforms\Platform\ARM\VExpressPkg\ArmVExpress.dsc.inc But if this is done the same stack overflow issue is seen on the FVP model. Considering that, should I send out a patch for ArmPlatformPkg/PrePi as well? Also, I think it be good to enable the stack guard check in edk2-platforms\Platform\ARM\VExpressPkg\ArmVExpress.dsc.inc. Please let me know your thoughts about this. [/SAMI] Regards, Sami Mujawar -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105038): https://edk2.groups.io/g/devel/message/105038 Mute This Topic: https://groups.io/mt/98987538/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2025 Red Hat, Inc.