[PATCH v2 0/2] x86/HVM: Properly handle SMAP check in certain cases

[PATCH v2 0/2] x86/HVM: Properly handle SMAP check in certain cases

[Feng Wu]

This patch set fixs a issue found by Sander Eikelenboom. Here is the log
when this issue occurs:

(d2)  Booting from Hard Disk...
(d2)  Booting from 0000:7c00
(XEN) irq.c:380: Dom1 callback via changed to Direct Vector 0xf3
(XEN) irq.c:380: Dom2 callback via changed to Direct Vector 0xf3
(XEN) Segment register inaccessible for d1v0
(XEN) (If you see this outside of debugging activity, please report to xen-devel@lists.xenproject.org)

And here is the Xen call trace:
(XEN) [<ffff82d0801dc9c5>] vmx_get_segment_register+0x4d/0x422
(XEN) [<ffff82d0801f4415>] guest_walk_tables_3_levels+0x189/0x520
(XEN) [<ffff82d0802204a8>] hap_p2m_ga_to_gfn_3_levels+0x158/0x2c2
(XEN) [<ffff82d08022062e>] hap_gva_to_gfn_3_levels+0x1c/0x1e
(XEN) [<ffff82d0801ec215>] paging_gva_to_gfn+0xb8/0xce
(XEN) [<ffff82d0801ba88d>] __hvm_copy+0x87/0x354
(XEN) [<ffff82d0801bac7c>] hvm_copy_to_guest_virt_nofault+0x1e/0x20
(XEN) [<ffff82d0801bace5>] copy_to_user_hvm+0x67/0x87
(XEN) [<ffff82d08016237c>] update_runstate_area+0x98/0xfb
(XEN) [<ffff82d0801623f0>] _update_runstate_area+0x11/0x39
(XEN) [<ffff82d0801634db>] context_switch+0x10c3/0x10fa
(XEN) [<ffff82d080126a19>] schedule+0x5a8/0x5da
(XEN) [<ffff82d0801297f9>] __do_softirq+0x81/0x8c
(XEN) [<ffff82d080129852>] do_softirq+0x13/0x15
(XEN) [<ffff82d08015f70a>] idle_loop+0x67/0x77

We need get guest's SS register via hvm_get_segment_register()
to do the SMAP checking, however, in these two cases, we cannot
do it that way since it is between setting 'current' and reloading
the VMCS context for it. As an alternative, here we treat these
accesses as implicit supervisor mode access, hence SMAP checking is
always need.
 
Feng Wu (2):
  x86/hvm: Always do SMAP check when updating runstate_guest(v)
  x86/hvm: Always do SMAP check when updating secondary system time for
    guest

 xen/arch/x86/domain.c        | 15 ++++++++++++---
 xen/arch/x86/mm/guest_walk.c | 41 ++++++++++++++++++++++++++++-------------
 xen/arch/x86/time.c          |  9 ++++++++-
 xen/include/asm-x86/domain.h | 17 +++++++++++++++--
 4 files changed, 63 insertions(+), 19 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/2] x86/hvm: Always do SMAP check when updating runstate_guest(v)

[Feng Wu]

In the current implementation, we honor the guest's CPL and AC
to determain whether do the SMAP check or not for runstate_guest(v).
However, this doesn't work. The VMCS feild is invalid when we try
to get geust's SS by hvm_get_segment_register(), since the
right VMCS has not beed loaded for the current VCPU.

In this patch, we always do the SMAP check when updating
runstate_guest(v) for the guest when SMAP is enabled by it.

Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Feng Wu <feng.wu@intel.com>
---
 xen/arch/x86/domain.c        | 15 ++++++++++++---
 xen/arch/x86/mm/guest_walk.c | 41 ++++++++++++++++++++++++++++-------------
 xen/include/asm-x86/domain.h | 15 ++++++++++++++-
 3 files changed, 54 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index e896210..b0c8810 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1349,22 +1349,31 @@ static void paravirt_ctxt_switch_to(struct vcpu *v)
 }
 
 /* Update per-VCPU guest runstate shared memory area (if registered). */
