* [PATCH v2 1/2] x86/hvm: Always do SMAP check when updating runstate_guest(v)
2001-01-08 0:10 [PATCH v2 0/2] x86/HVM: Properly handle SMAP check in certain cases Feng Wu
@ 2001-01-08 0:10 ` Feng Wu
2014-07-28 9:11 ` Jan Beulich
2001-01-08 0:10 ` [PATCH v2 2/2] x86/hvm: Always do SMAP check when updating secondary system time for guest Feng Wu
2014-07-28 5:47 ` [PATCH v2 0/2] x86/HVM: Properly handle SMAP check in certain cases Wu, Feng
2 siblings, 1 reply; 5+ messages in thread
From: Feng Wu @ 2001-01-08 0:10 UTC (permalink / raw)
To: xen-devel; +Cc: tim, Feng Wu, keir, jbeulich, linux
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v2 1/2] x86/hvm: Always do SMAP check when updating runstate_guest(v)
2001-01-08 0:10 ` [PATCH v2 1/2] x86/hvm: Always do SMAP check when updating runstate_guest(v) Feng Wu
@ 2014-07-28 9:11 ` Jan Beulich
0 siblings, 0 replies; 5+ messages in thread
From: Jan Beulich @ 2014-07-28 9:11 UTC (permalink / raw)
To: Feng Wu; +Cc: linux, tim, keir, xen-devel
>>> On 08.01.01 at 01:10, <feng.wu@intel.com> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1349,22 +1349,31 @@ static void paravirt_ctxt_switch_to(struct vcpu *v)
> }
>
> /* Update per-VCPU guest runstate shared memory area (if registered). */
> -bool_t update_runstate_area(const struct vcpu *v)
> +bool_t update_runstate_area(struct vcpu *v)
> {
> + bool_t rc;
> +
> if ( guest_handle_is_null(runstate_guest(v)) )
> return 1;
>
> + v->arch.smap_check_policy = SMAP_CHECK_ENABLED;
> +
> if ( has_32bit_shinfo(v->domain) )
> {
> struct compat_vcpu_runstate_info info;
>
> XLAT_vcpu_runstate_info(&info, &v->runstate);
> __copy_to_guest(v->runstate_guest.compat, &info, 1);
> - return 1;
> + rc = 1;
> + goto out;
> }
>
> - return __copy_to_guest(runstate_guest(v), &v->runstate, 1) !=
> + rc = __copy_to_guest(runstate_guest(v), &v->runstate, 1) !=
> sizeof(v->runstate);
> +
> +out:
Labels should be indented by at least one space. But even better
would be to handle this with "else" instead of "goto".
> + v->arch.smap_check_policy = SMAP_CHECK_HONOR_CPL_AC;
Please save the old value and restore it here rather than blindly
enforcing "honor" mode.
> + switch ( v->arch.smap_check_policy )
> + {
> + case SMAP_CHECK_HONOR_CPL_AC:
> + hvm_get_segment_register(v, x86_seg_ss, &seg);
> +
> + /*
> + * SMAP: kernel-mode data accesses from user-mode mappings
> + * should fault.
> + * A fault is considered as a SMAP violation if the following
> + * conditions come true:
> + * - X86_CR4_SMAP is set in CR4
> + * - A user page is accessed
> + * - CPL = 3 or X86_EFLAGS_AC is clear
> + * - Page fault in kernel mode
> + */
> + smap = hvm_smap_enabled(v) &&
> + ((seg.attr.fields.dpl == 3) ||
> + !(regs->eflags & X86_EFLAGS_AC));
> + break;
> + case SMAP_CHECK_ENABLED:
> + smap = hvm_smap_enabled(v);
> + break;
> + case SMAP_CHECK_DISABLED:
> + break;
> + default:
> + printk(XENLOG_INFO "Invalid SMAP check type!\n");
Isn't this more a BUG() or ASSERT(0), or perhaps - with the
SMAP_CHECK_DISABLED case dropped, BUG_ON(... !=
SMAP_CHECK_DISABLED) or ASSERT(... ==
SMAP_CHECK_DISABLED)?
> + /*
> + * The SMAP check policy when updating runstate_guest(v) and the
> + * secondary system time.
> + * SMAP_CHECK_HONOR_CPL_AC - honor the guest's CPL and AC
> + * SMAP_CHECK_ENABLED - enable the check
> + * SMAP_CHECK_DISABLED - disable the check
> + */
> + uint8_t smap_check_policy;
> } __cacheline_aligned;
>
> +#define SMAP_CHECK_HONOR_CPL_AC 0
> +#define SMAP_CHECK_ENABLED 1
> +#define SMAP_CHECK_DISABLED 2
I'd prefer this to be an enum.
Jan
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] x86/hvm: Always do SMAP check when updating secondary system time for guest
2001-01-08 0:10 [PATCH v2 0/2] x86/HVM: Properly handle SMAP check in certain cases Feng Wu
2001-01-08 0:10 ` [PATCH v2 1/2] x86/hvm: Always do SMAP check when updating runstate_guest(v) Feng Wu
@ 2001-01-08 0:10 ` Feng Wu
2014-07-28 5:47 ` [PATCH v2 0/2] x86/HVM: Properly handle SMAP check in certain cases Wu, Feng
2 siblings, 0 replies; 5+ messages in thread
From: Feng Wu @ 2001-01-08 0:10 UTC (permalink / raw)
To: xen-devel; +Cc: tim, Feng Wu, keir, jbeulich, linux
In this patch, we always do the SMAP check when updating secondary
system time for the guest when SMAP is enabled by it.
Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Feng Wu <feng.wu@intel.com>
---
xen/arch/x86/time.c | 9 ++++++++-
xen/include/asm-x86/domain.h | 2 +-
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index a4e1656..797f0cf 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -821,7 +821,7 @@ static void __update_vcpu_system_time(struct vcpu *v, int force)
v->arch.pv_vcpu.pending_system_time = _u;
}
-bool_t update_secondary_system_time(const struct vcpu *v,
+bool_t update_secondary_system_time(struct vcpu *v,
struct vcpu_time_info *u)
{
XEN_GUEST_HANDLE(vcpu_time_info_t) user_u = v->arch.time_info_guest;
@@ -829,9 +829,14 @@ bool_t update_secondary_system_time(const struct vcpu *v,
if ( guest_handle_is_null(user_u) )
return 1;
+ v->arch.smap_check_policy = SMAP_CHECK_ENABLED;
+
/* 1. Update userspace version. */
if ( __copy_field_to_guest(user_u, u, version) == sizeof(u->version) )
+ {
+ v->arch.smap_check_policy = SMAP_CHECK_HONOR_CPL_AC;
return 0;
+ }
wmb();
/* 2. Update all other userspace fields. */
__copy_to_guest(user_u, u, 1);
@@ -840,6 +845,8 @@ bool_t update_secondary_system_time(const struct vcpu *v,
u->version = version_update_end(u->version);
__copy_field_to_guest(user_u, u, version);
+ v->arch.smap_check_policy = SMAP_CHECK_HONOR_CPL_AC;
+
return 1;
}
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index d7cac4f..9440351 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -466,7 +466,7 @@ struct arch_vcpu
#define hvm_svm hvm_vcpu.u.svm
bool_t update_runstate_area(struct vcpu *);
-bool_t update_secondary_system_time(const struct vcpu *,
+bool_t update_secondary_system_time(struct vcpu *,
struct vcpu_time_info *);
void vcpu_show_execution_state(struct vcpu *);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v2 0/2] x86/HVM: Properly handle SMAP check in certain cases
2001-01-08 0:10 [PATCH v2 0/2] x86/HVM: Properly handle SMAP check in certain cases Feng Wu
2001-01-08 0:10 ` [PATCH v2 1/2] x86/hvm: Always do SMAP check when updating runstate_guest(v) Feng Wu
2001-01-08 0:10 ` [PATCH v2 2/2] x86/hvm: Always do SMAP check when updating secondary system time for guest Feng Wu
@ 2014-07-28 5:47 ` Wu, Feng
2 siblings, 0 replies; 5+ messages in thread
From: Wu, Feng @ 2014-07-28 5:47 UTC (permalink / raw)
To: xen-devel@lists.xen.org
Cc: tim@xen.org, keir@xen.org, jbeulich@suse.com,
linux@eikelenboom.it
Sorry, forget to add the change history. Adding it here:
V2: Remove ' VCPUOP_enable_smap_check_vcpu_time_memory_area' hypercall,
hence always do the SMAP checking for the secondary system time.
> -----Original Message-----
> From: Wu, Feng
> Sent: Monday, January 08, 2001 8:11 AM
> To: xen-devel@lists.xen.org
> Cc: keir@xen.org; jbeulich@suse.com; tim@xen.org; linux@eikelenboom.it;
> Wu, Feng
> Subject: [PATCH v2 0/2] x86/HVM: Properly handle SMAP check in certain cases
>
> This patch set fixs a issue found by Sander Eikelenboom. Here is the log
> when this issue occurs:
>
> (d2) Booting from Hard Disk...
> (d2) Booting from 0000:7c00
> (XEN) irq.c:380: Dom1 callback via changed to Direct Vector 0xf3
> (XEN) irq.c:380: Dom2 callback via changed to Direct Vector 0xf3
> (XEN) Segment register inaccessible for d1v0
> (XEN) (If you see this outside of debugging activity, please report to
> xen-devel@lists.xenproject.org)
>
> And here is the Xen call trace:
> (XEN) [<ffff82d0801dc9c5>] vmx_get_segment_register+0x4d/0x422
> (XEN) [<ffff82d0801f4415>] guest_walk_tables_3_levels+0x189/0x520
> (XEN) [<ffff82d0802204a8>] hap_p2m_ga_to_gfn_3_levels+0x158/0x2c2
> (XEN) [<ffff82d08022062e>] hap_gva_to_gfn_3_levels+0x1c/0x1e
> (XEN) [<ffff82d0801ec215>] paging_gva_to_gfn+0xb8/0xce
> (XEN) [<ffff82d0801ba88d>] __hvm_copy+0x87/0x354
> (XEN) [<ffff82d0801bac7c>] hvm_copy_to_guest_virt_nofault+0x1e/0x20
> (XEN) [<ffff82d0801bace5>] copy_to_user_hvm+0x67/0x87
> (XEN) [<ffff82d08016237c>] update_runstate_area+0x98/0xfb
> (XEN) [<ffff82d0801623f0>] _update_runstate_area+0x11/0x39
> (XEN) [<ffff82d0801634db>] context_switch+0x10c3/0x10fa
> (XEN) [<ffff82d080126a19>] schedule+0x5a8/0x5da
> (XEN) [<ffff82d0801297f9>] __do_softirq+0x81/0x8c
> (XEN) [<ffff82d080129852>] do_softirq+0x13/0x15
> (XEN) [<ffff82d08015f70a>] idle_loop+0x67/0x77
>
> We need get guest's SS register via hvm_get_segment_register()
> to do the SMAP checking, however, in these two cases, we cannot
> do it that way since it is between setting 'current' and reloading
> the VMCS context for it. As an alternative, here we treat these
> accesses as implicit supervisor mode access, hence SMAP checking is
> always need.
>
> Feng Wu (2):
> x86/hvm: Always do SMAP check when updating runstate_guest(v)
> x86/hvm: Always do SMAP check when updating secondary system time for
> guest
>
> xen/arch/x86/domain.c | 15 ++++++++++++---
> xen/arch/x86/mm/guest_walk.c | 41
> ++++++++++++++++++++++++++++-------------
> xen/arch/x86/time.c | 9 ++++++++-
> xen/include/asm-x86/domain.h | 17 +++++++++++++++--
> 4 files changed, 63 insertions(+), 19 deletions(-)
>
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 5+ messages in thread