diff mbox

[kvm-unit-tests] access: check SMEP on prefetch pte path

Message ID 1308899818-9027-1-git-send-email-wei.y.yang@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Yang, Wei Y June 24, 2011, 7:16 a.m. UTC 
This patch checks SMEP on prefetch pte path when
cr0.wp=1 and cr0.wp=0.

 Signed-off-by: Yang, Wei <wei.y.yang@intel.com>
 Signed-off-by: Li, Xin <xin.li@intel.com>
 Signed-off-by: Shan, Haitao <haitao.shan@intel.com>

---
 x86/access.c   |   92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 x86/cstart64.S |    1 +
 2 files changed, 90 insertions(+), 3 deletions(-)

Comments

Xiao Guangrong June 24, 2011, 9:11 a.m. UTC | #1 
On 06/24/2011 03:16 PM, Yang, Wei wrote:

> +void set_cr4_smep(int smep)
> +{
> +    unsigned long cr4 = read_cr4();
> +
> +    cr4 &= ~CR4_SMEP_MASK;
> +    if (smep)
> +	cr4 |= CR4_SMEP_MASK;
> +    write_cr4(cr4);
> +}
> +

It can work if the box does not support SMEP?

>  void set_efer_nx(int nx)
>  {
>      unsigned long long efer;
> @@ -176,7 +188,7 @@ void ac_test_init(ac_test_t *at, void *virt)
>  
>  int ac_test_bump_one(ac_test_t *at)
>  {
> -    for (int i = 0; i < NR_AC_FLAGS; ++i)
> +    for (int i = 0; i < NR_AC_FLAGS-1; ++i)

Why not test "SMEP" for all test case?

>  	if (!at->flags[i]) {
>  	    at->flags[i] = 1;
>  	    return 1;
> @@ -287,6 +299,9 @@ void ac_set_expected_status(ac_test_t *at)
>      if (at->flags[AC_PDE_PSE]) {
>  	if (at->flags[AC_ACCESS_WRITE] && !at->expected_fault)
>  	    at->expected_pde |= PT_DIRTY_MASK;
> +	if (at->flags[AC_ACCESS_FETCH] && at->flags[AC_PDE_USER]
> +	    && at->flags[AC_CPU_CR4_SMEP])
> +	    at->expected_fault = 1;
>  	goto no_pte;
>      }
>  
> @@ -306,7 +321,11 @@ void ac_set_expected_status(ac_test_t *at)
>  	&& (at->flags[AC_CPU_CR0_WP] || at->flags[AC_ACCESS_USER]))
>  	at->expected_fault = 1;
>  
> -    if (at->flags[AC_ACCESS_FETCH] && at->flags[AC_PTE_NX])
> +    if (at->flags[AC_ACCESS_FETCH]
> +	&& (at->flags[AC_PTE_NX]
> +	    || (at->flags[AC_CPU_CR4_SMEP]
> +		&& at->flags[AC_PDE_USER]
> +		&& at->flags[AC_PTE_USER])))
>  	at->expected_fault = 1;
>  
>      if (at->expected_fault)
> @@ -320,7 +339,7 @@ no_pte:
>  fault:
>      if (!at->expected_fault)
>          at->ignore_pde = 0;
> -    if (!at->flags[AC_CPU_EFER_NX])
> +    if (!at->flags[AC_CPU_EFER_NX] && !at->flags[AC_CPU_CR4_SMEP])
>          at->expected_error &= ~PFERR_FETCH_MASK;
>  }
>  

You check the AC_CPU_CR4_SMEP for all case, but only set the cr4 bit for
check_smep_on_prefetch_pte(), it is better to move set_cr4_smep to
ac_test_do_access()?

> @@ -645,6 +664,72 @@ err:
>      return 0;
>  }
>  
> +static int check_smep_on_prefetch_pte(ac_pool_t *pool)
> +{
> +	ac_test_t at1;
> +	int err_smep, err_prepare_notwp, err_smep_notwp;
> +	extern u64 ptl2[];
> +
> +	ac_test_init(&at1, (void *)(0x123406001000));
> +
> +	at1.flags[AC_PDE_PRESENT] = 1;
> +	at1.flags[AC_PTE_PRESENT] = 1;
> +	at1.flags[AC_PDE_USER] = 1;
> +	at1.flags[AC_PTE_USER] = 1;
> +	at1.flags[AC_PDE_ACCESSED] = 1;
> +	at1.flags[AC_PTE_ACCESSED] = 1;
> +	at1.flags[AC_ACCESS_FETCH] = 1;
> +	at1.flags[AC_CPU_CR4_SMEP] = 1;
> +	at1.flags[AC_CPU_CR0_WP] = 1;
> +	ac_test_setup_pte(&at1, pool);
> +	ptl2[2] -= 0x4;

Why is it needed? :-)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xiao Guangrong June 27, 2011, 6:02 a.m. UTC | #2 
On 06/25/2011 10:44 AM, Yang, Wei Y wrote:
> 
>>
>>> +void set_cr4_smep(int smep)
>>> +{
>>> +    unsigned long cr4 = read_cr4();
>>> +
>>> +    cr4 &= ~CR4_SMEP_MASK;
>>> +    if (smep)
>>> +	cr4 |= CR4_SMEP_MASK;
>>> +    write_cr4(cr4);
>>> +}
>>> +
>>
>> It can work if the box does not support SMEP?
> 
> It will report unhandled exception 13 in access.out which
> we count as errors either. Do we need verify it before
> running the smep test case?
> 

