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

["Jan Beulich" <JBeulich@suse.com>]

>>> 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.

> @@ -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?

> --- 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?

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.

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

Jan