1. Patchew
  2. Xen
  3. View series

[PATCH] x86/smpboot: Unconditionally call memguard_unguard_stack() in cpu_smpboot_free()

Andrew Cooper posted 1 patch 3 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20201005122325.17395-1-andrew.cooper3@citrix.com
xen/arch/x86/smpboot.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
[PATCH] x86/smpboot: Unconditionally call memguard_unguard_stack() in cpu_smpboot_free()
Posted by Andrew Cooper 3 years, 6 months ago 
For simplicity between various configuration, Xen always uses shadow stack
mappings (Read-only + Dirty) for the guard page, irrespective of whether
CET-SS is enabled.

memguard_guard_stack() writes shadow stack tokens with plain writes.  This is
necessary to configure the BSP shadow stack correctly, and cannot be
implemented with WRSS.

Therefore, unconditionally call memguard_unguard_stack() to return the
mappings to fully writeable, so a subsequent call to memguard_guard_stack()
will succeed.

Fixes: 91d26ed304f ("x86/shstk: Create shadow stacks")
Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

This can more easily be demonstrated with CPU hotplug than S3, and the absence
of bug reports goes to show how rarely hotplug is used.
---
 xen/arch/x86/smpboot.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 5708573c41..c193cc0fb8 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -971,16 +971,16 @@ static void cpu_smpboot_free(unsigned int cpu, bool remove)
     if ( IS_ENABLED(CONFIG_PV32) )
         FREE_XENHEAP_PAGE(per_cpu(compat_gdt, cpu));
 
+    if ( stack_base[cpu] )
+        memguard_unguard_stack(stack_base[cpu]);
+
     if ( remove )
     {
         FREE_XENHEAP_PAGE(per_cpu(gdt, cpu));
         FREE_XENHEAP_PAGE(idt_tables[cpu]);
 
         if ( stack_base[cpu] )
-        {
-            memguard_unguard_stack(stack_base[cpu]);
             FREE_XENHEAP_PAGES(stack_base[cpu], STACK_ORDER);
-        }
     }
 }
 
-- 
2.11.0
Re: [PATCH] x86/smpboot: Unconditionally call memguard_unguard_stack() in cpu_smpboot_free()
Posted by Marek Marczykowski-Górecki 3 years, 6 months ago 
On Mon, Oct 05, 2020 at 01:23:25PM +0100, Andrew Cooper wrote:
> For simplicity between various configuration, Xen always uses shadow stack
> mappings (Read-only + Dirty) for the guard page, irrespective of whether
> CET-SS is enabled.
> 
> memguard_guard_stack() writes shadow stack tokens with plain writes.  This is
> necessary to configure the BSP shadow stack correctly, and cannot be
> implemented with WRSS.
> 
> Therefore, unconditionally call memguard_unguard_stack() to return the
> mappings to fully writeable, so a subsequent call to memguard_guard_stack()
> will succeed.
> 
> Fixes: 91d26ed304f ("x86/shstk: Create shadow stacks")
> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> 
> This can more easily be demonstrated with CPU hotplug than S3, and the absence
> of bug reports goes to show how rarely hotplug is used.
> ---
>  xen/arch/x86/smpboot.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> index 5708573c41..c193cc0fb8 100644
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -971,16 +971,16 @@ static void cpu_smpboot_free(unsigned int cpu, bool remove)
>      if ( IS_ENABLED(CONFIG_PV32) )
>          FREE_XENHEAP_PAGE(per_cpu(compat_gdt, cpu));
>  
> +    if ( stack_base[cpu] )
> +        memguard_unguard_stack(stack_base[cpu]);
> +
>      if ( remove )
>      {
>          FREE_XENHEAP_PAGE(per_cpu(gdt, cpu));
>          FREE_XENHEAP_PAGE(idt_tables[cpu]);
>  
>          if ( stack_base[cpu] )
> -        {
> -            memguard_unguard_stack(stack_base[cpu]);
>              FREE_XENHEAP_PAGES(stack_base[cpu], STACK_ORDER);
> -        }
>      }
>  }
>  

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
Re: [PATCH] x86/smpboot: Unconditionally call memguard_unguard_stack() in cpu_smpboot_free()
Posted by Jan Beulich 3 years, 6 months ago 
On 05.10.2020 14:23, Andrew Cooper wrote:
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -971,16 +971,16 @@ static void cpu_smpboot_free(unsigned int cpu, bool remove)
>      if ( IS_ENABLED(CONFIG_PV32) )
>          FREE_XENHEAP_PAGE(per_cpu(compat_gdt, cpu));
>  
> +    if ( stack_base[cpu] )
> +        memguard_unguard_stack(stack_base[cpu]);
> +
>      if ( remove )
>      {
>          FREE_XENHEAP_PAGE(per_cpu(gdt, cpu));
>          FREE_XENHEAP_PAGE(idt_tables[cpu]);
>  
>          if ( stack_base[cpu] )
> -        {
> -            memguard_unguard_stack(stack_base[cpu]);
>              FREE_XENHEAP_PAGES(stack_base[cpu], STACK_ORDER);
> -        }
>      }
>  }

