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

Sami Mujawar posted 6 patches 1 year, 1 month ago
There is a newer version of this series
[edk2-devel] [PATCH v1 5/6] ArmVirtPkg: Kvmtool: Increase primary core stack size
Posted by Sami Mujawar 1 year, 1 month ago
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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 5/6] ArmVirtPkg: Kvmtool: Increase primary core stack size
Posted by Ard Biesheuvel 1 year, 1 month ago
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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 5/6] ArmVirtPkg: Kvmtool: Increase primary core stack size
Posted by Sami Mujawar 1 year, 1 month ago
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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 5/6] ArmVirtPkg: Kvmtool: Increase primary core stack size
Posted by Ard Biesheuvel 1 year, 1 month ago
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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 5/6] ArmVirtPkg: Kvmtool: Increase primary core stack size
Posted by Sami Mujawar 1 year, 1 month ago
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]
-=-=-=-=-=-=-=-=-=-=-=-