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

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

The root cause of this issue is that the we try to get guest's SS
register via hvm_get_segment_register() between setting 'current'
and reloading the VMCS context for it.

Feng Wu (2):
  x86/hvm: Always do SMAP check when updating runstate_guest(v)
  x86/hvm: honor guest's option when updating secondary system time for
    guest

 xen/arch/x86/domain.c        | 21 ++++++++++++++++++---
 xen/arch/x86/hvm/hvm.c       |  2 ++
 xen/arch/x86/mm/guest_walk.c | 41 ++++++++++++++++++++++++++++-------------
 xen/arch/x86/time.c          | 12 +++++++++++-
 xen/include/asm-x86/domain.h | 24 ++++++++++++++++++++++--
 xen/include/public/vcpu.h    | 10 ++++++++++
 6 files changed, 91 insertions(+), 19 deletions(-)

-- 
1.8.3.1

[PATCH 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 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

[Feng Wu]

In this patch, we add a new hypervisor which allows guests to enable
SMAP check when Xen is updating the secondary system time for guest.
The SMAP check for this update is disabled by default.

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

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index b0c8810..a787c46 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1053,6 +1053,12 @@ arch_do_vcpu_op(
         break;
     }
 
+    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
+    {
+        v->arch.enable_smap_check = 1;
+        break;
+    }
+
     case VCPUOP_get_physid:
     {
         struct vcpu_get_physid cpu_id;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ef2411c..a922711 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4629,6 +4629,7 @@ static long hvm_vcpu_op(
     case VCPUOP_stop_singleshot_timer:
     case VCPUOP_register_vcpu_info:
     case VCPUOP_register_vcpu_time_memory_area:
+    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
         rc = do_vcpu_op(cmd, vcpuid, arg);
         break;
     default:
@@ -4688,6 +4689,7 @@ static long hvm_vcpu_op_compat32(
     case VCPUOP_stop_singleshot_timer:
     case VCPUOP_register_vcpu_info:
     case VCPUOP_register_vcpu_time_memory_area:
+    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
         rc = compat_vcpu_op(cmd, vcpuid, arg);
         break;
     default:
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index a4e1656..338381e 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,16 @@ bool_t update_secondary_system_time(const struct vcpu *v,
     if ( guest_handle_is_null(user_u) )
         return 1;
 
+    if ( v->arch.enable_smap_check )
+        v->arch.smap_check_policy = SMAP_CHECK_ENABLED;
+
     /* 1. Update userspace version. */
     if ( __copy_field_to_guest(user_u, u, version) == sizeof(u->version) )
+    {
+        if ( v->arch.enable_smap_check )
+            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 +847,9 @@ 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);
 
+    if ( v->arch.enable_smap_check )
+        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..051fed0 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -455,6 +455,13 @@ struct arch_vcpu
      *     SMAP_CHECK_DISABLED     - disable the check
      */
     uint8_t smap_check_policy;
+
+    /*
+     * This is a flag to indicate that whether the guest wants to enable
+     * SMAP check when Xen is updating the secondary system time from it.
+     */
+    bool_t enable_smap_check;
+    uint16_t pad;
 } __cacheline_aligned;
 
 #define SMAP_CHECK_HONOR_CPL_AC  0
@@ -466,7 +473,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 *);
diff --git a/xen/include/public/vcpu.h b/xen/include/public/vcpu.h
index e888daf..d348395 100644
--- a/xen/include/public/vcpu.h
+++ b/xen/include/public/vcpu.h
@@ -227,6 +227,16 @@ struct vcpu_register_time_memory_area {
 typedef struct vcpu_register_time_memory_area vcpu_register_time_memory_area_t;
 DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
 
+/*
+ * Flags to tell Xen whether we need to do the SMAP check when updating
+ * the secondary copy of the vcpu time when SMAP is enabled. Since the
+ * memory location for the secondary copy of the vcpu time may be mapped
+ * into userspace by guests intendedly, we let the guest to determine
+ * whether the check is needed. The default behavior of hypevisor is
+ * not doing the check.
+ */
+#define VCPUOP_enable_smap_check_vcpu_time_memory_area   14
+
 #endif /* __XEN_PUBLIC_VCPU_H__ */
 
 /*
-- 
1.8.3.1

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

[Sander Eikelenboom]

Tuesday, July 8, 2014, 1:18:16 AM, you wrote:

> 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

> The root cause of this issue is that the we try to get guest's SS
> register via hvm_get_segment_register() between setting 'current'
> and reloading the VMCS context for it.

> Feng Wu (2):
>   x86/hvm: Always do SMAP check when updating runstate_guest(v)
>   x86/hvm: honor guest's option when updating secondary system time for
>     guest

>  xen/arch/x86/domain.c        | 21 ++++++++++++++++++---
>  xen/arch/x86/hvm/hvm.c       |  2 ++
>  xen/arch/x86/mm/guest_walk.c | 41 ++++++++++++++++++++++++++++-------------
>  xen/arch/x86/time.c          | 12 +++++++++++-
>  xen/include/asm-x86/domain.h | 24 ++++++++++++++++++++++--
>  xen/include/public/vcpu.h    | 10 ++++++++++
>  6 files changed, 91 insertions(+), 19 deletions(-)

Hi Feng,

Just tested it and the warning is indeed gone.

Thanks,

Sander

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

[Andrew Cooper]

On 08/07/14 00:18, Feng Wu wrote:
> 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>

Why can't the VMCS be reloaded in vmx_ctxt_switch_to() rather than
vmx_do_resume() ?  The problem is not with updating the runstate area
persay, but due to using guest_walk_tables() during a context switch
before the VMCS has been loaded.

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

Can you describe what behavioural change this will cause.  From my
understanding, for a guest which has enabled CR4.SMAP, it will
unconditionally cause an -EFAULT for runstate areas in user pagetables?

~Andrew

> +            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 *);
>  

Re: [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

[Andrew Cooper]

On 08/07/14 00:18, Feng Wu wrote:
> In this patch, we add a new hypervisor which allows guests to enable
> SMAP check when Xen is updating the secondary system time for guest.
> The SMAP check for this update is disabled by default.
>
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Signed-off-by: Feng Wu <feng.wu@intel.com>

This patch uses the same v->arch.smap_check_policy variable as the
previous patch, meaning its value is unconditionally
SMAP_CHECK_HONOR_CPL_AC other than the short window while the runstate
area is being updated.

As a result, there appears to be no way for this to cause working
updates to a system time living in guest userspace.

~Andrew

> ---
>  xen/arch/x86/domain.c        |  6 ++++++
>  xen/arch/x86/hvm/hvm.c       |  2 ++
>  xen/arch/x86/time.c          | 12 +++++++++++-
>  xen/include/asm-x86/domain.h |  9 ++++++++-
>  xen/include/public/vcpu.h    | 10 ++++++++++
>  5 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index b0c8810..a787c46 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1053,6 +1053,12 @@ arch_do_vcpu_op(
>          break;
>      }
>  
> +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
> +    {
> +        v->arch.enable_smap_check = 1;
> +        break;
> +    }
> +
>      case VCPUOP_get_physid:
>      {
>          struct vcpu_get_physid cpu_id;
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index ef2411c..a922711 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4629,6 +4629,7 @@ static long hvm_vcpu_op(
>      case VCPUOP_stop_singleshot_timer:
>      case VCPUOP_register_vcpu_info:
>      case VCPUOP_register_vcpu_time_memory_area:
> +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
>          rc = do_vcpu_op(cmd, vcpuid, arg);
>          break;
>      default:
> @@ -4688,6 +4689,7 @@ static long hvm_vcpu_op_compat32(
>      case VCPUOP_stop_singleshot_timer:
>      case VCPUOP_register_vcpu_info:
>      case VCPUOP_register_vcpu_time_memory_area:
> +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
>          rc = compat_vcpu_op(cmd, vcpuid, arg);
>          break;
>      default:
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index a4e1656..338381e 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,16 @@ bool_t update_secondary_system_time(const struct vcpu *v,
>      if ( guest_handle_is_null(user_u) )
>          return 1;
>  
> +    if ( v->arch.enable_smap_check )
> +        v->arch.smap_check_policy = SMAP_CHECK_ENABLED;
> +
>      /* 1. Update userspace version. */
>      if ( __copy_field_to_guest(user_u, u, version) == sizeof(u->version) )
> +    {
> +        if ( v->arch.enable_smap_check )
> +            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 +847,9 @@ 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);
>  
> +    if ( v->arch.enable_smap_check )
> +        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..051fed0 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -455,6 +455,13 @@ struct arch_vcpu
>       *     SMAP_CHECK_DISABLED     - disable the check
>       */
>      uint8_t smap_check_policy;
> +
> +    /*
> +     * This is a flag to indicate that whether the guest wants to enable
> +     * SMAP check when Xen is updating the secondary system time from it.
> +     */
> +    bool_t enable_smap_check;
> +    uint16_t pad;
>  } __cacheline_aligned;
>  
>  #define SMAP_CHECK_HONOR_CPL_AC  0
> @@ -466,7 +473,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 *);
> diff --git a/xen/include/public/vcpu.h b/xen/include/public/vcpu.h
> index e888daf..d348395 100644
> --- a/xen/include/public/vcpu.h
> +++ b/xen/include/public/vcpu.h
> @@ -227,6 +227,16 @@ struct vcpu_register_time_memory_area {
>  typedef struct vcpu_register_time_memory_area vcpu_register_time_memory_area_t;
>  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>  
> +/*
> + * Flags to tell Xen whether we need to do the SMAP check when updating
> + * the secondary copy of the vcpu time when SMAP is enabled. Since the
> + * memory location for the secondary copy of the vcpu time may be mapped
> + * into userspace by guests intendedly, we let the guest to determine
> + * whether the check is needed. The default behavior of hypevisor is
> + * not doing the check.
> + */
> +#define VCPUOP_enable_smap_check_vcpu_time_memory_area   14
> +
>  #endif /* __XEN_PUBLIC_VCPU_H__ */
>  
>  /*

Re: [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

[Konrad Rzeszutek Wilk]

On Tue, Jul 08, 2014 at 07:18:18AM +0800, Feng Wu wrote:
> In this patch, we add a new hypervisor which allows guests to enable
> SMAP check when Xen is updating the secondary system time for guest.
> The SMAP check for this update is disabled by default.
> 
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
>  xen/arch/x86/domain.c        |  6 ++++++
>  xen/arch/x86/hvm/hvm.c       |  2 ++
>  xen/arch/x86/time.c          | 12 +++++++++++-
>  xen/include/asm-x86/domain.h |  9 ++++++++-
>  xen/include/public/vcpu.h    | 10 ++++++++++
>  5 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index b0c8810..a787c46 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1053,6 +1053,12 @@ arch_do_vcpu_op(
>          break;
>      }
>  
> +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
> +    {
> +        v->arch.enable_smap_check = 1;
> +        break;
> +    }
> +
>      case VCPUOP_get_physid:
>      {
>          struct vcpu_get_physid cpu_id;
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index ef2411c..a922711 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4629,6 +4629,7 @@ static long hvm_vcpu_op(
>      case VCPUOP_stop_singleshot_timer:
>      case VCPUOP_register_vcpu_info:
>      case VCPUOP_register_vcpu_time_memory_area:
> +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
>          rc = do_vcpu_op(cmd, vcpuid, arg);
>          break;
>      default:
> @@ -4688,6 +4689,7 @@ static long hvm_vcpu_op_compat32(
>      case VCPUOP_stop_singleshot_timer:
>      case VCPUOP_register_vcpu_info:
>      case VCPUOP_register_vcpu_time_memory_area:
> +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
>          rc = compat_vcpu_op(cmd, vcpuid, arg);
>          break;
>      default:
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index a4e1656..338381e 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,16 @@ bool_t update_secondary_system_time(const struct vcpu *v,
>      if ( guest_handle_is_null(user_u) )
>          return 1;
>  
> +    if ( v->arch.enable_smap_check )
> +        v->arch.smap_check_policy = SMAP_CHECK_ENABLED;
> +
>      /* 1. Update userspace version. */
>      if ( __copy_field_to_guest(user_u, u, version) == sizeof(u->version) )
> +    {
> +        if ( v->arch.enable_smap_check )
> +            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 +847,9 @@ 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);
>  
> +    if ( v->arch.enable_smap_check )
> +        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..051fed0 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -455,6 +455,13 @@ struct arch_vcpu
>       *     SMAP_CHECK_DISABLED     - disable the check
>       */
>      uint8_t smap_check_policy;
> +
> +    /*
> +     * This is a flag to indicate that whether the guest wants to enable
> +     * SMAP check when Xen is updating the secondary system time from it.
> +     */
> +    bool_t enable_smap_check;
> +    uint16_t pad;
>  } __cacheline_aligned;
>  
>  #define SMAP_CHECK_HONOR_CPL_AC  0
> @@ -466,7 +473,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 *);
> diff --git a/xen/include/public/vcpu.h b/xen/include/public/vcpu.h
> index e888daf..d348395 100644
> --- a/xen/include/public/vcpu.h
> +++ b/xen/include/public/vcpu.h
> @@ -227,6 +227,16 @@ struct vcpu_register_time_memory_area {
>  typedef struct vcpu_register_time_memory_area vcpu_register_time_memory_area_t;
>  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>  
> +/*
> + * Flags to tell Xen whether we need to do the SMAP check when updating
> + * the secondary copy of the vcpu time when SMAP is enabled. Since the
> + * memory location for the secondary copy of the vcpu time may be mapped
> + * into userspace by guests intendedly, we let the guest to determine

What is 'intendedly'?

> + * whether the check is needed. The default behavior of hypevisor is
> + * not doing the check.

Under what conditions do we want the guest to do the check? What is
the impact (performance) if we do the check?

Why don't we want by default do the check?

> + */
> +#define VCPUOP_enable_smap_check_vcpu_time_memory_area   14
> +
>  #endif /* __XEN_PUBLIC_VCPU_H__ */
>  
>  /*
> -- 
> 1.8.3.1
> 

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

[Wu, Feng]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Tuesday, July 08, 2014 6:05 PM
> To: Wu, Feng; xen-devel@lists.xen.org
> Cc: tim@xen.org; linux@eikelenboom.it; jbeulich@suse.com; keir@xen.org
> Subject: Re: [Xen-devel] [PATCH 1/2] x86/hvm: Always do SMAP check when
> updating runstate_guest(v)
> 
> On 08/07/14 00:18, Feng Wu wrote:
> > 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>
> 
> Why can't the VMCS be reloaded in vmx_ctxt_switch_to() rather than
> vmx_do_resume() ?  The problem is not with updating the runstate area
> persay, but due to using guest_walk_tables() during a context switch
> before the VMCS has been loaded.

This is another option in the discussion with Jan. However, seems
the current option is preferred by Jan.

> 
> > ---
> >  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);
> 
> Can you describe what behavioural change this will cause.  From my
> understanding, for a guest which has enabled CR4.SMAP, it will
> unconditionally cause an -EFAULT for runstate areas in user pagetables?
> 
> ~Andrew

Here is the call path according to the Xen call trace:

copy_to_user_hvm() --> hvm_copy_to_guest_virt_nofault() --> __hvm_copy()
--> paging_gva_to_gfn() --> hap_gva_to_gfn_3_levels() --> hap_p2m_ga_to_gfn_3_levels()
--> guest_walk_tables_3_levels()

In the case you mentioned, guest_walk_tables_3_levels() will return an non-zero value, so
hap_p2m_ga_to_gfn_3_levels will return INVALID_GFN, and this return value will be eventually
returned by paging_gva_to_gfn(). Hence HVMCOPY_bad_gva_to_gfn will be returned by
hvm_copy_to_guest_virt_nofault(). At last in copy_to_user_hvm() the following code is executed:

    rc = hvm_copy_to_guest_virt_nofault((unsigned long)to, (void *)from,
                                        len, 0);
    return rc ? len : 0; /* fake a copy_to_user() return code */

So, 'len' will be returned. Maybe this is not the expected behavior,
However, do you think there are some problems with this return logic in copy_to_user_hvm()
irrespective of SMAP cases? If hvm_copy_to_guest_virt_nofault() fails, it always return a
non-zero value, and in this case, copy_to_user_hvm() will continue to return 'len' instead of an error.

Thanks,
Feng

> 
> > +            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 *);
> >

Re: [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

[Wu, Feng]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Tuesday, July 08, 2014 6:08 PM
> To: Wu, Feng; xen-devel@lists.xen.org
> Cc: tim@xen.org; linux@eikelenboom.it; jbeulich@suse.com; keir@xen.org
> Subject: Re: [Xen-devel] [PATCH 2/2] x86/hvm: honor guest's option when
> updating secondary system time for guest
> 
> On 08/07/14 00:18, Feng Wu wrote:
> > In this patch, we add a new hypervisor which allows guests to enable
> > SMAP check when Xen is updating the secondary system time for guest.
> > The SMAP check for this update is disabled by default.
> >
> > Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> > Signed-off-by: Feng Wu <feng.wu@intel.com>
> 
> This patch uses the same v->arch.smap_check_policy variable as the
> previous patch, meaning its value is unconditionally
> SMAP_CHECK_HONOR_CPL_AC other than the short window while the
> runstate
> area is being updated.

Guests can enable it by hypercall ' VCPUOP_enable_smap_check_vcpu_time_memory_area ',
If the guest calls this hypercall, v->arch.enable_smap_check will be set to 1, so when updating
the secondary system time for guest, v->arch.smap_check_policy will be set to SMAP_CHECK_ENABLED.

Thanks,
Feng

> 
> As a result, there appears to be no way for this to cause working
> updates to a system time living in guest userspace.
> 
> ~Andrew
> 
> > ---
> >  xen/arch/x86/domain.c        |  6 ++++++
> >  xen/arch/x86/hvm/hvm.c       |  2 ++
> >  xen/arch/x86/time.c          | 12 +++++++++++-
> >  xen/include/asm-x86/domain.h |  9 ++++++++-
> >  xen/include/public/vcpu.h    | 10 ++++++++++
> >  5 files changed, 37 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index b0c8810..a787c46 100644
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -1053,6 +1053,12 @@ arch_do_vcpu_op(
> >          break;
> >      }
> >
> > +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
> > +    {
> > +        v->arch.enable_smap_check = 1;
> > +        break;
> > +    }
> > +
> >      case VCPUOP_get_physid:
> >      {
> >          struct vcpu_get_physid cpu_id;
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index ef2411c..a922711 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -4629,6 +4629,7 @@ static long hvm_vcpu_op(
> >      case VCPUOP_stop_singleshot_timer:
> >      case VCPUOP_register_vcpu_info:
> >      case VCPUOP_register_vcpu_time_memory_area:
> > +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
> >          rc = do_vcpu_op(cmd, vcpuid, arg);
> >          break;
> >      default:
> > @@ -4688,6 +4689,7 @@ static long hvm_vcpu_op_compat32(
> >      case VCPUOP_stop_singleshot_timer:
> >      case VCPUOP_register_vcpu_info:
> >      case VCPUOP_register_vcpu_time_memory_area:
> > +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
> >          rc = compat_vcpu_op(cmd, vcpuid, arg);
> >          break;
> >      default:
> > diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> > index a4e1656..338381e 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,16 @@ bool_t update_secondary_system_time(const
> struct vcpu *v,
> >      if ( guest_handle_is_null(user_u) )
> >          return 1;
> >
> > +    if ( v->arch.enable_smap_check )
> > +        v->arch.smap_check_policy = SMAP_CHECK_ENABLED;
> > +
> >      /* 1. Update userspace version. */
> >      if ( __copy_field_to_guest(user_u, u, version) == sizeof(u->version) )
> > +    {
> > +        if ( v->arch.enable_smap_check )
> > +            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 +847,9 @@ 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);
> >
> > +    if ( v->arch.enable_smap_check )
> > +        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..051fed0 100644
> > --- a/xen/include/asm-x86/domain.h
> > +++ b/xen/include/asm-x86/domain.h
> > @@ -455,6 +455,13 @@ struct arch_vcpu
> >       *     SMAP_CHECK_DISABLED     - disable the check
> >       */
> >      uint8_t smap_check_policy;
> > +
> > +    /*
> > +     * This is a flag to indicate that whether the guest wants to enable
> > +     * SMAP check when Xen is updating the secondary system time from
> it.
> > +     */
> > +    bool_t enable_smap_check;
> > +    uint16_t pad;
> >  } __cacheline_aligned;
> >
> >  #define SMAP_CHECK_HONOR_CPL_AC  0
> > @@ -466,7 +473,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 *);
> > diff --git a/xen/include/public/vcpu.h b/xen/include/public/vcpu.h
> > index e888daf..d348395 100644
> > --- a/xen/include/public/vcpu.h
> > +++ b/xen/include/public/vcpu.h
> > @@ -227,6 +227,16 @@ struct vcpu_register_time_memory_area {
> >  typedef struct vcpu_register_time_memory_area
> vcpu_register_time_memory_area_t;
> >  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
> >
> > +/*
> > + * Flags to tell Xen whether we need to do the SMAP check when updating
> > + * the secondary copy of the vcpu time when SMAP is enabled. Since the
> > + * memory location for the secondary copy of the vcpu time may be mapped
> > + * into userspace by guests intendedly, we let the guest to determine
> > + * whether the check is needed. The default behavior of hypevisor is
> > + * not doing the check.
> > + */
> > +#define VCPUOP_enable_smap_check_vcpu_time_memory_area   14
> > +
> >  #endif /* __XEN_PUBLIC_VCPU_H__ */
> >
> >  /*

Re: [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

[Wu, Feng]

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: Wednesday, July 09, 2014 12:14 AM
> To: Wu, Feng
> Cc: xen-devel@lists.xen.org; keir@xen.org; jbeulich@suse.com; tim@xen.org;
> linux@eikelenboom.it
> Subject: Re: [PATCH 2/2] x86/hvm: honor guest's option when updating
> secondary system time for guest
> 
> On Tue, Jul 08, 2014 at 07:18:18AM +0800, Feng Wu wrote:
> > In this patch, we add a new hypervisor which allows guests to enable
> > SMAP check when Xen is updating the secondary system time for guest.
> > The SMAP check for this update is disabled by default.
> >
> > Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> > Signed-off-by: Feng Wu <feng.wu@intel.com>
> > ---
> >  xen/arch/x86/domain.c        |  6 ++++++
> >  xen/arch/x86/hvm/hvm.c       |  2 ++
> >  xen/arch/x86/time.c          | 12 +++++++++++-
> >  xen/include/asm-x86/domain.h |  9 ++++++++-
> >  xen/include/public/vcpu.h    | 10 ++++++++++
> >  5 files changed, 37 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index b0c8810..a787c46 100644
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -1053,6 +1053,12 @@ arch_do_vcpu_op(
> >          break;
> >      }
> >
> > +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
> > +    {
> > +        v->arch.enable_smap_check = 1;
> > +        break;
> > +    }
> > +
> >      case VCPUOP_get_physid:
> >      {
> >          struct vcpu_get_physid cpu_id;
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index ef2411c..a922711 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -4629,6 +4629,7 @@ static long hvm_vcpu_op(
> >      case VCPUOP_stop_singleshot_timer:
> >      case VCPUOP_register_vcpu_info:
> >      case VCPUOP_register_vcpu_time_memory_area:
> > +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
> >          rc = do_vcpu_op(cmd, vcpuid, arg);
> >          break;
> >      default:
> > @@ -4688,6 +4689,7 @@ static long hvm_vcpu_op_compat32(
> >      case VCPUOP_stop_singleshot_timer:
> >      case VCPUOP_register_vcpu_info:
> >      case VCPUOP_register_vcpu_time_memory_area:
> > +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
> >          rc = compat_vcpu_op(cmd, vcpuid, arg);
> >          break;
> >      default:
> > diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> > index a4e1656..338381e 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,16 @@ bool_t update_secondary_system_time(const
> struct vcpu *v,
> >      if ( guest_handle_is_null(user_u) )
> >          return 1;
> >
> > +    if ( v->arch.enable_smap_check )
> > +        v->arch.smap_check_policy = SMAP_CHECK_ENABLED;
> > +
> >      /* 1. Update userspace version. */
> >      if ( __copy_field_to_guest(user_u, u, version) == sizeof(u->version) )
> > +    {
> > +        if ( v->arch.enable_smap_check )
> > +            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 +847,9 @@ 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);
> >
> > +    if ( v->arch.enable_smap_check )
> > +        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..051fed0 100644
> > --- a/xen/include/asm-x86/domain.h
> > +++ b/xen/include/asm-x86/domain.h
> > @@ -455,6 +455,13 @@ struct arch_vcpu
> >       *     SMAP_CHECK_DISABLED     - disable the check
> >       */
> >      uint8_t smap_check_policy;
> > +
> > +    /*
> > +     * This is a flag to indicate that whether the guest wants to enable
> > +     * SMAP check when Xen is updating the secondary system time from
> it.
> > +     */
> > +    bool_t enable_smap_check;
> > +    uint16_t pad;
> >  } __cacheline_aligned;
> >
> >  #define SMAP_CHECK_HONOR_CPL_AC  0
> > @@ -466,7 +473,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 *);
> > diff --git a/xen/include/public/vcpu.h b/xen/include/public/vcpu.h
> > index e888daf..d348395 100644
> > --- a/xen/include/public/vcpu.h
> > +++ b/xen/include/public/vcpu.h
> > @@ -227,6 +227,16 @@ struct vcpu_register_time_memory_area {
> >  typedef struct vcpu_register_time_memory_area
> vcpu_register_time_memory_area_t;
> >  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
> >
> > +/*
> > + * Flags to tell Xen whether we need to do the SMAP check when updating
> > + * the secondary copy of the vcpu time when SMAP is enabled. Since the
> > + * memory location for the secondary copy of the vcpu time may be mapped
> > + * into userspace by guests intendedly, we let the guest to determine
> 
> What is 'intendedly'?
> 
> > + * whether the check is needed. The default behavior of hypevisor is
> > + * not doing the check.
> 
> Under what conditions do we want the guest to do the check? What is
> the impact (performance) if we do the check?
> 
> Why don't we want by default do the check?

If SMAP is enabled, supervisor mode accesses to user-accessible pages are not
allowed when CPL=0 & EFLAGS.AC=0. 

- If the address passed to hypervisor is in guest kernel space, we need to do
the check. If the check fails, it means some abnormal happened.
- If the address passed to hypervisor is in guest user space, which means guest
wants hypervisor to put the information to its user-space address. In this case,
We shouldn't do the check, or a SMAP violation will be triggered and this is not
what the guest wants.

Thanks,
Feng

> 
> > + */
> > +#define VCPUOP_enable_smap_check_vcpu_time_memory_area   14
> > +
> >  #endif /* __XEN_PUBLIC_VCPU_H__ */
> >
> >  /*
> > --
> > 1.8.3.1
> >

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

[Tim Deegan]

At 07:18 +0800 on 08 Jul (1404800297), Feng Wu wrote:
> 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.

Surely the correct behaviour is _not_ to do the check -- this is the
context switch path in the _hypervisor_, not a guest-kernel operation.

Apart from that, I very much dislike this roundabout mechanism; there
may be other paths that want to walk this VCPU's tables while your
operation is running.

I think that the copy_to_guest() path probably ought to have an opt-out
from SMAP, probably signalled by inventing a new PFEC bit to say that
you don't want it.

Cheers,

Tim.

> 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
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

[Tim Deegan]

At 07:18 +0800 on 08 Jul (1404800298), Feng Wu wrote:
> In this patch, we add a new hypervisor which allows guests to enable
> SMAP check when Xen is updating the secondary system time for guest.
> The SMAP check for this update is disabled by default.

Again, it seems like we should never do SMAP checks here, since (a)
this is a hypervisor operation, totally independent of whether the
vcpu is in user mode or not, (b) this happens asynchronously to the
guest so we can't raise a fault, an (c) this is explicitly _supposed_
to access a user-space mapping. 

Tim.

> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
>  xen/arch/x86/domain.c        |  6 ++++++
>  xen/arch/x86/hvm/hvm.c       |  2 ++
>  xen/arch/x86/time.c          | 12 +++++++++++-
>  xen/include/asm-x86/domain.h |  9 ++++++++-
>  xen/include/public/vcpu.h    | 10 ++++++++++
>  5 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index b0c8810..a787c46 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1053,6 +1053,12 @@ arch_do_vcpu_op(
>          break;
>      }
>  
> +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
> +    {
> +        v->arch.enable_smap_check = 1;
> +        break;
> +    }
> +
>      case VCPUOP_get_physid:
>      {
>          struct vcpu_get_physid cpu_id;
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index ef2411c..a922711 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4629,6 +4629,7 @@ static long hvm_vcpu_op(
>      case VCPUOP_stop_singleshot_timer:
>      case VCPUOP_register_vcpu_info:
>      case VCPUOP_register_vcpu_time_memory_area:
> +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
>          rc = do_vcpu_op(cmd, vcpuid, arg);
>          break;
>      default:
> @@ -4688,6 +4689,7 @@ static long hvm_vcpu_op_compat32(
>      case VCPUOP_stop_singleshot_timer:
>      case VCPUOP_register_vcpu_info:
>      case VCPUOP_register_vcpu_time_memory_area:
> +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
>          rc = compat_vcpu_op(cmd, vcpuid, arg);
>          break;
>      default:
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index a4e1656..338381e 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,16 @@ bool_t update_secondary_system_time(const struct vcpu *v,
>      if ( guest_handle_is_null(user_u) )
>          return 1;
>  
> +    if ( v->arch.enable_smap_check )
> +        v->arch.smap_check_policy = SMAP_CHECK_ENABLED;
> +
¼>      /* 1. Update userspace version. */
>      if ( __copy_field_to_guest(user_u, u, version) == sizeof(u->version) )
> +    {
> +        if ( v->arch.enable_smap_check )
> +            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 +847,9 @@ 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);
>  
> +    if ( v->arch.enable_smap_check )
> +        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..051fed0 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -455,6 +455,13 @@ struct arch_vcpu
>       *     SMAP_CHECK_DISABLED     - disable the check
>       */
>      uint8_t smap_check_policy;
> +
> +    /*
> +     * This is a flag to indicate that whether the guest wants to enable
> +     * SMAP check when Xen is updating the secondary system time from it.
> +     */
> +    bool_t enable_smap_check;
> +    uint16_t pad;
>  } __cacheline_aligned;
>  
>  #define SMAP_CHECK_HONOR_CPL_AC  0
> @@ -466,7 +473,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 *);
> diff --git a/xen/include/public/vcpu.h b/xen/include/public/vcpu.h
> index e888daf..d348395 100644
> --- a/xen/include/public/vcpu.h
> +++ b/xen/include/public/vcpu.h
> @@ -227,6 +227,16 @@ struct vcpu_register_time_memory_area {
>  typedef struct vcpu_register_time_memory_area vcpu_register_time_memory_area_t;
>  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>  
> +/*
> + * Flags to tell Xen whether we need to do the SMAP check when updating
> + * the secondary copy of the vcpu time when SMAP is enabled. Since the
> + * memory location for the secondary copy of the vcpu time may be mapped
> + * into userspace by guests intendedly, we let the guest to determine
> + * whether the check is needed. The default behavior of hypevisor is
> + * not doing the check.
> + */
> +#define VCPUOP_enable_smap_check_vcpu_time_memory_area   14
> +
>  #endif /* __XEN_PUBLIC_VCPU_H__ */
>  
>  /*
> -- 
> 1.8.3.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 09.07.14 at 03:33, <feng.wu@intel.com> wrote:
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> On 08/07/14 00:18, Feng Wu wrote:
>> > 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>
>> 
>> Why can't the VMCS be reloaded in vmx_ctxt_switch_to() rather than
>> vmx_do_resume() ?  The problem is not with updating the runstate area
>> persay, but due to using guest_walk_tables() during a context switch
>> before the VMCS has been loaded.
> 
> This is another option in the discussion with Jan. However, seems
> the current option is preferred by Jan.

I don't think I ever said so. What I probably did say is that doing it
the alternative way, while presumably better, will need quite a bit
more care (i.e. is overall more risky than the change here).

Jan

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

[Jan Beulich]

>>> On 10.07.14 at 12:56, <tim@xen.org> wrote:
> At 07:18 +0800 on 08 Jul (1404800297), Feng Wu wrote:
>> 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.
> 
> Surely the correct behaviour is _not_ to do the check -- this is the
> context switch path in the _hypervisor_, not a guest-kernel operation.

But it is being "asked for" by the kernel, and hence should be treated
as implicit supervisor mode access just like e.g. descriptor table
accesses (see also the earlier discussion in the thread where the problem
got reported).

Jan

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

[Tim Deegan]

At 13:11 +0100 on 23 Jul (1406117492), Jan Beulich wrote:
> >>> On 10.07.14 at 12:56, <tim@xen.org> wrote:
> > At 07:18 +0800 on 08 Jul (1404800297), Feng Wu wrote:
> >> 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.
> > 
> > Surely the correct behaviour is _not_ to do the check -- this is the
> > context switch path in the _hypervisor_, not a guest-kernel operation.
> 
> But it is being "asked for" by the kernel, and hence should be treated
> as implicit supervisor mode access just like e.g. descriptor table
> accesses (see also the earlier discussion in the thread where the problem
> got reported).

OK, that makes sense.

Tim.

Re: [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

[Jan Beulich]

>>> On 10.07.14 at 13:00, <tim@xen.org> wrote:
> At 07:18 +0800 on 08 Jul (1404800298), Feng Wu wrote:
>> In this patch, we add a new hypervisor which allows guests to enable
>> SMAP check when Xen is updating the secondary system time for guest.
>> The SMAP check for this update is disabled by default.
> 
> Again, it seems like we should never do SMAP checks here, since (a)
> this is a hypervisor operation, totally independent of whether the
> vcpu is in user mode or not, (b) this happens asynchronously to the
> guest so we can't raise a fault, an (c) this is explicitly _supposed_
> to access a user-space mapping. 

Not exactly: It is supposed to access a page that has a user space
mapping, but there's no implication that the update has to happen
through this user space mapping. In fact it is more likely for this not
to be the case, as I'd expect the user mode mapping to be r/o
while the mapping via which the update is to happen obviously has
to be r/w).

Jan

Re: [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

[Jan Beulich]

>>> On 08.07.14 at 01:18, <feng.wu@intel.com> wrote:
> --- a/xen/include/public/vcpu.h
> +++ b/xen/include/public/vcpu.h
> @@ -227,6 +227,16 @@ struct vcpu_register_time_memory_area {
>  typedef struct vcpu_register_time_memory_area 
> vcpu_register_time_memory_area_t;
>  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>  
> +/*
> + * Flags to tell Xen whether we need to do the SMAP check when updating
> + * the secondary copy of the vcpu time when SMAP is enabled. Since the
> + * memory location for the secondary copy of the vcpu time may be mapped
> + * into userspace by guests intendedly, we let the guest to determine
> + * whether the check is needed. The default behavior of hypevisor is
> + * not doing the check.
> + */
> +#define VCPUOP_enable_smap_check_vcpu_time_memory_area   14

I think the new op to be VCPUOP_register_vcpu_time_memory_area_smap,
identical to VCPUOP_register_vcpu_time_memory_area apart from also
setting the flag, would be more natural. But considering what I just wrote
in the reply to Tim I guess we can expect a nun-user mapping to be
presented here anyway, i.e. we wouldn't need to new operation at all.

Jan

Re: [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, July 23, 2014 8:19 PM
> To: Wu, Feng
> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org; konrad.wilk@oracle.com;
> keir@xen.org; tim@xen.org
> Subject: Re: [PATCH 2/2] x86/hvm: honor guest's option when updating
> secondary system time for guest
> 
> >>> On 08.07.14 at 01:18, <feng.wu@intel.com> wrote:
> > --- a/xen/include/public/vcpu.h
> > +++ b/xen/include/public/vcpu.h
> > @@ -227,6 +227,16 @@ struct vcpu_register_time_memory_area {
> >  typedef struct vcpu_register_time_memory_area
> > vcpu_register_time_memory_area_t;
> >  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
> >
> > +/*
> > + * Flags to tell Xen whether we need to do the SMAP check when updating
> > + * the secondary copy of the vcpu time when SMAP is enabled. Since the
> > + * memory location for the secondary copy of the vcpu time may be mapped
> > + * into userspace by guests intendedly, we let the guest to determine
> > + * whether the check is needed. The default behavior of hypevisor is
> > + * not doing the check.
> > + */
> > +#define VCPUOP_enable_smap_check_vcpu_time_memory_area   14
> 
> I think the new op to be VCPUOP_register_vcpu_time_memory_area_smap,
> identical to VCPUOP_register_vcpu_time_memory_area apart from also
> setting the flag, would be more natural. But considering what I just wrote
> in the reply to Tim I guess we can expect a nun-user mapping to be
> presented here anyway, i.e. we wouldn't need to new operation at all.

Do you mean since the user-paging is r/o, guest will pass a r/w kernel page to
Xen for updating the system time. So we don't need to do the SMAP check
in this case?

Thanks,
Feng
> 
> Jan

Re: [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

[Jan Beulich]

>>> On 25.07.14 at 06:30, <feng.wu@intel.com> wrote:

> 
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Wednesday, July 23, 2014 8:19 PM
>> To: Wu, Feng
>> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org; konrad.wilk@oracle.com;
>> keir@xen.org; tim@xen.org 
>> Subject: Re: [PATCH 2/2] x86/hvm: honor guest's option when updating
>> secondary system time for guest
>> 
>> >>> On 08.07.14 at 01:18, <feng.wu@intel.com> wrote:
>> > --- a/xen/include/public/vcpu.h
>> > +++ b/xen/include/public/vcpu.h
>> > @@ -227,6 +227,16 @@ struct vcpu_register_time_memory_area {
>> >  typedef struct vcpu_register_time_memory_area
>> > vcpu_register_time_memory_area_t;
>> >  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>> >
>> > +/*
>> > + * Flags to tell Xen whether we need to do the SMAP check when updating
>> > + * the secondary copy of the vcpu time when SMAP is enabled. Since the
>> > + * memory location for the secondary copy of the vcpu time may be mapped
>> > + * into userspace by guests intendedly, we let the guest to determine
>> > + * whether the check is needed. The default behavior of hypevisor is
>> > + * not doing the check.
>> > + */
>> > +#define VCPUOP_enable_smap_check_vcpu_time_memory_area   14
>> 
>> I think the new op to be VCPUOP_register_vcpu_time_memory_area_smap,
>> identical to VCPUOP_register_vcpu_time_memory_area apart from also
>> setting the flag, would be more natural. But considering what I just wrote
>> in the reply to Tim I guess we can expect a nun-user mapping to be
>> presented here anyway, i.e. we wouldn't need to new operation at all.
> 
> Do you mean since the user-paging is r/o, guest will pass a r/w kernel page 
> to
> Xen for updating the system time. So we don't need to do the SMAP check
> in this case?

If the user page is r/o, it's VA obviously can't be used for updating by
Xen. Hence the kernel has to provide a r/w mapped VA. That should be
subject to SMAP checking (consistent with the runstate area handling),
to make sure it's not a user accessible mapping.

Jan

Re: [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, July 25, 2014 3:26 PM
> To: Wu, Feng
> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org; konrad.wilk@oracle.com;
> keir@xen.org; tim@xen.org
> Subject: RE: [PATCH 2/2] x86/hvm: honor guest's option when updating
> secondary system time for guest
> 
> >>> On 25.07.14 at 06:30, <feng.wu@intel.com> wrote:
> 
> >
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Wednesday, July 23, 2014 8:19 PM
> >> To: Wu, Feng
> >> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org;
> konrad.wilk@oracle.com;
> >> keir@xen.org; tim@xen.org
> >> Subject: Re: [PATCH 2/2] x86/hvm: honor guest's option when updating
> >> secondary system time for guest
> >>
> >> >>> On 08.07.14 at 01:18, <feng.wu@intel.com> wrote:
> >> > --- a/xen/include/public/vcpu.h
> >> > +++ b/xen/include/public/vcpu.h
> >> > @@ -227,6 +227,16 @@ struct vcpu_register_time_memory_area {
> >> >  typedef struct vcpu_register_time_memory_area
> >> > vcpu_register_time_memory_area_t;
> >> >  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
> >> >
> >> > +/*
> >> > + * Flags to tell Xen whether we need to do the SMAP check when
> updating
> >> > + * the secondary copy of the vcpu time when SMAP is enabled. Since the
> >> > + * memory location for the secondary copy of the vcpu time may be
> mapped
> >> > + * into userspace by guests intendedly, we let the guest to determine
> >> > + * whether the check is needed. The default behavior of hypevisor is
> >> > + * not doing the check.
> >> > + */
> >> > +#define VCPUOP_enable_smap_check_vcpu_time_memory_area   14
> >>
> >> I think the new op to be VCPUOP_register_vcpu_time_memory_area_smap,
> >> identical to VCPUOP_register_vcpu_time_memory_area apart from also
> >> setting the flag, would be more natural. But considering what I just wrote
> >> in the reply to Tim I guess we can expect a nun-user mapping to be
> >> presented here anyway, i.e. we wouldn't need to new operation at all.
> >
> > Do you mean since the user-paging is r/o, guest will pass a r/w kernel page
> > to
> > Xen for updating the system time. So we don't need to do the SMAP check
> > in this case?
> 
> If the user page is r/o, it's VA obviously can't be used for updating by
> Xen. Hence the kernel has to provide a r/w mapped VA. That should be
> subject to SMAP checking (consistent with the runstate area handling),
> to make sure it's not a user accessible mapping.

But there are two possible problems here:
1. Is it possible that guest passes a user r/w page to update the system time information?
2. Even the user page is r/o, the kernel can still use it to update the system time info when WP is disabled.

Thanks,
Feng

> 
> Jan

Re: [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

[Jan Beulich]

>>> On 25.07.14 at 09:33, <feng.wu@intel.com> wrote:

> 
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Friday, July 25, 2014 3:26 PM
>> To: Wu, Feng
>> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org; konrad.wilk@oracle.com;
>> keir@xen.org; tim@xen.org 
>> Subject: RE: [PATCH 2/2] x86/hvm: honor guest's option when updating
>> secondary system time for guest
>> 
>> >>> On 25.07.14 at 06:30, <feng.wu@intel.com> wrote:
>> 
>> >
>> >> -----Original Message-----
>> >> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> Sent: Wednesday, July 23, 2014 8:19 PM
>> >> To: Wu, Feng
>> >> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org;
>> konrad.wilk@oracle.com;
>> >> keir@xen.org; tim@xen.org 
>> >> Subject: Re: [PATCH 2/2] x86/hvm: honor guest's option when updating
>> >> secondary system time for guest
>> >>
>> >> >>> On 08.07.14 at 01:18, <feng.wu@intel.com> wrote:
>> >> > --- a/xen/include/public/vcpu.h
>> >> > +++ b/xen/include/public/vcpu.h
>> >> > @@ -227,6 +227,16 @@ struct vcpu_register_time_memory_area {
>> >> >  typedef struct vcpu_register_time_memory_area
>> >> > vcpu_register_time_memory_area_t;
>> >> >  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>> >> >
>> >> > +/*
>> >> > + * Flags to tell Xen whether we need to do the SMAP check when
>> updating
>> >> > + * the secondary copy of the vcpu time when SMAP is enabled. Since the
>> >> > + * memory location for the secondary copy of the vcpu time may be
>> mapped
>> >> > + * into userspace by guests intendedly, we let the guest to determine
>> >> > + * whether the check is needed. The default behavior of hypevisor is
>> >> > + * not doing the check.
>> >> > + */
>> >> > +#define VCPUOP_enable_smap_check_vcpu_time_memory_area   14
>> >>
>> >> I think the new op to be VCPUOP_register_vcpu_time_memory_area_smap,
>> >> identical to VCPUOP_register_vcpu_time_memory_area apart from also
>> >> setting the flag, would be more natural. But considering what I just wrote
>> >> in the reply to Tim I guess we can expect a nun-user mapping to be
>> >> presented here anyway, i.e. we wouldn't need to new operation at all.
>> >
>> > Do you mean since the user-paging is r/o, guest will pass a r/w kernel page
>> > to
>> > Xen for updating the system time. So we don't need to do the SMAP check
>> > in this case?
>> 
>> If the user page is r/o, it's VA obviously can't be used for updating by
>> Xen. Hence the kernel has to provide a r/w mapped VA. That should be
>> subject to SMAP checking (consistent with the runstate area handling),
>> to make sure it's not a user accessible mapping.
> 
> But there are two possible problems here:
> 1. Is it possible that guest passes a user r/w page to update the system 
> time information?

The guest kernel may pass a page that was originally mapped r/w-
kernel, but the mapping later gets (say inadvertently) changed to
r/w-user. That's why I think the SMAP checking ought to be done
here.

> 2. Even the user page is r/o, the kernel can still use it to update the 
> system time info when WP is disabled.

The kernel can - for its own accesses to the page - do whatever
it wants. It can't, however, control Xen's updating of it, and Xen
won't do that with CR0.WP clear (leaving aside Aravindh's PV
mem-access work, where this option is currently being discussed).

Jan

Re: [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, July 25, 2014 3:40 PM
> To: Wu, Feng
> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org; konrad.wilk@oracle.com;
> keir@xen.org; tim@xen.org
> Subject: RE: [PATCH 2/2] x86/hvm: honor guest's option when updating
> secondary system time for guest
> 
> >>> On 25.07.14 at 09:33, <feng.wu@intel.com> wrote:
> 
> >
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Friday, July 25, 2014 3:26 PM
> >> To: Wu, Feng
> >> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org;
> konrad.wilk@oracle.com;
> >> keir@xen.org; tim@xen.org
> >> Subject: RE: [PATCH 2/2] x86/hvm: honor guest's option when updating
> >> secondary system time for guest
> >>
> >> >>> On 25.07.14 at 06:30, <feng.wu@intel.com> wrote:
> >>
> >> >
> >> >> -----Original Message-----
> >> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >> Sent: Wednesday, July 23, 2014 8:19 PM
> >> >> To: Wu, Feng
> >> >> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org;
> >> konrad.wilk@oracle.com;
> >> >> keir@xen.org; tim@xen.org
> >> >> Subject: Re: [PATCH 2/2] x86/hvm: honor guest's option when updating
> >> >> secondary system time for guest
> >> >>
> >> >> >>> On 08.07.14 at 01:18, <feng.wu@intel.com> wrote:
> >> >> > --- a/xen/include/public/vcpu.h
> >> >> > +++ b/xen/include/public/vcpu.h
> >> >> > @@ -227,6 +227,16 @@ struct vcpu_register_time_memory_area {
> >> >> >  typedef struct vcpu_register_time_memory_area
> >> >> > vcpu_register_time_memory_area_t;
> >> >> >  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
> >> >> >
> >> >> > +/*
> >> >> > + * Flags to tell Xen whether we need to do the SMAP check when
> >> updating
> >> >> > + * the secondary copy of the vcpu time when SMAP is enabled. Since
> the
> >> >> > + * memory location for the secondary copy of the vcpu time may be
> >> mapped
> >> >> > + * into userspace by guests intendedly, we let the guest to determine
> >> >> > + * whether the check is needed. The default behavior of hypevisor is
> >> >> > + * not doing the check.
> >> >> > + */
> >> >> > +#define VCPUOP_enable_smap_check_vcpu_time_memory_area
> 14
> >> >>
> >> >> I think the new op to be
> VCPUOP_register_vcpu_time_memory_area_smap,
> >> >> identical to VCPUOP_register_vcpu_time_memory_area apart from also
> >> >> setting the flag, would be more natural. But considering what I just
> wrote
> >> >> in the reply to Tim I guess we can expect a nun-user mapping to be
> >> >> presented here anyway, i.e. we wouldn't need to new operation at all.
> >> >
> >> > Do you mean since the user-paging is r/o, guest will pass a r/w kernel page
> >> > to
> >> > Xen for updating the system time. So we don't need to do the SMAP check
> >> > in this case?
> >>
> >> If the user page is r/o, it's VA obviously can't be used for updating by
> >> Xen. Hence the kernel has to provide a r/w mapped VA. That should be
> >> subject to SMAP checking (consistent with the runstate area handling),
> >> to make sure it's not a user accessible mapping.
> >
> > But there are two possible problems here:
> > 1. Is it possible that guest passes a user r/w page to update the system
> > time information?
> 
> The guest kernel may pass a page that was originally mapped r/w-
> kernel, but the mapping later gets (say inadvertently) changed to
> r/w-user. That's why I think the SMAP checking ought to be done
> here.

But in another case, as what we discussed before, if the guest intended to
pass a user r/w page to get the time information. Do we need to do the
SMAP checking for this?

> 
> > 2. Even the user page is r/o, the kernel can still use it to update the
> > system time info when WP is disabled.
> 
> The kernel can - for its own accesses to the page - do whatever
> it wants. It can't, however, control Xen's updating of it, and Xen
> won't do that with CR0.WP clear (leaving aside Aravindh's PV
> mem-access work, where this option is currently being discussed).

Just like my comments above, my point is in some case, guest may pass a
user page (r/o or r/w) to get the time information, I doubt whether the SMAP
checking should be done for this?

Thanks,
Feng

> 
> Jan

Re: [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

[Jan Beulich]

>>> On 25.07.14 at 10:03, <feng.wu@intel.com> wrote:

> 
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Friday, July 25, 2014 3:40 PM
>> To: Wu, Feng
>> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org; konrad.wilk@oracle.com;
>> keir@xen.org; tim@xen.org 
>> Subject: RE: [PATCH 2/2] x86/hvm: honor guest's option when updating
>> secondary system time for guest
>> 
>> >>> On 25.07.14 at 09:33, <feng.wu@intel.com> wrote:
>> 
>> >
>> >> -----Original Message-----
>> >> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> Sent: Friday, July 25, 2014 3:26 PM
>> >> To: Wu, Feng
>> >> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org;
>> konrad.wilk@oracle.com;
>> >> keir@xen.org; tim@xen.org 
>> >> Subject: RE: [PATCH 2/2] x86/hvm: honor guest's option when updating
>> >> secondary system time for guest
>> >>
>> >> >>> On 25.07.14 at 06:30, <feng.wu@intel.com> wrote:
>> >>
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> >> Sent: Wednesday, July 23, 2014 8:19 PM
>> >> >> To: Wu, Feng
>> >> >> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org;
>> >> konrad.wilk@oracle.com;
>> >> >> keir@xen.org; tim@xen.org 
>> >> >> Subject: Re: [PATCH 2/2] x86/hvm: honor guest's option when updating
>> >> >> secondary system time for guest
>> >> >>
>> >> >> >>> On 08.07.14 at 01:18, <feng.wu@intel.com> wrote:
>> >> >> > --- a/xen/include/public/vcpu.h
>> >> >> > +++ b/xen/include/public/vcpu.h
>> >> >> > @@ -227,6 +227,16 @@ struct vcpu_register_time_memory_area {
>> >> >> >  typedef struct vcpu_register_time_memory_area
>> >> >> > vcpu_register_time_memory_area_t;
>> >> >> >  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>> >> >> >
>> >> >> > +/*
>> >> >> > + * Flags to tell Xen whether we need to do the SMAP check when
>> >> updating
>> >> >> > + * the secondary copy of the vcpu time when SMAP is enabled. Since
>> the
>> >> >> > + * memory location for the secondary copy of the vcpu time may be
>> >> mapped
>> >> >> > + * into userspace by guests intendedly, we let the guest to determine
>> >> >> > + * whether the check is needed. The default behavior of hypevisor is
>> >> >> > + * not doing the check.
>> >> >> > + */
>> >> >> > +#define VCPUOP_enable_smap_check_vcpu_time_memory_area
>> 14
>> >> >>
>> >> >> I think the new op to be
>> VCPUOP_register_vcpu_time_memory_area_smap,
>> >> >> identical to VCPUOP_register_vcpu_time_memory_area apart from also
>> >> >> setting the flag, would be more natural. But considering what I just
>> wrote
>> >> >> in the reply to Tim I guess we can expect a nun-user mapping to be
>> >> >> presented here anyway, i.e. we wouldn't need to new operation at all.
>> >> >
>> >> > Do you mean since the user-paging is r/o, guest will pass a r/w kernel page
>> >> > to
>> >> > Xen for updating the system time. So we don't need to do the SMAP check
>> >> > in this case?
>> >>
>> >> If the user page is r/o, it's VA obviously can't be used for updating by
>> >> Xen. Hence the kernel has to provide a r/w mapped VA. That should be
>> >> subject to SMAP checking (consistent with the runstate area handling),
>> >> to make sure it's not a user accessible mapping.
>> >
>> > But there are two possible problems here:
>> > 1. Is it possible that guest passes a user r/w page to update the system
>> > time information?
>> 
>> The guest kernel may pass a page that was originally mapped r/w-
>> kernel, but the mapping later gets (say inadvertently) changed to
>> r/w-user. That's why I think the SMAP checking ought to be done
>> here.
> 
> But in another case, as what we discussed before, if the guest intended to
> pass a user r/w page to get the time information. Do we need to do the
> SMAP checking for this?

The question is whether we should bother supporting such insecure
kernels.

>> > 2. Even the user page is r/o, the kernel can still use it to update the
>> > system time info when WP is disabled.
>> 
>> The kernel can - for its own accesses to the page - do whatever
>> it wants. It can't, however, control Xen's updating of it, and Xen
>> won't do that with CR0.WP clear (leaving aside Aravindh's PV
>> mem-access work, where this option is currently being discussed).
> 
> Just like my comments above, my point is in some case, guest may pass a
> user page (r/o or r/w) to get the time information, I doubt whether the SMAP
> checking should be done for this?

If we want to allow kernels to expose the time data r/w to user mode,
then yes, we would need to avoid the SMAP check. But as said above,
I don't think helping kernels to be insecure is of any particular value.

Jan

Re: [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, July 25, 2014 4:32 PM
> To: Wu, Feng
> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org; konrad.wilk@oracle.com;
> keir@xen.org; tim@xen.org
> Subject: RE: [PATCH 2/2] x86/hvm: honor guest's option when updating
> secondary system time for guest
> 
> >>> On 25.07.14 at 10:03, <feng.wu@intel.com> wrote:
> 
> >
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Friday, July 25, 2014 3:40 PM
> >> To: Wu, Feng
> >> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org;
> konrad.wilk@oracle.com;
> >> keir@xen.org; tim@xen.org
> >> Subject: RE: [PATCH 2/2] x86/hvm: honor guest's option when updating
> >> secondary system time for guest
> >>
> >> >>> On 25.07.14 at 09:33, <feng.wu@intel.com> wrote:
> >>
> >> >
> >> >> -----Original Message-----
> >> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >> Sent: Friday, July 25, 2014 3:26 PM
> >> >> To: Wu, Feng
> >> >> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org;
> >> konrad.wilk@oracle.com;
> >> >> keir@xen.org; tim@xen.org
> >> >> Subject: RE: [PATCH 2/2] x86/hvm: honor guest's option when updating
> >> >> secondary system time for guest
> >> >>
> >> >> >>> On 25.07.14 at 06:30, <feng.wu@intel.com> wrote:
> >> >>
> >> >> >
> >> >> >> -----Original Message-----
> >> >> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >> >> Sent: Wednesday, July 23, 2014 8:19 PM
> >> >> >> To: Wu, Feng
> >> >> >> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org;
> >> >> konrad.wilk@oracle.com;
> >> >> >> keir@xen.org; tim@xen.org
> >> >> >> Subject: Re: [PATCH 2/2] x86/hvm: honor guest's option when
> updating
> >> >> >> secondary system time for guest
> >> >> >>
> >> >> >> >>> On 08.07.14 at 01:18, <feng.wu@intel.com> wrote:
> >> >> >> > --- a/xen/include/public/vcpu.h
> >> >> >> > +++ b/xen/include/public/vcpu.h
> >> >> >> > @@ -227,6 +227,16 @@ struct vcpu_register_time_memory_area {
> >> >> >> >  typedef struct vcpu_register_time_memory_area
> >> >> >> > vcpu_register_time_memory_area_t;
> >> >> >> >
> DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
> >> >> >> >
> >> >> >> > +/*
> >> >> >> > + * Flags to tell Xen whether we need to do the SMAP check when
> >> >> updating
> >> >> >> > + * the secondary copy of the vcpu time when SMAP is enabled.
> Since
> >> the
> >> >> >> > + * memory location for the secondary copy of the vcpu time may be
> >> >> mapped
> >> >> >> > + * into userspace by guests intendedly, we let the guest to
> determine
> >> >> >> > + * whether the check is needed. The default behavior of hypevisor is
> >> >> >> > + * not doing the check.
> >> >> >> > + */
> >> >> >> > +#define VCPUOP_enable_smap_check_vcpu_time_memory_area
> >> 14
> >> >> >>
> >> >> >> I think the new op to be
> >> VCPUOP_register_vcpu_time_memory_area_smap,
> >> >> >> identical to VCPUOP_register_vcpu_time_memory_area apart from
> also
> >> >> >> setting the flag, would be more natural. But considering what I just
> >> wrote
> >> >> >> in the reply to Tim I guess we can expect a nun-user mapping to be
> >> >> >> presented here anyway, i.e. we wouldn't need to new operation at all.
> >> >> >
> >> >> > Do you mean since the user-paging is r/o, guest will pass a r/w kernel
> page
> >> >> > to
> >> >> > Xen for updating the system time. So we don't need to do the SMAP
> check
> >> >> > in this case?
> >> >>
> >> >> If the user page is r/o, it's VA obviously can't be used for updating by
> >> >> Xen. Hence the kernel has to provide a r/w mapped VA. That should be
> >> >> subject to SMAP checking (consistent with the runstate area handling),
> >> >> to make sure it's not a user accessible mapping.
> >> >
> >> > But there are two possible problems here:
> >> > 1. Is it possible that guest passes a user r/w page to update the system
> >> > time information?
> >>
> >> The guest kernel may pass a page that was originally mapped r/w-
> >> kernel, but the mapping later gets (say inadvertently) changed to
> >> r/w-user. That's why I think the SMAP checking ought to be done
> >> here.
> >
> > But in another case, as what we discussed before, if the guest intended to
> > pass a user r/w page to get the time information. Do we need to do the
> > SMAP checking for this?
> 
> The question is whether we should bother supporting such insecure
> kernels.
> 
> >> > 2. Even the user page is r/o, the kernel can still use it to update the
> >> > system time info when WP is disabled.
> >>
> >> The kernel can - for its own accesses to the page - do whatever
> >> it wants. It can't, however, control Xen's updating of it, and Xen
> >> won't do that with CR0.WP clear (leaving aside Aravindh's PV
> >> mem-access work, where this option is currently being discussed).
> >
> > Just like my comments above, my point is in some case, guest may pass a
> > user page (r/o or r/w) to get the time information, I doubt whether the SMAP
> > checking should be done for this?
> 
> If we want to allow kernels to expose the time data r/w to user mode,
> then yes, we would need to avoid the SMAP check. But as said above,
> I don't think helping kernels to be insecure is of any particular value.

Okay, thanks a lot for your comments! It makes sense. I got your point. I will do the
SMAP checking for both of the two cases (runstate area and system time information).

Thanks,
Feng

> 
> Jan

Re: [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

[Andrew Cooper]

On 25/07/14 09:31, Jan Beulich wrote:
>>>> On 25.07.14 at 10:03, <feng.wu@intel.com> wrote:
>>> -----Original Message-----
>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>> Sent: Friday, July 25, 2014 3:40 PM
>>> To: Wu, Feng
>>> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org; konrad.wilk@oracle.com;
>>> keir@xen.org; tim@xen.org 
>>> Subject: RE: [PATCH 2/2] x86/hvm: honor guest's option when updating
>>> secondary system time for guest
>>>
>>>>>> On 25.07.14 at 09:33, <feng.wu@intel.com> wrote:
>>>>> -----Original Message-----
>>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>>> Sent: Friday, July 25, 2014 3:26 PM
>>>>> To: Wu, Feng
>>>>> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org;
>>> konrad.wilk@oracle.com;
>>>>> keir@xen.org; tim@xen.org 
>>>>> Subject: RE: [PATCH 2/2] x86/hvm: honor guest's option when updating
>>>>> secondary system time for guest
>>>>>
>>>>>>>> On 25.07.14 at 06:30, <feng.wu@intel.com> wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>>>>> Sent: Wednesday, July 23, 2014 8:19 PM
>>>>>>> To: Wu, Feng
>>>>>>> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org;
>>>>> konrad.wilk@oracle.com;
>>>>>>> keir@xen.org; tim@xen.org 
>>>>>>> Subject: Re: [PATCH 2/2] x86/hvm: honor guest's option when updating
>>>>>>> secondary system time for guest
>>>>>>>
>>>>>>>>>> On 08.07.14 at 01:18, <feng.wu@intel.com> wrote:
>>>>>>>> --- a/xen/include/public/vcpu.h
>>>>>>>> +++ b/xen/include/public/vcpu.h
>>>>>>>> @@ -227,6 +227,16 @@ struct vcpu_register_time_memory_area {
>>>>>>>>  typedef struct vcpu_register_time_memory_area
>>>>>>>> vcpu_register_time_memory_area_t;
>>>>>>>>  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>>>>>>>>
>>>>>>>> +/*
>>>>>>>> + * Flags to tell Xen whether we need to do the SMAP check when
>>>>> updating
>>>>>>>> + * the secondary copy of the vcpu time when SMAP is enabled. Since
>>> the
>>>>>>>> + * memory location for the secondary copy of the vcpu time may be
>>>>> mapped
>>>>>>>> + * into userspace by guests intendedly, we let the guest to determine
>>>>>>>> + * whether the check is needed. The default behavior of hypevisor is
>>>>>>>> + * not doing the check.
>>>>>>>> + */
>>>>>>>> +#define VCPUOP_enable_smap_check_vcpu_time_memory_area
>>> 14
>>>>>>> I think the new op to be
>>> VCPUOP_register_vcpu_time_memory_area_smap,
>>>>>>> identical to VCPUOP_register_vcpu_time_memory_area apart from also
>>>>>>> setting the flag, would be more natural. But considering what I just
>>> wrote
>>>>>>> in the reply to Tim I guess we can expect a nun-user mapping to be
>>>>>>> presented here anyway, i.e. we wouldn't need to new operation at all.
>>>>>> Do you mean since the user-paging is r/o, guest will pass a r/w kernel page
>>>>>> to
>>>>>> Xen for updating the system time. So we don't need to do the SMAP check
>>>>>> in this case?
>>>>> If the user page is r/o, it's VA obviously can't be used for updating by
>>>>> Xen. Hence the kernel has to provide a r/w mapped VA. That should be
>>>>> subject to SMAP checking (consistent with the runstate area handling),
>>>>> to make sure it's not a user accessible mapping.
>>>> But there are two possible problems here:
>>>> 1. Is it possible that guest passes a user r/w page to update the system
>>>> time information?
>>> The guest kernel may pass a page that was originally mapped r/w-
>>> kernel, but the mapping later gets (say inadvertently) changed to
>>> r/w-user. That's why I think the SMAP checking ought to be done
>>> here.
>> But in another case, as what we discussed before, if the guest intended to
>> pass a user r/w page to get the time information. Do we need to do the
>> SMAP checking for this?
> The question is whether we should bother supporting such insecure
> kernels.

Thinking about this some more, how can we possibly know whether writing
to the nominated page is even safe?  At the point of update, there is no
knowledge about the current cr3 in use.

As far as I can see, the only safe actions Xen can take is refuse to
ever write to user mappings, and insist that any OS using this feature
provide a supervisor mapping to Xen (and guarantee it will never swap
the target of that mapping) and give user mappings of the same gpa to
any of its own userspace which wants access.  This then avoids any
concerns wrt SMAP.

~Andrew

Re: [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

[Jan Beulich]

>>> On 25.07.14 at 10:52, <andrew.cooper3@citrix.com> wrote:
> Thinking about this some more, how can we possibly know whether writing
> to the nominated page is even safe?  At the point of update, there is no
> knowledge about the current cr3 in use.

That's a problem the kernel has to be bothered by, but not the
hypervisor.

> As far as I can see, the only safe actions Xen can take is refuse to
> ever write to user mappings, and insist that any OS using this feature
> provide a supervisor mapping to Xen (and guarantee it will never swap
> the target of that mapping) and give user mappings of the same gpa to
> any of its own userspace which wants access.  This then avoids any
> concerns wrt SMAP.

No, user mappings can validly (and do) appear in the non-user
mode parts of the page tables.

Jan