From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@suse.com>
Subject: Re: [PATCH 8/8] xen/x86: Additional SMAP modes to work
 around buggy 32bit PV guests
Date: Thu, 25 Jun 2015 16:12:48 +0100
Message-ID: <558C36900200007800089BEE@mail.emea.novell.com>
References: <1435163500-10589-1-git-send-email-andrew.cooper3@citrix.com>
	<1435163500-10589-9-git-send-email-andrew.cooper3@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1435163500-10589-9-git-send-email-andrew.cooper3@citrix.com>
Content-Disposition: inline
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

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