Re: [PATCH 8/8] xen/x86: Additional SMAP modes to work around buggy 32bit PV guests

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

On 25/06/15 16:12, Jan Beulich wrote:
>>>> On 24.06.15 at 18:31, <andrew.cooper3@citrix.com> wrote:
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -1261,11 +1261,32 @@ Set the serial transmit buffer size.
>>  Flag to enable Supervisor Mode Execution Protection
>>  
>>  ### smap
>> -> `= <boolean>`
>> +> `= <boolean> | compat | fixup`
>>  
>>  > Default: `true`
> Knowing that it breaks certain guests, I think the default can't be
> true, but instead needs to become compat. People wanting more
> security at the expense of breaking guests can then pick either
> of true or fixup.

Ok.

> Jan
>


>
>> @@ -730,6 +731,12 @@ static int __init init_pv_cr4_masks(void)
>>      if ( cpu_has_fsgsbase )
>>          pv_cr4_mask &= ~X86_CR4_FSGSBASE;
>>  
>> +    /*
>> +     * 32bit PV guests may attempt to modify SMAP.
>> +     */
>> +    if ( cpu_has_smap )
>> +        compat_pv_cr4_mask &= ~X86_CR4_SMAP;
> Shouldn't this then be accompanied by exposing the CPUID flag to
> 32-bit guests?

I have become undecided on whether actually exposing SMAP is sensible or
not.  Not exposing it does make these fixes more simple.

>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -1437,21 +1437,38 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs)
>>          return 0;
>>      }
>>  
>> -    if ( guest_kernel_mode(v, regs) &&
>> -         !(regs->error_code & (PFEC_reserved_bit | PFEC_insn_fetch)) &&
>> -         (regs->error_code & PFEC_write_access) )
>> -    {
>> -        if ( VM_ASSIST(d, writable_pagetables) &&
>> -             /* Do not check if access-protection fault since the page may
>> -                legitimately be not present in shadow page tables */
>> -             (paging_mode_enabled(d) ||
>> -              (regs->error_code & PFEC_page_present)) &&
>> -             ptwr_do_page_fault(v, addr, regs) )
>> -            return EXCRET_fault_fixed;
>> +    if ( guest_kernel_mode(v, regs) )
>> +    {
>> +        if (!(regs->error_code & (PFEC_reserved_bit | PFEC_insn_fetch)) &&
>> +            (regs->error_code & PFEC_write_access) )
>> +        {
>> +            if ( VM_ASSIST(d, writable_pagetables) &&
>> +                 /* Do not check if access-protection fault since the page may
>> +                    legitimately be not present in shadow page tables */
>> +                 (paging_mode_enabled(d) ||
>> +                  (regs->error_code & PFEC_page_present)) &&
>> +                 ptwr_do_page_fault(v, addr, regs) )
>> +                return EXCRET_fault_fixed;
>>  
>> -        if ( is_hardware_domain(d) && (regs->error_code & PFEC_page_present) &&
>> -             mmio_ro_do_page_fault(v, addr, regs) )
>> +            if ( is_hardware_domain(d) && (regs->error_code & PFEC_page_present) &&
>> +                 mmio_ro_do_page_fault(v, addr, regs) )
>> +                return EXCRET_fault_fixed;
>> +        }
>> +
>> +        /*
>> +         * SMAP violation behind an unaware 32bit PV guest kernel? Set
>> +         * EFLAGS.AC behind its back and try again.
>> +         */
>> +        if ( (smap_mode == smap_mode_fixup) && is_pv_32bit_domain(d) &&
>> +             ((regs->error_code &
>> +               (PFEC_insn_fetch | PFEC_reserved_bit |
>> +                PFEC_user_mode | PFEC_page_present)) == PFEC_page_present) &&
>> +             ((v->arch.pv_vcpu.ctrlreg[4] & X86_CR4_SMAP) == 0) &&
>> +             ((regs->eflags & X86_EFLAGS_AC) == 0) )
> At least for single bit checks like these, could I talk you into using
> !(x & mask), as is being done elsewhere in the code you modify
> above?

Ok.

>
> Also here as well as in the code enforcing compat mode, I'm not
> sure it's correct to tie this to the current guest setting of CR4.SMAP.
> Instead I think you should latch any attempt by the guest to set
> the bit into a per-domain flag. After all it may itself have reasons to
> play strange things with the bit.

In both cases, if the guest sets CR4.SMAP itself, Xen needs to stop any
fixup behind the guests back.  If the guest subsequently clears SMAP for
a bit, it will expect to stop getting violations, but Xen still wants to
fix things up properly.

>
> Or alternatively the terminology in the comment and command line
> option documentation should be changed from "unaware" to "not
> currently having enabled".

This is better.  I will update the description.

~Andrew