In my initial reply to Marek's report I did suggest putting the fix
in the alloc path in order to keep the pages "guarded" while the CPU
is parked, as the CPU during that period may still access at least
some of the stacks (e.g. when sending it an NMI IPI to enter a deeper
C state).

Otherwise, if the fix really was to remain here, the if() could now
also be dropped. And in this case I'd also suggest adding the new
piece of code a few lines earlier, so that all the
FREE_XENHEAP_PAGE() stay close together.

Jan
Re: [PATCH] x86/smpboot: Unconditionally call memguard_unguard_stack() in cpu_smpboot_free()
Posted by Andrew Cooper 3 years, 6 months ago 
On 13/10/2020 14:16, Jan Beulich wrote:
> On 05.10.2020 14:23, Andrew Cooper wrote:
>> --- a/xen/arch/x86/smpboot.c
>> +++ b/xen/arch/x86/smpboot.c
>> @@ -971,16 +971,16 @@ static void cpu_smpboot_free(unsigned int cpu, bool remove)
>>      if ( IS_ENABLED(CONFIG_PV32) )
>>          FREE_XENHEAP_PAGE(per_cpu(compat_gdt, cpu));
>>  
>> +    if ( stack_base[cpu] )
>> +        memguard_unguard_stack(stack_base[cpu]);
>> +
>>      if ( remove )
>>      {
>>          FREE_XENHEAP_PAGE(per_cpu(gdt, cpu));
>>          FREE_XENHEAP_PAGE(idt_tables[cpu]);
>>  
>>          if ( stack_base[cpu] )
>> -        {
>> -            memguard_unguard_stack(stack_base[cpu]);
>>              FREE_XENHEAP_PAGES(stack_base[cpu], STACK_ORDER);
>> -        }
>>      }
>>  }
> In my initial reply to Marek's report I did suggest putting the fix
> in the alloc path in order to keep the pages "guarded" while the CPU
> is parked

In which case you should have identified that bug explicitly.

Because I can't read your mind while fixing the other real bugs in your
suggestion.

~Andrew
Re: [PATCH] x86/smpboot: Unconditionally call memguard_unguard_stack() in cpu_smpboot_free()
Posted by Jan Beulich 3 years, 6 months ago 
On 13.10.2020 15:23, Andrew Cooper wrote:
> On 13/10/2020 14:16, Jan Beulich wrote:
>> On 05.10.2020 14:23, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/smpboot.c
>>> +++ b/xen/arch/x86/smpboot.c
>>> @@ -971,16 +971,16 @@ static void cpu_smpboot_free(unsigned int cpu, bool remove)
>>>      if ( IS_ENABLED(CONFIG_PV32) )
>>>          FREE_XENHEAP_PAGE(per_cpu(compat_gdt, cpu));
>>>  
>>> +    if ( stack_base[cpu] )
>>> +        memguard_unguard_stack(stack_base[cpu]);
>>> +
>>>      if ( remove )
>>>      {
>>>          FREE_XENHEAP_PAGE(per_cpu(gdt, cpu));
>>>          FREE_XENHEAP_PAGE(idt_tables[cpu]);
>>>  
>>>          if ( stack_base[cpu] )
>>> -        {
>>> -            memguard_unguard_stack(stack_base[cpu]);
>>>              FREE_XENHEAP_PAGES(stack_base[cpu], STACK_ORDER);
>>> -        }
>>>      }
>>>  }
>> In my initial reply to Marek's report I did suggest putting the fix
>> in the alloc path in order to keep the pages "guarded" while the CPU
>> is parked
> 
> In which case you should have identified that bug explicitly.
> 
> Because I can't read your mind while fixing the other real bugs in your
> suggestion.

I'm sorry for the brevity at that point - it was a Sunday, and I merely
thought I'd write down my observation after reading the report. And of
course I'm curious as to the other real bugs in my suggestion (when I
anyway said "something like").

Jan

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.