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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.05.2020 19:48, Andrew Cooper wrote:
> 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.

If you don't like IST_MAX+1, can I at least talk you into introducing
a separate constant? This is the only way to connect together the
various places where it is used. We can't be sure that we're not going
to touch this code anymore, ever. And if we do, it'll help if related
places are easy to spot.

Jan