It can generate the wrong report that is not caused by the fault of KVM,
we'd better avoid it.

>>
>>>  void set_efer_nx(int nx)
>>>  {
>>>      unsigned long long efer;
>>> @@ -176,7 +188,7 @@ void ac_test_init(ac_test_t *at, void *virt)
>>>
>>>  int ac_test_bump_one(ac_test_t *at)
>>>  {
>>> -    for (int i = 0; i < NR_AC_FLAGS; ++i)
>>> +    for (int i = 0; i < NR_AC_FLAGS-1; ++i)
>>
>> Why not test "SMEP" for all test case?
> 
> It's actually the last question. :)
> The page where the current code stays is user page.
> If enabled SMEP, we must add some code to convert the current page
> into kernel page, or the page fault will occur again and again. And it seems
> no need to test SMEP for all test case. We just need fetch access which we
> do in a separate case.
> 

Oh, i see, thanks for your explanation, i think we'd better test SMEP for
all kinds of access, but i don't have strong opinion :-)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/x86/access.c b/x86/access.c
index 7c8b9a5..71f04e0 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -27,6 +27,7 @@  typedef unsigned long pt_element_t;
 #define PT_NX_MASK         ((pt_element_t)1 << 63)
 
 #define CR0_WP_MASK (1UL << 16)
+#define CR4_SMEP_MASK (1UL << 20)
 
 #define PFERR_PRESENT_MASK (1U << 0)
 #define PFERR_WRITE_MASK (1U << 1)
@@ -70,6 +71,7 @@  enum {
 
     AC_CPU_EFER_NX,
     AC_CPU_CR0_WP,
+    AC_CPU_CR4_SMEP,
 
     NR_AC_FLAGS
 };
@@ -140,6 +142,16 @@  void set_cr0_wp(int wp)
     write_cr0(cr0);
 }
 
+void set_cr4_smep(int smep)
+{
+    unsigned long cr4 = read_cr4();
+
+    cr4 &= ~CR4_SMEP_MASK;
+    if (smep)
+	cr4 |= CR4_SMEP_MASK;
+    write_cr4(cr4);
+}
+
 void set_efer_nx(int nx)
 {
     unsigned long long efer;
@@ -176,7 +188,7 @@  void ac_test_init(ac_test_t *at, void *virt)
 
 int ac_test_bump_one(ac_test_t *at)
 {
-    for (int i = 0; i < NR_AC_FLAGS; ++i)
+    for (int i = 0; i < NR_AC_FLAGS-1; ++i)
 	if (!at->flags[i]) {
 	    at->flags[i] = 1;
 	    return 1;
@@ -287,6 +299,9 @@  void ac_set_expected_status(ac_test_t *at)
     if (at->flags[AC_PDE_PSE]) {
 	if (at->flags[AC_ACCESS_WRITE] && !at->expected_fault)
 	    at->expected_pde |= PT_DIRTY_MASK;
+	if (at->flags[AC_ACCESS_FETCH] && at->flags[AC_PDE_USER]
+	    && at->flags[AC_CPU_CR4_SMEP])
+	    at->expected_fault = 1;
 	goto no_pte;
     }
 
@@ -306,7 +321,11 @@  void ac_set_expected_status(ac_test_t *at)
 	&& (at->flags[AC_CPU_CR0_WP] || at->flags[AC_ACCESS_USER]))
 	at->expected_fault = 1;
 
-    if (at->flags[AC_ACCESS_FETCH] && at->flags[AC_PTE_NX])
+    if (at->flags[AC_ACCESS_FETCH]
+	&& (at->flags[AC_PTE_NX]
+	    || (at->flags[AC_CPU_CR4_SMEP]
+		&& at->flags[AC_PDE_USER]
+		&& at->flags[AC_PTE_USER])))
 	at->expected_fault = 1;
 
     if (at->expected_fault)
@@ -320,7 +339,7 @@  no_pte:
 fault:
     if (!at->expected_fault)
         at->ignore_pde = 0;
-    if (!at->flags[AC_CPU_EFER_NX])
+    if (!at->flags[AC_CPU_EFER_NX] && !at->flags[AC_CPU_CR4_SMEP])
         at->expected_error &= ~PFERR_FETCH_MASK;
 }
 
@@ -645,6 +664,72 @@  err:
     return 0;
 }
 
+static int check_smep_on_prefetch_pte(ac_pool_t *pool)
+{
+	ac_test_t at1;
+	int err_smep, err_prepare_notwp, err_smep_notwp;
+	extern u64 ptl2[];
+
+	ac_test_init(&at1, (void *)(0x123406001000));
+
+	at1.flags[AC_PDE_PRESENT] = 1;
+	at1.flags[AC_PTE_PRESENT] = 1;
+	at1.flags[AC_PDE_USER] = 1;
+	at1.flags[AC_PTE_USER] = 1;
+	at1.flags[AC_PDE_ACCESSED] = 1;
+	at1.flags[AC_PTE_ACCESSED] = 1;
+	at1.flags[AC_ACCESS_FETCH] = 1;
+	at1.flags[AC_CPU_CR4_SMEP] = 1;
+	at1.flags[AC_CPU_CR0_WP] = 1;
+	ac_test_setup_pte(&at1, pool);
+	ptl2[2] -= 0x4;
+	set_cr4_smep(at1.flags[AC_CPU_CR4_SMEP]);
+	err_smep = ac_test_do_access(&at1);
+	if (!err_smep) {
+		printf("%s: check SMEP on prefetch pte path with wp"
+		       "fail\n", __FUNCTION__);
+		goto clean_up;
+	}
+
+	/*
+	 * Here we write the ro user page when
+	 * cr0.wp=0, then we execute it and SMEP
+	 * fault should happen.
+	 */
+	at1.flags[AC_ACCESS_WRITE] = 1;
+	at1.flags[AC_ACCESS_FETCH] = 0;
+	at1.flags[AC_CPU_CR0_WP] = 0;
+	ac_set_expected_status(&at1);
+	err_prepare_notwp = ac_test_do_access(&at1);
+	if (!err_prepare_notwp) {
+		printf("%s: SMEP prepare fail\n", __FUNCTION__);
+		goto clean_up;
+	}
+
+	at1.flags[AC_ACCESS_WRITE] = 0;
+	at1.flags[AC_ACCESS_FETCH] = 1;
+	ac_set_expected_status(&at1);
+	err_smep_notwp = ac_test_do_access(&at1);
+
+clean_up:
+	at1.flags[AC_CPU_CR4_SMEP] = 0;
+	set_cr4_smep(at1.flags[AC_CPU_CR4_SMEP]);
+	ptl2[2] += 0x4;
+
+	if (!err_smep || !err_prepare_notwp)
+		goto err;
+	if (!err_smep_notwp) {
+		printf("%s: check SMEP on prefetch pte path without wp"
+		       "fail\n", __FUNCTION__);
+			__FUNCTION__);
+		goto err;
+	}
+	return 1;
+
+err:
+	return 0;
+}
+
 int ac_test_exec(ac_test_t *at, ac_pool_t *pool)
 {
     int r;
@@ -662,6 +747,7 @@  const ac_test_fn ac_test_cases[] =
 {
 	corrupt_hugepage_triger,
 	check_pfec_on_prefetch_pte,
+	check_smep_on_prefetch_pte
 };
 
 int ac_test_run(void)
diff --git a/x86/cstart64.S b/x86/cstart64.S
index 71014d8..24df5f8 100644
--- a/x86/cstart64.S
+++ b/x86/cstart64.S
@@ -21,6 +21,7 @@  ring0stacktop:
 .data
 
 .align 4096
+.globl ptl2
 ptl2:
 i = 0
 	.rept 512 * 4