[PATCH] MdePkg/BaseLib AARCH64: terminate stack frame list on stack switch

[PATCH] MdePkg/BaseLib AARCH64: terminate stack frame list on stack switch

[Ard Biesheuvel]

When switching to the DXE phase stack, set the frame pointer to zero so
that code walking the stack frame will not try to access stack frames\
belonging to the old stack.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdePkg/Library/BaseLib/AArch64/SwitchStack.S | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MdePkg/Library/BaseLib/AArch64/SwitchStack.S b/MdePkg/Library/BaseLib/AArch64/SwitchStack.S
index 2bce9c998f4f..c3ac8d7e4dfe 100644
--- a/MdePkg/Library/BaseLib/AArch64/SwitchStack.S
+++ b/MdePkg/Library/BaseLib/AArch64/SwitchStack.S
@@ -40,6 +40,7 @@ InternalSwitchStackAsm (
   );
 **/
 ASM_PFX(InternalSwitchStackAsm):
+    mov   x29, #0
     mov   x30, x0
     mov   sp, x3
     mov   x0, x1
-- 
2.7.4

Re: [PATCH] MdePkg/BaseLib AARCH64: terminate stack frame list on stack switch

[Leif Lindholm]

On Fri, Sep 09, 2016 at 08:21:26AM +0100, Ard Biesheuvel wrote:
> When switching to the DXE phase stack, set the frame pointer to zero so
> that code walking the stack frame will not try to access stack frames\

Trailing '\'.

> belonging to the old stack.

Do you mean that code will check for zero and stop processing, or that
it will be accessing rubbish instead of parsing a valid-looking frame?

Either is an improvement, but if it is the latter I would prefer it
more explicitly stated.

You can fix up on commit:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdePkg/Library/BaseLib/AArch64/SwitchStack.S | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MdePkg/Library/BaseLib/AArch64/SwitchStack.S b/MdePkg/Library/BaseLib/AArch64/SwitchStack.S
> index 2bce9c998f4f..c3ac8d7e4dfe 100644
> --- a/MdePkg/Library/BaseLib/AArch64/SwitchStack.S
> +++ b/MdePkg/Library/BaseLib/AArch64/SwitchStack.S
> @@ -40,6 +40,7 @@ InternalSwitchStackAsm (
>    );
>  **/
>  ASM_PFX(InternalSwitchStackAsm):
> +    mov   x29, #0
>      mov   x30, x0
>      mov   sp, x3
>      mov   x0, x1
> -- 
> 2.7.4
> 

Re: [PATCH] MdePkg/BaseLib AARCH64: terminate stack frame list on stack switch

[Ard Biesheuvel]

On 9 September 2016 at 12:18, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, Sep 09, 2016 at 08:21:26AM +0100, Ard Biesheuvel wrote:
>> When switching to the DXE phase stack, set the frame pointer to zero so
>> that code walking the stack frame will not try to access stack frames\
>
> Trailing '\'.
>
>> belonging to the old stack.
>
> Do you mean that code will check for zero and stop processing, or that
> it will be accessing rubbish instead of parsing a valid-looking frame?
>

I don't understand this question. If it is zero, it will stop
processing. If it is not zero, it will proceed, and potentially
traverse stack frames in memory that is now owned by someone else.

> Either is an improvement, but if it is the latter I would prefer it
> more explicitly stated.
>


> You can fix up on commit:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  MdePkg/Library/BaseLib/AArch64/SwitchStack.S | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/MdePkg/Library/BaseLib/AArch64/SwitchStack.S b/MdePkg/Library/BaseLib/AArch64/SwitchStack.S
>> index 2bce9c998f4f..c3ac8d7e4dfe 100644
>> --- a/MdePkg/Library/BaseLib/AArch64/SwitchStack.S
>> +++ b/MdePkg/Library/BaseLib/AArch64/SwitchStack.S
>> @@ -40,6 +40,7 @@ InternalSwitchStackAsm (
>>    );
>>  **/
>>  ASM_PFX(InternalSwitchStackAsm):
>> +    mov   x29, #0
>>      mov   x30, x0
>>      mov   sp, x3
>>      mov   x0, x1
>> --
>> 2.7.4
>>

Re: [PATCH] MdePkg/BaseLib AARCH64: terminate stack frame list on stack switch

[Leif Lindholm]

On Fri, Sep 09, 2016 at 01:03:59PM +0100, Ard Biesheuvel wrote:
> On 9 September 2016 at 12:18, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Fri, Sep 09, 2016 at 08:21:26AM +0100, Ard Biesheuvel wrote:
> >> When switching to the DXE phase stack, set the frame pointer to zero so
> >> that code walking the stack frame will not try to access stack frames\
> >
> > Trailing '\'.
> >
> >> belonging to the old stack.
> >
> > Do you mean that code will check for zero and stop processing, or that
> > it will be accessing rubbish instead of parsing a valid-looking frame?
> 
> I don't understand this question. If it is zero, it will stop
> processing. If it is not zero, it will proceed, and potentially
> traverse stack frames in memory that is now owned by someone else.

Which was exactly what I was asking :)
So no need to change (but drop the '\').

/
    Leif