-bool_t update_runstate_area(const struct vcpu *v)
+bool_t update_runstate_area(struct vcpu *v)
 {
+    bool_t rc;
+
     if ( guest_handle_is_null(runstate_guest(v)) )
         return 1;
 
+    v->arch.smap_check_policy = SMAP_CHECK_ENABLED;
+
     if ( has_32bit_shinfo(v->domain) )
     {
         struct compat_vcpu_runstate_info info;
 
         XLAT_vcpu_runstate_info(&info, &v->runstate);
         __copy_to_guest(v->runstate_guest.compat, &info, 1);
-        return 1;
+        rc = 1;
+        goto out;
     }
 
-    return __copy_to_guest(runstate_guest(v), &v->runstate, 1) !=
+    rc = __copy_to_guest(runstate_guest(v), &v->runstate, 1) !=
            sizeof(v->runstate);
+
+out:
+    v->arch.smap_check_policy = SMAP_CHECK_HONOR_CPL_AC;
+    return rc;
 }
 
 static void _update_runstate_area(struct vcpu *v)
diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
index bb38fda..1afa7fd 100644
--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -164,25 +164,40 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
         struct segment_register seg;
         const struct cpu_user_regs *regs = guest_cpu_user_regs();
 
-        hvm_get_segment_register(v, x86_seg_ss, &seg);
-
         /* SMEP: kernel-mode instruction fetches from user-mode mappings
          * should fault.  Unlike NX or invalid bits, we're looking for _all_
          * entries in the walk to have _PAGE_USER set, so we need to do the
          * whole walk as if it were a user-mode one and then invert the answer. */
         smep =  hvm_smep_enabled(v) && (pfec & PFEC_insn_fetch);
 
-        /*
-         * SMAP: kernel-mode data accesses from user-mode mappings should fault
-         * A fault is considered as a SMAP violation if the following
-         * conditions come true:
-         *   - X86_CR4_SMAP is set in CR4
-         *   - A user page is accessed
-         *   - CPL = 3 or X86_EFLAGS_AC is clear
-         *   - Page fault in kernel mode
-         */
-        smap = hvm_smap_enabled(v) &&
-               ((seg.attr.fields.dpl == 3) || !(regs->eflags & X86_EFLAGS_AC));
+        switch ( v->arch.smap_check_policy )
+        {
+        case SMAP_CHECK_HONOR_CPL_AC:
+            hvm_get_segment_register(v, x86_seg_ss, &seg);
+
+            /*
+             * SMAP: kernel-mode data accesses from user-mode mappings
+             * should fault.
+             * A fault is considered as a SMAP violation if the following
+             * conditions come true:
+             *   - X86_CR4_SMAP is set in CR4
+             *   - A user page is accessed
+             *   - CPL = 3 or X86_EFLAGS_AC is clear
+             *   - Page fault in kernel mode
+             */
+            smap = hvm_smap_enabled(v) &&
+                   ((seg.attr.fields.dpl == 3) ||
+                   !(regs->eflags & X86_EFLAGS_AC));
+            break;
+        case SMAP_CHECK_ENABLED:
+            smap = hvm_smap_enabled(v);
+            break;
+        case SMAP_CHECK_DISABLED:
+            break;
+        default:
+            printk(XENLOG_INFO "Invalid SMAP check type!\n");
+            break;
+        }
     }
 
     if ( smep || smap )
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index abf55fb..d7cac4f 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -446,13 +446,26 @@ struct arch_vcpu
 
     /* A secondary copy of the vcpu time info. */
     XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
+
+    /*
+     * The SMAP check policy when updating runstate_guest(v) and the
+     * secondary system time.
+     *     SMAP_CHECK_HONOR_CPL_AC - honor the guest's CPL and AC
+     *     SMAP_CHECK_ENABLED      - enable the check
+     *     SMAP_CHECK_DISABLED     - disable the check
+     */
+    uint8_t smap_check_policy;
 } __cacheline_aligned;
 
+#define SMAP_CHECK_HONOR_CPL_AC  0
+#define SMAP_CHECK_ENABLED       1
+#define SMAP_CHECK_DISABLED      2
+
 /* Shorthands to improve code legibility. */
 #define hvm_vmx         hvm_vcpu.u.vmx
 #define hvm_svm         hvm_vcpu.u.svm
 
-bool_t update_runstate_area(const struct vcpu *);
+bool_t update_runstate_area(struct vcpu *);
 bool_t update_secondary_system_time(const struct vcpu *,
                                     struct vcpu_time_info *);
 
-- 
1.8.3.1

[PATCH v2 2/2] x86/hvm: Always do SMAP check when updating secondary system time for guest

[Feng Wu]

In this patch, we always do the SMAP check when updating secondary
system time for the guest when SMAP is enabled by it.

Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Feng Wu <feng.wu@intel.com>
---
 xen/arch/x86/time.c          | 9 ++++++++-
 xen/include/asm-x86/domain.h | 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index a4e1656..797f0cf 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -821,7 +821,7 @@ static void __update_vcpu_system_time(struct vcpu *v, int force)
         v->arch.pv_vcpu.pending_system_time = _u;
 }
 
