Re: [PATCH 07/16] x86/shstk: Re-layout the stack block for shadow stacks

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 04/05/2020 15:24, Jan Beulich wrote:
> On 02.05.2020 00:58, Andrew Cooper wrote:
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -732,14 +732,14 @@ void load_system_tables(void)
>>  		.rsp2 = 0x8600111111111111ul,
>>  
>>  		/*
>> -		 * MCE, NMI and Double Fault handlers get their own stacks.
>> +		 * #DB, NMI, DF and #MCE handlers get their own stacks.
> Then also #DF and #MC?

Ok.

>
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -6002,25 +6002,18 @@ void memguard_unguard_range(void *p, unsigned long l)
>>  
>>  void memguard_guard_stack(void *p)
>>  {
>> -    /* IST_MAX IST pages + at least 1 guard page + primary stack. */
>> -    BUILD_BUG_ON((IST_MAX + 1) * PAGE_SIZE + PRIMARY_STACK_SIZE > STACK_SIZE);
>> +    map_pages_to_xen((unsigned long)p, virt_to_mfn(p), 1, _PAGE_NONE);
>>  
>> -    memguard_guard_range(p + IST_MAX * PAGE_SIZE,
>> -                         STACK_SIZE - PRIMARY_STACK_SIZE - IST_MAX * PAGE_SIZE);
>> +    p += 5 * PAGE_SIZE;
> The literal 5 here and ...
>
>> +    map_pages_to_xen((unsigned long)p, virt_to_mfn(p), 1, _PAGE_NONE);
>>  }
>>  
>>  void memguard_unguard_stack(void *p)
>>  {
>> -    memguard_unguard_range(p + IST_MAX * PAGE_SIZE,
>> -                           STACK_SIZE - PRIMARY_STACK_SIZE - IST_MAX * PAGE_SIZE);
>> -}
>> -
>> -bool memguard_is_stack_guard_page(unsigned long addr)
>> -{
>> -    addr &= STACK_SIZE - 1;
>> +    map_pages_to_xen((unsigned long)p, virt_to_mfn(p), 1, PAGE_HYPERVISOR_RW);
>>  
>> -    return addr >= IST_MAX * PAGE_SIZE &&
>> -           addr < STACK_SIZE - PRIMARY_STACK_SIZE;
>> +    p += 5 * PAGE_SIZE;
> ... here could do with macro-izing: IST_MAX + 1 would already be
> a little better, I guess.

The problem is that "IST_MAX + 1" is now less meaningful than a literal
5, because at least 5 obviously matches up with the comment describing
which page does what.

~Andrew

>
> Preferably with adjustments along these lines
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Jan