-bool_t update_secondary_system_time(const struct vcpu *v,
+bool_t update_secondary_system_time(struct vcpu *v,
                                     struct vcpu_time_info *u)
 {
     XEN_GUEST_HANDLE(vcpu_time_info_t) user_u = v->arch.time_info_guest;
@@ -829,9 +829,14 @@ bool_t update_secondary_system_time(const struct vcpu *v,
     if ( guest_handle_is_null(user_u) )
         return 1;
 
+    v->arch.smap_check_policy = SMAP_CHECK_ENABLED;
+
     /* 1. Update userspace version. */
     if ( __copy_field_to_guest(user_u, u, version) == sizeof(u->version) )
+    {
+        v->arch.smap_check_policy = SMAP_CHECK_HONOR_CPL_AC;
         return 0;
+    }
     wmb();
     /* 2. Update all other userspace fields. */
     __copy_to_guest(user_u, u, 1);
@@ -840,6 +845,8 @@ bool_t update_secondary_system_time(const struct vcpu *v,
     u->version = version_update_end(u->version);
     __copy_field_to_guest(user_u, u, version);
 
+    v->arch.smap_check_policy = SMAP_CHECK_HONOR_CPL_AC;
+
     return 1;
 }
 
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index d7cac4f..9440351 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -466,7 +466,7 @@ struct arch_vcpu
 #define hvm_svm         hvm_vcpu.u.svm
 
 bool_t update_runstate_area(struct vcpu *);
-bool_t update_secondary_system_time(const struct vcpu *,
+bool_t update_secondary_system_time(struct vcpu *,
                                     struct vcpu_time_info *);
 
 void vcpu_show_execution_state(struct vcpu *);
-- 
1.8.3.1

Re: [PATCH v2 0/2] x86/HVM: Properly handle SMAP check in certain cases

[Wu, Feng]

Sorry, forget to add the change history. Adding it here:

V2: Remove ' VCPUOP_enable_smap_check_vcpu_time_memory_area' hypercall,
hence always do the SMAP checking for the secondary system time.

> -----Original Message-----
> From: Wu, Feng
> Sent: Monday, January 08, 2001 8:11 AM
> To: xen-devel@lists.xen.org
> Cc: keir@xen.org; jbeulich@suse.com; tim@xen.org; linux@eikelenboom.it;
> Wu, Feng
> Subject: [PATCH v2 0/2] x86/HVM: Properly handle SMAP check in certain cases
> 
> This patch set fixs a issue found by Sander Eikelenboom. Here is the log
> when this issue occurs:
> 
> (d2)  Booting from Hard Disk...
> (d2)  Booting from 0000:7c00
> (XEN) irq.c:380: Dom1 callback via changed to Direct Vector 0xf3
> (XEN) irq.c:380: Dom2 callback via changed to Direct Vector 0xf3
> (XEN) Segment register inaccessible for d1v0
> (XEN) (If you see this outside of debugging activity, please report to
> xen-devel@lists.xenproject.org)
> 
> And here is the Xen call trace:
> (XEN) [<ffff82d0801dc9c5>] vmx_get_segment_register+0x4d/0x422
> (XEN) [<ffff82d0801f4415>] guest_walk_tables_3_levels+0x189/0x520
> (XEN) [<ffff82d0802204a8>] hap_p2m_ga_to_gfn_3_levels+0x158/0x2c2
> (XEN) [<ffff82d08022062e>] hap_gva_to_gfn_3_levels+0x1c/0x1e
> (XEN) [<ffff82d0801ec215>] paging_gva_to_gfn+0xb8/0xce
> (XEN) [<ffff82d0801ba88d>] __hvm_copy+0x87/0x354
> (XEN) [<ffff82d0801bac7c>] hvm_copy_to_guest_virt_nofault+0x1e/0x20
> (XEN) [<ffff82d0801bace5>] copy_to_user_hvm+0x67/0x87
> (XEN) [<ffff82d08016237c>] update_runstate_area+0x98/0xfb
> (XEN) [<ffff82d0801623f0>] _update_runstate_area+0x11/0x39
> (XEN) [<ffff82d0801634db>] context_switch+0x10c3/0x10fa
> (XEN) [<ffff82d080126a19>] schedule+0x5a8/0x5da
> (XEN) [<ffff82d0801297f9>] __do_softirq+0x81/0x8c
> (XEN) [<ffff82d080129852>] do_softirq+0x13/0x15
> (XEN) [<ffff82d08015f70a>] idle_loop+0x67/0x77
> 
> We need get guest's SS register via hvm_get_segment_register()
> to do the SMAP checking, however, in these two cases, we cannot
> do it that way since it is between setting 'current' and reloading
> the VMCS context for it. As an alternative, here we treat these
> accesses as implicit supervisor mode access, hence SMAP checking is
> always need.
> 
> Feng Wu (2):
>   x86/hvm: Always do SMAP check when updating runstate_guest(v)
>   x86/hvm: Always do SMAP check when updating secondary system time for
>     guest
> 
>  xen/arch/x86/domain.c        | 15 ++++++++++++---
>  xen/arch/x86/mm/guest_walk.c | 41
> ++++++++++++++++++++++++++++-------------
>  xen/arch/x86/time.c          |  9 ++++++++-
>  xen/include/asm-x86/domain.h | 17 +++++++++++++++--
>  4 files changed, 63 insertions(+), 19 deletions(-)
> 
> --
> 1.8.3.1

Re: [PATCH v2 1/2] x86/hvm: Always do SMAP check when updating runstate_guest(v)

[Jan Beulich]

>>> On 08.01.01 at 01:10, <feng.wu@intel.com> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1349,22 +1349,31 @@ static void paravirt_ctxt_switch_to(struct vcpu *v)
>  }
>  
>  /* Update per-VCPU guest runstate shared memory area (if registered). */
> -bool_t update_runstate_area(const struct vcpu *v)
> +bool_t update_runstate_area(struct vcpu *v)
>  {
> +    bool_t rc;
> +
>      if ( guest_handle_is_null(runstate_guest(v)) )
>          return 1;
>  
> +    v->arch.smap_check_policy = SMAP_CHECK_ENABLED;
> +
>      if ( has_32bit_shinfo(v->domain) )
>      {
>          struct compat_vcpu_runstate_info info;
>  
>          XLAT_vcpu_runstate_info(&info, &v->runstate);
>          __copy_to_guest(v->runstate_guest.compat, &info, 1);
> -        return 1;
> +        rc = 1;
> +        goto out;
>      }
>  
> -    return __copy_to_guest(runstate_guest(v), &v->runstate, 1) !=
> +    rc = __copy_to_guest(runstate_guest(v), &v->runstate, 1) !=
>             sizeof(v->runstate);
> +
> +out:

Labels should be indented by at least one space. But even better
would be to handle this with "else" instead of "goto".

> +    v->arch.smap_check_policy = SMAP_CHECK_HONOR_CPL_AC;

Please save the old value and restore it here rather than blindly
enforcing "honor" mode.

> +        switch ( v->arch.smap_check_policy )
> +        {
> +        case SMAP_CHECK_HONOR_CPL_AC:
> +            hvm_get_segment_register(v, x86_seg_ss, &seg);
> +
> +            /*
> +             * SMAP: kernel-mode data accesses from user-mode mappings
> +             * should fault.
> +             * A fault is considered as a SMAP violation if the following
> +             * conditions come true:
> +             *   - X86_CR4_SMAP is set in CR4
> +             *   - A user page is accessed
> +             *   - CPL = 3 or X86_EFLAGS_AC is clear
> +             *   - Page fault in kernel mode
> +             */
> +            smap = hvm_smap_enabled(v) &&
> +                   ((seg.attr.fields.dpl == 3) ||
> +                   !(regs->eflags & X86_EFLAGS_AC));
> +            break;
> +        case SMAP_CHECK_ENABLED:
> +            smap = hvm_smap_enabled(v);
> +            break;
> +        case SMAP_CHECK_DISABLED:
> +            break;
> +        default:
> +            printk(XENLOG_INFO "Invalid SMAP check type!\n");

Isn't this more a BUG() or ASSERT(0), or perhaps - with the
SMAP_CHECK_DISABLED case dropped, BUG_ON(... !=
SMAP_CHECK_DISABLED) or ASSERT(... ==
SMAP_CHECK_DISABLED)?

> +    /*
> +     * The SMAP check policy when updating runstate_guest(v) and the
> +     * secondary system time.
> +     *     SMAP_CHECK_HONOR_CPL_AC - honor the guest's CPL and AC
> +     *     SMAP_CHECK_ENABLED      - enable the check
> +     *     SMAP_CHECK_DISABLED     - disable the check
> +     */
> +    uint8_t smap_check_policy;
>  } __cacheline_aligned;
>  
> +#define SMAP_CHECK_HONOR_CPL_AC  0
> +#define SMAP_CHECK_ENABLED       1
> +#define SMAP_CHECK_DISABLED      2

I'd prefer this to be an enum.

Jan