[PATCH] x86/cpufeatures: Expose UMIP to HVM guest

[PATCH] x86/cpufeatures: Expose UMIP to HVM guest

[Boqun Feng (Intel)]

User-Mode Instruction Prevention (UMIP) is a security feature present in
new Intel Processors. With this feature, when the UMIP bit in CR4 set,
the following instructions cannot be executed if CPL > 0: SGDT, SIDT,
SLDT, SMSW, and STR. An attempt at such execution causes a general-
protection exception (#GP).

This patch simply adds necessary definitions to expose this feature to
hvm guests.

Signed-off-by: Boqun Feng (Intel) <boqun.feng@gmail.com>
Cc: Jan Beulich <jbeulich@suse.com>
---
This patch is basically based on Jan Beulich's patch:

	https://lists.xenproject.org/archives/html/xen-devel/2016-12/msg00552.html

I simply picked up exposing bits in that patch and ran some tests on our
simics environment. If any SoB adjustion is needed, please let me know.

Another patch for XTF is sent out along with this patch, as that patch add a
new test for UMIP.

 xen/arch/x86/hvm/hvm.c                      | 1 +
 xen/include/public/arch-x86/cpufeatureset.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 814538574725..1284460cda8e 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -960,6 +960,7 @@ unsigned long hvm_cr4_guest_valid_bits(const struct vcpu *v, bool restore)
             (p->basic.xsave   ? X86_CR4_OSXSAVE           : 0) |
             (p->feat.smep     ? X86_CR4_SMEP              : 0) |
             (p->feat.smap     ? X86_CR4_SMAP              : 0) |
+            (p->feat.umip     ? X86_CR4_UMIP              : 0) |
             (p->feat.pku      ? X86_CR4_PKE               : 0));
 }
 
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index 97dd3534c573..0ee3ea350fc9 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -225,6 +225,7 @@ XEN_CPUFEATURE(AVX512VL,      5*32+31) /*A  AVX-512 Vector Length Extensions */
 /* Intel-defined CPU features, CPUID level 0x00000007:0.ecx, word 6 */
 XEN_CPUFEATURE(PREFETCHWT1,   6*32+ 0) /*A  PREFETCHWT1 instruction */
 XEN_CPUFEATURE(AVX512VBMI,    6*32+ 1) /*A  AVX-512 Vector Byte Manipulation Instrs */
+XEN_CPUFEATURE(UMIP,          6*32+ 2) /*S  User Mode Instruction Prevention */
 XEN_CPUFEATURE(PKU,           6*32+ 3) /*H  Protection Keys for Userspace */
 XEN_CPUFEATURE(OSPKE,         6*32+ 4) /*!  OS Protection Keys Enable */
 XEN_CPUFEATURE(AVX512_VPOPCNTDQ, 6*32+14) /*A  POPCNT for vectors of DW/QW */
-- 
2.13.3


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH XTF] Functional: Add a UMIP test

[Boqun Feng (Intel)]

Add a "umip" test for the User-Model Instruction Prevention. The test
simply tries to run sgdt/sidt/sldt/str/smsw in guest user-mode with
CR4_UMIP = 1.

Signed-off-by: Boqun Feng (Intel) <boqun.feng@gmail.com>
---
 docs/all-tests.dox  |   2 +
 tests/umip/Makefile |   9 ++++
 tests/umip/main.c   | 120 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+)
 create mode 100644 tests/umip/Makefile
 create mode 100644 tests/umip/main.c

diff --git a/docs/all-tests.dox b/docs/all-tests.dox
index 01a7a572f472..ec5328b50189 100644
--- a/docs/all-tests.dox
+++ b/docs/all-tests.dox
@@ -109,4 +109,6 @@ guest breakout.
 @section index-in-development In Development
 
 @subpage test-vvmx - Nested VT-x tests.
+
+@subpage test-umip - User-Mode Instruction Prevention
 */
diff --git a/tests/umip/Makefile b/tests/umip/Makefile
new file mode 100644
index 000000000000..0248c8b247a0
--- /dev/null
+++ b/tests/umip/Makefile
@@ -0,0 +1,9 @@
+include $(ROOT)/build/common.mk
+
+NAME      := umip
+CATEGORY  := functional
+TEST-ENVS := hvm32 hvm64
+
+obj-perenv += main.o
+
+include $(ROOT)/build/gen.mk
diff --git a/tests/umip/main.c b/tests/umip/main.c
new file mode 100644
index 000000000000..27b7d44f4b98
--- /dev/null
+++ b/tests/umip/main.c
@@ -0,0 +1,120 @@
+/**
+ * @file tests/umip/main.c
+ * @ref test-umip
+ *
+ * @page test-umip umip
+ *
+ * @todo Docs for test-umip
+ *
+ * @see tests/umip/main.c
+ */
+#include <xtf.h>
+#include <arch/processor.h>
+
+const char test_title[] = "User-Mode Instruction Prevention Test";
+bool test_wants_user_mapping = true;
+
+unsigned long umip_sgdt(void)
+{
+    unsigned long fault = 0;
+    unsigned long tmp;
+
+    asm volatile("1: sgdt %[tmp]; 2:"
+                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
+		 : "+D" (fault), [tmp] "=m" (tmp)
+		 :);
+
+    return fault;
+}
+
+unsigned long umip_sldt(void)
+{
+    unsigned long fault = 0;
+    unsigned long tmp;
+
+    asm volatile("1: sldt %[tmp]; 2:"
+                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
+		 : "+D" (fault), [tmp] "=m" (tmp)
+		 :);
+
+    return fault;
+}
+
+unsigned long umip_sidt(void)
+{
+    unsigned long fault = 0;
+    unsigned long tmp;
+
+    asm volatile("1: sidt %[tmp]; 2:"
+                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
+		 : "+D" (fault), [tmp] "=m" (tmp)
+		 :);
+
+    return fault;
+}
+
+unsigned long umip_str(void)
+{
+    unsigned long fault = 0;
+    unsigned long tmp;
+
+    asm volatile("1: str %[tmp]; 2:"
+                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
+		 : "+D" (fault), [tmp] "=m" (tmp)
+		 :);
+
+    return fault;
+}
+
+unsigned long umip_smsw(void)
+{
+    unsigned long fault = 0;
+    unsigned long tmp;
+
+    asm volatile("1: smsw %[tmp]; 2:"
+                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
+		 : "+D" (fault), [tmp] "=m" (tmp)
+		 :);
+
+    return fault;
+}
+
+void test_main(void)
+{
+    unsigned long exp;
+    unsigned long cr4 = read_cr4();
+
+    if ( !cpu_has_umip )
+        xtf_failure("Fail: UMIP is not supported\n");
+
+    write_cr4(cr4 | X86_CR4_UMIP);
+
+    exp = EXINFO_SYM(GP, 0);
+
+    if ( exec_user(umip_sgdt) != exp )
+        xtf_failure("Fail: sgdt didn't trigger #GP\n");
+
+    if ( exec_user(umip_sldt) != exp )
+        xtf_failure("Fail: sldt didn't trigger #GP\n");
+
+    if ( exec_user(umip_sidt) != exp )
+        xtf_failure("Fail: sidt didn't trigger #GP\n");
+
+    if ( exec_user(umip_str) != exp )
+        xtf_failure("Fail: str didn't trigger #GP\n");
+
+    if ( exec_user(umip_smsw) != exp )
+        xtf_failure("Fail: smsw didn't trigger #GP\n");
+
+    xtf_success(NULL);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.13.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH XTF] Functional: Add a UMIP test

[Andrew Cooper]

On 20/07/17 06:29, Boqun Feng (Intel) wrote:
> Add a "umip" test for the User-Model Instruction Prevention. The test
> simply tries to run sgdt/sidt/sldt/str/smsw in guest user-mode with
> CR4_UMIP = 1.
>
> Signed-off-by: Boqun Feng (Intel) <boqun.feng@gmail.com>

Thankyou very much for providing a test.

As a general remark, how have you found XTF to use?

> ---
>   docs/all-tests.dox  |   2 +
>   tests/umip/Makefile |   9 ++++
>   tests/umip/main.c   | 120 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 131 insertions(+)
>   create mode 100644 tests/umip/Makefile
>   create mode 100644 tests/umip/main.c
>
> diff --git a/docs/all-tests.dox b/docs/all-tests.dox
> index 01a7a572f472..ec5328b50189 100644
> --- a/docs/all-tests.dox
> +++ b/docs/all-tests.dox
> @@ -109,4 +109,6 @@ guest breakout.
>   @section index-in-development In Development
>   
>   @subpage test-vvmx - Nested VT-x tests.
> +
> +@subpage test-umip - User-Mode Instruction Prevention
>   */
> diff --git a/tests/umip/Makefile b/tests/umip/Makefile
> new file mode 100644
> index 000000000000..0248c8b247a0
> --- /dev/null
> +++ b/tests/umip/Makefile
> @@ -0,0 +1,9 @@
> +include $(ROOT)/build/common.mk
> +
> +NAME      := umip
> +CATEGORY  := functional
> +TEST-ENVS := hvm32 hvm64
> +
> +obj-perenv += main.o
> +
> +include $(ROOT)/build/gen.mk
> diff --git a/tests/umip/main.c b/tests/umip/main.c
> new file mode 100644
> index 000000000000..27b7d44f4b98
> --- /dev/null
> +++ b/tests/umip/main.c
> @@ -0,0 +1,120 @@
> +/**
> + * @file tests/umip/main.c
> + * @ref test-umip
> + *
> + * @page test-umip umip
> + *
> + * @todo Docs for test-umip
> + *
> + * @see tests/umip/main.c
> + */
> +#include <xtf.h>
> +#include <arch/processor.h>
> +
> +const char test_title[] = "User-Mode Instruction Prevention Test";
> +bool test_wants_user_mapping = true;
> +
> +unsigned long umip_sgdt(void)

The prevailing naming would be stub_sgdt(), and it can be static. For 
reasons I will explain later, it should take an unsigned long fep parameter.

> +{
> +    unsigned long fault = 0;

exinfo_t fault = 0;

> +    unsigned long tmp;

sgdt writes out two bytes more than an unsigned long, so this will 
corrupt the stack.  If you follow the sgdt() example in lib.h, turning 
this to "desc_ptr tmp" ought to suffice.

> +
> +    asm volatile("1: sgdt %[tmp]; 2:"
> +                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
> +		 : "+D" (fault), [tmp] "=m" (tmp)
> +		 :);

The extra colon isn't necessary.  Did you perhaps originally have memory 
clobbers here?

> +
> +    return fault;
> +}
> +
> +unsigned long umip_sldt(void)
> +{
> +    unsigned long fault = 0;
> +    unsigned long tmp;
> +
> +    asm volatile("1: sldt %[tmp]; 2:"
> +                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
> +		 : "+D" (fault), [tmp] "=m" (tmp)

str and sldt are deceptively hard to encode in their memory operand 
form, as the operand is r32/m16.  I couldn't find a way of doing it 
which didn't leave most of tmp uninitialised on the stack, or without 
gcc/clang trying to use prefixes to get the behaviour described.  I 
recommend switching to the register form which is far easier to work with.

> +		 :);
> +
> +    return fault;
> +}
> +
> +unsigned long umip_sidt(void)
> +{
> +    unsigned long fault = 0;
> +    unsigned long tmp;
> +
> +    asm volatile("1: sidt %[tmp]; 2:"
> +                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
> +		 : "+D" (fault), [tmp] "=m" (tmp)
> +		 :);
> +
> +    return fault;
> +}
> +
> +unsigned long umip_str(void)
> +{
> +    unsigned long fault = 0;
> +    unsigned long tmp;
> +
> +    asm volatile("1: str %[tmp]; 2:"
> +                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
> +		 : "+D" (fault), [tmp] "=m" (tmp)
> +		 :);
> +
> +    return fault;
> +}
> +
> +unsigned long umip_smsw(void)
> +{
> +    unsigned long fault = 0;
> +    unsigned long tmp;
> +
> +    asm volatile("1: smsw %[tmp]; 2:"
> +                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
> +		 : "+D" (fault), [tmp] "=m" (tmp)
> +		 :);
> +
> +    return fault;
> +}
> +
> +void test_main(void)
> +{
> +    unsigned long exp;
> +    unsigned long cr4 = read_cr4();

This is all good.  However, it is insufficient to properly test the UMIP 
behaviour.  Please look at the cpuid-faulting to see how I structured 
things.

In particular, you should:

1) Test the regular behaviour of the instructions.
2) Search for UMIP, skipping if it isn't available.
3) Enable UMIP.
4) Test the instructions again, this time checking for #GP in userspace.
5) Disable UMIP.
6) Check again for regular behaviour.

This way, you also check that turning it off works as well as turning it on.

In addition, each test needs to check more than just the block of tests 
below.

1) The tests should run the instructions natively, and forced through 
the instruction emulator.  See the FPU Exception Emulation test which is 
along the same lines.  One thing to be aware of though is that in older 
versions of Xen, the s??? instructions weren't implemented in the 
instruction emulator, so the test should tolerate and skip if it gets 
#UD back.

2) You need to check supervisor behaviour as well as user behaviour, and 
in particular, that supervisor instructions still work irrespective of 
UMIP.  Unfortunately, I don't have a good example to point you at 
(because none of them have been cleaned up and committed yet).  
Therefore, I've tried mocking something suitable up rather than leaving 
you in the dark.  This is entirely untested, but should be along the 
right lines:

static const struct stub {
     unsigned long (*fn)(unsigned long);
     const char *name;
} stubs[] = {
     { stub_sgdt, "SGDT" },
     { stub_sidt, "SIDT" },
     { stub_sldt, "SLDT" },
     { stub_str,  "STR" },
     { stub_smsw, "SMSW" },
};

void test_umip(bool umip_active, bool force)
{
     unsigned int i;
     bool user;

     for ( user = false; ; user = true )
{
         exinfo_t exp = user && umip_active ? EXINFO_SYM(GP, 0) : 0;

         for ( i = 0; i < ARRAY_SIZE(stubs); ++i )
{
             const struct stub *s = &stubs[i];
             exinfo_t res;

             res = user ? exec_user_param(s->fn, force) : s->fn(force);

/*
              * Tolerate the instruction emulator not understanding these
              * instructions in older releases of Xen.
*/
             if ( force && res == EXINFO_SYM(UD, 0) )
{
                 static bool once;

                 if ( !once )
{
                     xtf_skip("Skip: Emulator doesn't implement %s\n",
s->name);
                     once = true;
}
continue;
}

             if ( res != exp )
{
                 char expstr[16], gotstr[16];

                 x86_decode_exinfo(expstr, ARRAY_SIZE(expstr), exp);
                 x86_decode_exinfo(gotstr, ARRAY_SIZE(gotstr), res);

                 xtf_failure("Fail: %s %s\n"
                             "  expected %s\n"
                             "       got %s\n",
                             user ? "user" : "supervisor", s->name,
                             expstr, gotstr);
}
}

         if ( user )
break;
}
}

Thanks,

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH XTF] Functional: Add a UMIP test

[Boqun Feng]

On Thu, Jul 20, 2017 at 10:38:59AM +0100, Andrew Cooper wrote:
> On 20/07/17 06:29, Boqun Feng (Intel) wrote:
> > Add a "umip" test for the User-Model Instruction Prevention. The test
> > simply tries to run sgdt/sidt/sldt/str/smsw in guest user-mode with
> > CR4_UMIP = 1.
> > 
> > Signed-off-by: Boqun Feng (Intel) <boqun.feng@gmail.com>
> 
> Thankyou very much for providing a test.
> 
> As a general remark, how have you found XTF to use?
> 

Great tool! Especially when you need to run Xen in a simulated
environment like simics and want to test something, bringing up even a
simple Linux domainU would be a lot of pain. ;-) While XTF just works
like a charm and it's easy to write a test case, though according to
your comments I'm now very good at it now ;-)

[...]
> > +
> > +unsigned long umip_sgdt(void)
> 
> The prevailing naming would be stub_sgdt(), and it can be static. For
> reasons I will explain later, it should take an unsigned long fep parameter.
> 
> > +{
> > +    unsigned long fault = 0;
> 
> exinfo_t fault = 0;
> 
> > +    unsigned long tmp;
> 
> sgdt writes out two bytes more than an unsigned long, so this will corrupt
> the stack.  If you follow the sgdt() example in lib.h, turning this to
> "desc_ptr tmp" ought to suffice.
> 
> > +
> > +    asm volatile("1: sgdt %[tmp]; 2:"
> > +                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
> > +		 : "+D" (fault), [tmp] "=m" (tmp)
> > +		 :);
> 
> The extra colon isn't necessary.  Did you perhaps originally have memory
> clobbers here?
> 

You're right, this could be removed.

> > +
> > +    return fault;
> > +}
> > +
> > +unsigned long umip_sldt(void)
> > +{
> > +    unsigned long fault = 0;
> > +    unsigned long tmp;
> > +
> > +    asm volatile("1: sldt %[tmp]; 2:"
> > +                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
> > +		 : "+D" (fault), [tmp] "=m" (tmp)
> 
> str and sldt are deceptively hard to encode in their memory operand form, as
> the operand is r32/m16.  I couldn't find a way of doing it which didn't
> leave most of tmp uninitialised on the stack, or without gcc/clang trying to
> use prefixes to get the behaviour described.  I recommend switching to the
> register form which is far easier to work with.
> 

Clearly, I haven't learned those instruction well. Will read the SDM
carefully and follow your suggestions, thanks!

> > +		 :);
> > +
> > +    return fault;
> > +}
> > +
> > +unsigned long umip_sidt(void)
> > +{
> > +    unsigned long fault = 0;
> > +    unsigned long tmp;
> > +
> > +    asm volatile("1: sidt %[tmp]; 2:"
> > +                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
> > +		 : "+D" (fault), [tmp] "=m" (tmp)
> > +		 :);
> > +
> > +    return fault;
> > +}
> > +
> > +unsigned long umip_str(void)
> > +{
> > +    unsigned long fault = 0;
> > +    unsigned long tmp;
> > +
> > +    asm volatile("1: str %[tmp]; 2:"
> > +                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
> > +		 : "+D" (fault), [tmp] "=m" (tmp)
> > +		 :);
> > +
> > +    return fault;
> > +}
> > +
> > +unsigned long umip_smsw(void)
> > +{
> > +    unsigned long fault = 0;
> > +    unsigned long tmp;
> > +
> > +    asm volatile("1: smsw %[tmp]; 2:"
> > +                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
> > +		 : "+D" (fault), [tmp] "=m" (tmp)
> > +		 :);
> > +
> > +    return fault;
> > +}
> > +
> > +void test_main(void)
> > +{
> > +    unsigned long exp;
> > +    unsigned long cr4 = read_cr4();
> 
> This is all good.  However, it is insufficient to properly test the UMIP
> behaviour.  Please look at the cpuid-faulting to see how I structured
> things.
> 
> In particular, you should:
> 
> 1) Test the regular behaviour of the instructions.
> 2) Search for UMIP, skipping if it isn't available.
> 3) Enable UMIP.

Maybe I also need to provide a write_cr4_safe() similar as wrmsr_safe(),
in case that cpuid indicates UMIP supported while UMIP CR4 bit is not
allowed to set, which means a bug?

> 4) Test the instructions again, this time checking for #GP in userspace.
> 5) Disable UMIP.
> 6) Check again for regular behaviour.
> 
> This way, you also check that turning it off works as well as turning it on.
> 
> In addition, each test needs to check more than just the block of tests
> below.
> 
> 1) The tests should run the instructions natively, and forced through the
> instruction emulator.  See the FPU Exception Emulation test which is along
> the same lines.  One thing to be aware of though is that in older versions
> of Xen, the s??? instructions weren't implemented in the instruction
> emulator, so the test should tolerate and skip if it gets #UD back.
> 

Rogar that.

> 2) You need to check supervisor behaviour as well as user behaviour, and in
> particular, that supervisor instructions still work irrespective of UMIP.
> Unfortunately, I don't have a good example to point you at (because none of
> them have been cleaned up and committed yet).  Therefore, I've tried mocking
> something suitable up rather than leaving you in the dark.  This is entirely
> untested, but should be along the right lines:
> 

Thank you! This is a great example, very helpful. I will follow your
guide and send out a v2(probably in next week).

Thanks again and Best Regards,
Boqun

> static const struct stub {
>     unsigned long (*fn)(unsigned long);
>     const char *name;
> } stubs[] = {
>     { stub_sgdt, "SGDT" },
>     { stub_sidt, "SIDT" },
>     { stub_sldt, "SLDT" },
>     { stub_str,  "STR" },
>     { stub_smsw, "SMSW" },
> };
> 
> void test_umip(bool umip_active, bool force)
> {
>     unsigned int i;
>     bool user;
> 
>     for ( user = false; ; user = true )
> {
>         exinfo_t exp = user && umip_active ? EXINFO_SYM(GP, 0) : 0;
> 
>         for ( i = 0; i < ARRAY_SIZE(stubs); ++i )
> {
>             const struct stub *s = &stubs[i];
>             exinfo_t res;
> 
>             res = user ? exec_user_param(s->fn, force) : s->fn(force);
> 
> /*
>              * Tolerate the instruction emulator not understanding these
>              * instructions in older releases of Xen.
> */
>             if ( force && res == EXINFO_SYM(UD, 0) )
> {
>                 static bool once;
> 
>                 if ( !once )
> {
>                     xtf_skip("Skip: Emulator doesn't implement %s\n",
> s->name);
>                     once = true;
> }
> continue;
> }
> 
>             if ( res != exp )
> {
>                 char expstr[16], gotstr[16];
> 
>                 x86_decode_exinfo(expstr, ARRAY_SIZE(expstr), exp);
>                 x86_decode_exinfo(gotstr, ARRAY_SIZE(gotstr), res);
> 
>                 xtf_failure("Fail: %s %s\n"
>                             "  expected %s\n"
>                             "       got %s\n",
>                             user ? "user" : "supervisor", s->name,
>                             expstr, gotstr);
> }
> }
> 
>         if ( user )
> break;
> }
> }
> 
> Thanks,
> 
> ~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH XTF] Functional: Add a UMIP test

[Andrew Cooper]

On 21/07/17 02:42, Boqun Feng wrote:
> On Thu, Jul 20, 2017 at 10:38:59AM +0100, Andrew Cooper wrote:
>> On 20/07/17 06:29, Boqun Feng (Intel) wrote:
>>> Add a "umip" test for the User-Model Instruction Prevention. The test
>>> simply tries to run sgdt/sidt/sldt/str/smsw in guest user-mode with
>>> CR4_UMIP = 1.
>>>
>>> Signed-off-by: Boqun Feng (Intel) <boqun.feng@gmail.com>
>> Thankyou very much for providing a test.
>>
>> As a general remark, how have you found XTF to use?
>>
> Great tool! Especially when you need to run Xen in a simulated
> environment like simics and want to test something, bringing up even a
> simple Linux domainU would be a lot of pain. ;-) While XTF just works
> like a charm and it's easy to write a test case, though according to
> your comments I'm now very good at it now ;-)

I'm glad to hear this.

>
>>> +void test_main(void)
>>> +{
>>> +    unsigned long exp;
>>> +    unsigned long cr4 = read_cr4();
>> This is all good.  However, it is insufficient to properly test the UMIP
>> behaviour.  Please look at the cpuid-faulting to see how I structured
>> things.
>>
>> In particular, you should:
>>
>> 1) Test the regular behaviour of the instructions.
>> 2) Search for UMIP, skipping if it isn't available.
>> 3) Enable UMIP.
> Maybe I also need to provide a write_cr4_safe() similar as wrmsr_safe(),
> in case that cpuid indicates UMIP supported while UMIP CR4 bit is not
> allowed to set, which means a bug?

Yes.  You are entirely correct.  Feel free to put write_cr4_safe() in 
lib.h along with the other *_safe() variants.

>
>> 4) Test the instructions again, this time checking for #GP in userspace.
>> 5) Disable UMIP.
>> 6) Check again for regular behaviour.
>>
>> This way, you also check that turning it off works as well as turning it on.
>>
>> In addition, each test needs to check more than just the block of tests
>> below.
>>
>> 1) The tests should run the instructions natively, and forced through the
>> instruction emulator.  See the FPU Exception Emulation test which is along
>> the same lines.  One thing to be aware of though is that in older versions
>> of Xen, the s??? instructions weren't implemented in the instruction
>> emulator, so the test should tolerate and skip if it gets #UD back.
>>
> Rogar that.

:)

Roger.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86/cpufeatures: Expose UMIP to HVM guest

[Jan Beulich]

>>> On 20.07.17 at 07:29, <boqun.feng@gmail.com> wrote:
> User-Mode Instruction Prevention (UMIP) is a security feature present in
> new Intel Processors. With this feature, when the UMIP bit in CR4 set,
> the following instructions cannot be executed if CPL > 0: SGDT, SIDT,
> SLDT, SMSW, and STR. An attempt at such execution causes a general-
> protection exception (#GP).
> 
> This patch simply adds necessary definitions to expose this feature to
> hvm guests.
> 
> Signed-off-by: Boqun Feng (Intel) <boqun.feng@gmail.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH XTF v2] Functional: Add a UMIP test

[Boqun Feng (Intel)]

Add a "umip" test for the User-Model Instruction Prevention. The test
simply tries to run sgdt/sidt/sldt/str/smsw in guest user-mode with
CR4_UMIP = 1.

Signed-off-by: Boqun Feng (Intel) <boqun.feng@gmail.com>
---
v1 --> v2:
	* add a new write_cr4_safe()
	* use %pe for exception print
	* refactor the code based on Andrew's guide and advice

Test results:

* With UMIP patch:
** boot with hvm_fep: SUCCESS
** boot without hvm_fep: SKIP, due to "FEP support not detected.."

* Without UMIP patch:
** boot with hvm_fep: SKIP, due to "UMIP is not supported.."
** boot without hvm_fep: SKIP, due to "UMIP is not supported.."

* With UMIP cpuid exposed but CR4 invalid:
** boot with hvm_fep: FAILURE, due to "Fail: Unable to activate UMIP.."
** boot without hvm_fep: FAILURE, due to "Fail: Unable to activate UMIP.."


 arch/x86/include/arch/lib.h |  13 +++
 docs/all-tests.dox          |   2 +
 tests/umip/Makefile         |   9 ++
 tests/umip/main.c           | 219 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 243 insertions(+)
 create mode 100644 tests/umip/Makefile
 create mode 100644 tests/umip/main.c

diff --git a/arch/x86/include/arch/lib.h b/arch/x86/include/arch/lib.h
index f608af9996f0..4f0d85290cf0 100644
--- a/arch/x86/include/arch/lib.h
+++ b/arch/x86/include/arch/lib.h
@@ -340,6 +340,19 @@ static inline void write_cr4(unsigned long cr4)
     asm volatile ("mov %0, %%cr4" :: "r" (cr4));
 }
 
+static inline bool write_cr4_safe(unsigned long cr4)
+{
+    exinfo_t fault = 0;
+
+    asm volatile ("1: mov %1, %%cr4; 2:"
+                  _ASM_EXTABLE_HANDLER(1b, 2b, ex_record_fault_edi)
+                  : "+D" (fault)
+                  : "r" (cr4),
+                    "X" (ex_record_fault_edi));
+
+    return !fault;
+}
+
 static inline void write_cr8(unsigned long cr8)
 {
     asm volatile ("mov %0, %%cr8" :: "r" (cr8));
diff --git a/docs/all-tests.dox b/docs/all-tests.dox
index c1b163a926cb..ef011007cf68 100644
--- a/docs/all-tests.dox
+++ b/docs/all-tests.dox
@@ -111,4 +111,6 @@ guest breakout.
 @section index-in-development In Development
 
 @subpage test-vvmx - Nested VT-x tests.
+
+@subpage test-umip - User-Mode Instruction Prevention
 */
diff --git a/tests/umip/Makefile b/tests/umip/Makefile
new file mode 100644
index 000000000000..0248c8b247a0
--- /dev/null
+++ b/tests/umip/Makefile
@@ -0,0 +1,9 @@
+include $(ROOT)/build/common.mk
+
+NAME      := umip
+CATEGORY  := functional
+TEST-ENVS := hvm32 hvm64
+
+obj-perenv += main.o
+
+include $(ROOT)/build/gen.mk
diff --git a/tests/umip/main.c b/tests/umip/main.c
new file mode 100644
index 000000000000..fcaba4e34570
--- /dev/null
+++ b/tests/umip/main.c
@@ -0,0 +1,219 @@
+/**
+ * @file tests/umip/main.c
+ * @ref test-umip
+ *
+ * @page test-umip umip
+ *
+ * @todo Docs for test-umip
+ *
+ * @see tests/umip/main.c
+ */
+#include <xtf.h>
+#include <arch/exinfo.h>
+#include <arch/processor.h>
+
+const char test_title[] = "User-Mode Instruction Prevention Test";
+bool test_wants_user_mappings = true;
+
+static unsigned long stub_sgdt(unsigned long force)
+{
+    exinfo_t fault = 0;
+    desc_ptr tmp;
+
+    asm volatile("test %[fep], %[fep];"
+                 "jz 1f;"
+                 _ASM_XEN_FEP
+                 "1: sgdt %[tmp]; 2:"
+                _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
+		: "+D" (fault), [tmp] "=m" (tmp)
+                : [fep] "q" (force),
+                  "X" (ex_record_fault_edi));
+
+    return fault;
+}
+static unsigned long stub_sidt(unsigned long force)
+{
+    exinfo_t fault = 0;
+    desc_ptr tmp;
+
+    asm volatile("test %[fep], %[fep];"
+                 "jz 1f;"
+                 _ASM_XEN_FEP
+                 "1: sidt %[tmp]; 2:"
+                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
+		 : "+D" (fault), [tmp] "=m" (tmp)
+                 : [fep] "q" (force),
+                  "X" (ex_record_fault_edi));
+
+    return fault;
+}
+
+static unsigned long stub_sldt(unsigned long force)
+{
+    exinfo_t fault = 0;
+    unsigned int tmp;
+
+    asm volatile("test %[fep], %[fep];"
+                 "jz 1f;"
+                 _ASM_XEN_FEP
+                 "1: sldt %[tmp]; 2:"
+                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
+                 : "+D" (fault), [tmp] "=r" (tmp)
+                 : [fep] "q" (force),
+                  "X" (ex_record_fault_edi));
+
+    return fault;
+}
+
+static unsigned long stub_str(unsigned long force)
+{
+    exinfo_t fault = 0;
+    unsigned int tmp;
+
+    asm volatile("test %[fep], %[fep];"
+                 "jz 1f;"
+                 _ASM_XEN_FEP
+                 "1: str %[tmp]; 2:"
+                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
+                 : "+D" (fault), [tmp] "=r" (tmp)
+                 : [fep] "q" (force),
+                  "X" (ex_record_fault_edi));
+
+    return fault;
+}
+
+static unsigned long stub_smsw(unsigned long force)
+{
+    exinfo_t fault = 0;
+    unsigned int tmp;
+
+    asm volatile("test %[fep], %[fep];"
+                 "jz 1f;"
+                 _ASM_XEN_FEP
+                 "1: smsw %[tmp]; 2:"
+                 _ASM_EXTABLE_HANDLER(1b,2b, ex_record_fault_edi)
+                 : "+D" (fault), [tmp] "=r" (tmp)
+                 : [fep] "q" (force),
+                  "X" (ex_record_fault_edi));
+
+    return fault;
+}
+
+static const struct stub {
+    unsigned long (*fn)(unsigned long);
+    const char *name;
+} stubs[] = {
+    { stub_sgdt, "SGDT" },
+    { stub_sidt, "SIDT" },
+    { stub_sldt, "SLDT" },
+    { stub_str,  "STR" },
+    { stub_smsw, "SMSW" },
+};
+
+void test_umip(bool umip_active, bool force)
+{
+    unsigned int i;
+    bool user;
+
+    for ( user = false; ; user = true )
+    {
+        exinfo_t exp = user && umip_active ? EXINFO_SYM(GP, 0) : 0;
+
+        for ( i = 0; i < ARRAY_SIZE(stubs); i++)
+        {
+            const struct stub *s = &stubs[i];
+            exinfo_t ret;
+
+            ret = user ? exec_user_param(s->fn, force) : s->fn(force);
+
+            /*
+             * Tolerate the instruction emulator not understanding these
+             * instructions in older releases of Xen.
+             */
+
+            if ( force && ret == EXINFO_SYM(UD, 0) )
+            {
+                static bool once;
+
+                if ( !once )
+                {
+                    xtf_skip("Skip: Emulator doesn't implement %s\n", s->name);
+                    once = true;
+                }
+
+                continue;
+            }
+
+            if ( ret != exp )
+                xtf_failure("Fail: %s %s\n"
+                            "  expected %pe\n"
+                            "       got %pe\n",
+                            user ? "user" : "supervisor", s->name,
+                            _p(exp), _p(ret));
+        }
+
+        if ( user )
+            break;
+    }
+}
+
+void test_main(void)
+{
+    unsigned long cr4;
+
+    /* run with UMIP inactive */
+    test_umip(false, false);
+
+    if ( !xtf_has_fep )
+        xtf_skip("FEP support not detected - some tests will be skipped\n");
+    else
+        test_umip(false, true);
+
+    if ( !cpu_has_umip )
+    {
+        xtf_skip("UMIP is not supported, skip the rest of test\n");
+        return;
+    }
+
+    cr4 = read_cr4();
+
+    if ( !write_cr4_safe(cr4 | X86_CR4_UMIP) )
+    {
+        xtf_failure("Fail: Unable to activate UMIP\n");
+        return;
+    }
+
+    /* run with UMIP active */
+    test_umip(true, false);
+
+    if ( !xtf_has_fep )
+        xtf_skip("FEP support not detected - some tests will be skipped\n");
+    else
+        test_umip(true, true);
+
+    if ( !write_cr4_safe(cr4) )
+    {
+        xtf_failure("Fail: Unable to deactivate UMIP\n");
+        return;
+    }
+
+    /* run with UMIP inactive */
+    test_umip(false, false);
+
+    if ( !xtf_has_fep )
+        xtf_skip("FEP support not detected - some tests will be skipped\n");
+    else
+        test_umip(false, true);
+
+    xtf_success(NULL);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.13.3


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH XTF v2] Functional: Add a UMIP test

[Andrew Cooper]

On 14/08/17 06:08, Boqun Feng (Intel) wrote:
> Add a "umip" test for the User-Model Instruction Prevention. The test
> simply tries to run sgdt/sidt/sldt/str/smsw in guest user-mode with
> CR4_UMIP = 1.
>
> Signed-off-by: Boqun Feng (Intel) <boqun.feng@gmail.com>

Thankyou for this.  Its looking much better.  Just a few comments.

> ---
> v1 --> v2:
> 	* add a new write_cr4_safe()
> 	* use %pe for exception print
> 	* refactor the code based on Andrew's guide and advice
>
> Test results:
>
> * With UMIP patch:
> ** boot with hvm_fep: SUCCESS
> ** boot without hvm_fep: SKIP, due to "FEP support not detected.."
>
> * Without UMIP patch:
> ** boot with hvm_fep: SKIP, due to "UMIP is not supported.."
> ** boot without hvm_fep: SKIP, due to "UMIP is not supported.."
>
> * With UMIP cpuid exposed but CR4 invalid:
> ** boot with hvm_fep: FAILURE, due to "Fail: Unable to activate UMIP.."
> ** boot without hvm_fep: FAILURE, due to "Fail: Unable to activate UMIP.."

What do you mean by CR4 invalid here?

> diff --git a/arch/x86/include/arch/lib.h b/arch/x86/include/arch/lib.h
> index f608af9996f0..4f0d85290cf0 100644
> --- a/arch/x86/include/arch/lib.h
> +++ b/arch/x86/include/arch/lib.h
> @@ -340,6 +340,19 @@ static inline void write_cr4(unsigned long cr4)
>      asm volatile ("mov %0, %%cr4" :: "r" (cr4));
>  }
>  
> +static inline bool write_cr4_safe(unsigned long cr4)
> +{
> +    exinfo_t fault = 0;
> +
> +    asm volatile ("1: mov %1, %%cr4; 2:"
> +                  _ASM_EXTABLE_HANDLER(1b, 2b, ex_record_fault_edi)
> +                  : "+D" (fault)
> +                  : "r" (cr4),
> +                    "X" (ex_record_fault_edi));
> +
> +    return !fault;

To match the existing {rd,wr}msr_safe() implementation in XTF and the
common usage in Linux and Xen, the return value should be 0 for success
and nonzero for fault.

i.e. you should "return fault;"

> diff --git a/tests/umip/main.c b/tests/umip/main.c
> new file mode 100644
> index 000000000000..fcaba4e34570
> --- /dev/null
> +++ b/tests/umip/main.c
>
> +
> +static const struct stub {
> +    unsigned long (*fn)(unsigned long);
> +    const char *name;
> +} stubs[] = {
> +    { stub_sgdt, "SGDT" },
> +    { stub_sidt, "SIDT" },
> +    { stub_sldt, "SLDT" },
> +    { stub_str,  "STR" },
> +    { stub_smsw, "SMSW" },
> +};
> +
> +void test_umip(bool umip_active, bool force)

(For reasons explained below,) I'd rename this to test_insns(), and...

> +{
> +    unsigned int i;
> +    bool user;
> +
> +    for ( user = false; ; user = true )
> +    {
> +        exinfo_t exp = user && umip_active ? EXINFO_SYM(GP, 0) : 0;
> +
> +        for ( i = 0; i < ARRAY_SIZE(stubs); i++)
> +        {
> +            const struct stub *s = &stubs[i];
> +            exinfo_t ret;
> +
> +            ret = user ? exec_user_param(s->fn, force) : s->fn(force);
> +
> +            /*
> +             * Tolerate the instruction emulator not understanding these
> +             * instructions in older releases of Xen.
> +             */
> +
> +            if ( force && ret == EXINFO_SYM(UD, 0) )
> +            {
> +                static bool once;
> +
> +                if ( !once )
> +                {
> +                    xtf_skip("Skip: Emulator doesn't implement %s\n", s->name);
> +                    once = true;
> +                }
> +
> +                continue;
> +            }
> +
> +            if ( ret != exp )
> +                xtf_failure("Fail: %s %s\n"
> +                            "  expected %pe\n"
> +                            "       got %pe\n",
> +                            user ? "user" : "supervisor", s->name,
> +                            _p(exp), _p(ret));
> +        }
> +
> +        if ( user )
> +            break;
> +    }
> +}

... have this helper, which simplifies the test_main() logic.

static void test_umip(bool umip_active)
{
    test_insns(umip_active, false);

    if ( xtf_has_fep )
        test_insns(umip_active, true);
}

> +
> +void test_main(void)
> +{
> +    unsigned long cr4;
> +
> +    /* run with UMIP inactive */
> +    test_umip(false, false);
> +
> +    if ( !xtf_has_fep )
> +        xtf_skip("FEP support not detected - some tests will be skipped\n");

This text only needs to be shown once.  I'd move it ahead of the first
test_umip() call, and you can drop the else clauses given the new
test_umip() implementation.

> +    else
> +        test_umip(false, true);
> +
> +    if ( !cpu_has_umip )
> +    {
> +        xtf_skip("UMIP is not supported, skip the rest of test\n");

Since I pointed you at the cpuid-faulting test case, I've had my code
reviewed by someone in our testing team, and a logical issue was found. 
(As it turns out, when CPUID faulting is not available, writes
attempting to enable it are squashed and appear to have succeeded.  I'll
fix that bug in due course.)

At this point, we should check that attempting to set CR4.UMIP is
prohibited.

Otherwise, everything else looks fine.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH XTF v2] Functional: Add a UMIP test

[Boqun Feng]

[-- Attachment #1.1: Type: text/plain, Size: 6117 bytes --]

On Mon, Aug 14, 2017 at 11:35:47AM +0100, Andrew Cooper wrote:
> On 14/08/17 06:08, Boqun Feng (Intel) wrote:
> > Add a "umip" test for the User-Model Instruction Prevention. The test
> > simply tries to run sgdt/sidt/sldt/str/smsw in guest user-mode with
> > CR4_UMIP = 1.
> >
> > Signed-off-by: Boqun Feng (Intel) <boqun.feng@gmail.com>
> 
> Thankyou for this.  Its looking much better.  Just a few comments.
> 

Apologies for being so late, got interrupted by something more urgent..

> > ---
> > v1 --> v2:
> > 	* add a new write_cr4_safe()
> > 	* use %pe for exception print
> > 	* refactor the code based on Andrew's guide and advice
> >
> > Test results:
> >
> > * With UMIP patch:
> > ** boot with hvm_fep: SUCCESS
> > ** boot without hvm_fep: SKIP, due to "FEP support not detected.."
> >
> > * Without UMIP patch:
> > ** boot with hvm_fep: SKIP, due to "UMIP is not supported.."
> > ** boot without hvm_fep: SKIP, due to "UMIP is not supported.."
> >
> > * With UMIP cpuid exposed but CR4 invalid:
> > ** boot with hvm_fep: FAILURE, due to "Fail: Unable to activate UMIP.."
> > ** boot without hvm_fep: FAILURE, due to "Fail: Unable to activate UMIP.."
> 
> What do you mean by CR4 invalid here?
> 

I mean hvm_cr4_guest_valid_bits() doesn't return with X86_CR4_UMIP set,
and I manually modified my "Expose UMIP" patch to test this.

> > diff --git a/arch/x86/include/arch/lib.h b/arch/x86/include/arch/lib.h
> > index f608af9996f0..4f0d85290cf0 100644
> > --- a/arch/x86/include/arch/lib.h
> > +++ b/arch/x86/include/arch/lib.h
> > @@ -340,6 +340,19 @@ static inline void write_cr4(unsigned long cr4)
> >      asm volatile ("mov %0, %%cr4" :: "r" (cr4));
> >  }
> >  
> > +static inline bool write_cr4_safe(unsigned long cr4)
> > +{
> > +    exinfo_t fault = 0;
> > +
> > +    asm volatile ("1: mov %1, %%cr4; 2:"
> > +                  _ASM_EXTABLE_HANDLER(1b, 2b, ex_record_fault_edi)
> > +                  : "+D" (fault)
> > +                  : "r" (cr4),
> > +                    "X" (ex_record_fault_edi));
> > +
> > +    return !fault;
> 
> To match the existing {rd,wr}msr_safe() implementation in XTF and the
> common usage in Linux and Xen, the return value should be 0 for success
> and nonzero for fault.
> 
> i.e. you should "return fault;"
> 

Got it.

> > diff --git a/tests/umip/main.c b/tests/umip/main.c
> > new file mode 100644
> > index 000000000000..fcaba4e34570
> > --- /dev/null
> > +++ b/tests/umip/main.c
> >
> > +
> > +static const struct stub {
> > +    unsigned long (*fn)(unsigned long);
> > +    const char *name;
> > +} stubs[] = {
> > +    { stub_sgdt, "SGDT" },
> > +    { stub_sidt, "SIDT" },
> > +    { stub_sldt, "SLDT" },
> > +    { stub_str,  "STR" },
> > +    { stub_smsw, "SMSW" },
> > +};
> > +
> > +void test_umip(bool umip_active, bool force)
> 
> (For reasons explained below,) I'd rename this to test_insns(), and...
> 
> > +{
> > +    unsigned int i;
> > +    bool user;
> > +
> > +    for ( user = false; ; user = true )
> > +    {
> > +        exinfo_t exp = user && umip_active ? EXINFO_SYM(GP, 0) : 0;
> > +
> > +        for ( i = 0; i < ARRAY_SIZE(stubs); i++)
> > +        {
> > +            const struct stub *s = &stubs[i];
> > +            exinfo_t ret;
> > +
> > +            ret = user ? exec_user_param(s->fn, force) : s->fn(force);
> > +
> > +            /*
> > +             * Tolerate the instruction emulator not understanding these
> > +             * instructions in older releases of Xen.
> > +             */
> > +
> > +            if ( force && ret == EXINFO_SYM(UD, 0) )
> > +            {
> > +                static bool once;
> > +
> > +                if ( !once )
> > +                {
> > +                    xtf_skip("Skip: Emulator doesn't implement %s\n", s->name);
> > +                    once = true;
> > +                }
> > +
> > +                continue;
> > +            }
> > +
> > +            if ( ret != exp )
> > +                xtf_failure("Fail: %s %s\n"
> > +                            "  expected %pe\n"
> > +                            "       got %pe\n",
> > +                            user ? "user" : "supervisor", s->name,
> > +                            _p(exp), _p(ret));
> > +        }
> > +
> > +        if ( user )
> > +            break;
> > +    }
> > +}
> 
> ... have this helper, which simplifies the test_main() logic.
> 
> static void test_umip(bool umip_active)
> {
>     test_insns(umip_active, false);
> 
>     if ( xtf_has_fep )
>         test_insns(umip_active, true);
> }
> 

Good point, will do this in V3.

> > +
> > +void test_main(void)
> > +{
> > +    unsigned long cr4;
> > +
> > +    /* run with UMIP inactive */
> > +    test_umip(false, false);
> > +
> > +    if ( !xtf_has_fep )
> > +        xtf_skip("FEP support not detected - some tests will be skipped\n");
> 
> This text only needs to be shown once.  I'd move it ahead of the first
> test_umip() call, and you can drop the else clauses given the new
> test_umip() implementation.
> 

Agreed.

> > +    else
> > +        test_umip(false, true);
> > +
> > +    if ( !cpu_has_umip )
> > +    {
> > +        xtf_skip("UMIP is not supported, skip the rest of test\n");
> 
> Since I pointed you at the cpuid-faulting test case, I've had my code
> reviewed by someone in our testing team, and a logical issue was found. 
> (As it turns out, when CPUID faulting is not available, writes
> attempting to enable it are squashed and appear to have succeeded.  I'll
> fix that bug in due course.)
> 
> At this point, we should check that attempting to set CR4.UMIP is
> prohibited.
> 

Ok, I will add something like:

	if ( !cpu_has_umip )
	{
	     if (!write_cr4_safe(cr4 | X86_CR4_UMIP))
	         xtf_fail("UMIP unsupported, but setting CR4 bit succeeds");
	     else
	         xtf_skip("UMIP is not supported, skip the rest of test\n");
	}

Thoughts?

Regards,
Boqun

> Otherwise, everything else looks fine.
> 
> ~Andrew

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH XTF v2] Functional: Add a UMIP test

[Andrew Cooper]

On 14/08/17 13:43, Boqun Feng wrote:
> On Mon, Aug 14, 2017 at 11:35:47AM +0100, Andrew Cooper wrote:
>> On 14/08/17 06:08, Boqun Feng (Intel) wrote:
>>> Add a "umip" test for the User-Model Instruction Prevention. The test
>>> simply tries to run sgdt/sidt/sldt/str/smsw in guest user-mode with
>>> CR4_UMIP = 1.
>>>
>>> Signed-off-by: Boqun Feng (Intel) <boqun.feng@gmail.com>
>> Thankyou for this.  Its looking much better.  Just a few comments.
>>
> Apologies for being so late, got interrupted by something more urgent..

No worries.  I get exactly the same kind of interruptions as well.

>
>>> ---
>>> v1 --> v2:
>>> 	* add a new write_cr4_safe()
>>> 	* use %pe for exception print
>>> 	* refactor the code based on Andrew's guide and advice
>>>
>>> Test results:
>>>
>>> * With UMIP patch:
>>> ** boot with hvm_fep: SUCCESS
>>> ** boot without hvm_fep: SKIP, due to "FEP support not detected.."
>>>
>>> * Without UMIP patch:
>>> ** boot with hvm_fep: SKIP, due to "UMIP is not supported.."
>>> ** boot without hvm_fep: SKIP, due to "UMIP is not supported.."
>>>
>>> * With UMIP cpuid exposed but CR4 invalid:
>>> ** boot with hvm_fep: FAILURE, due to "Fail: Unable to activate UMIP.."
>>> ** boot without hvm_fep: FAILURE, due to "Fail: Unable to activate UMIP.."
>> What do you mean by CR4 invalid here?
>>
> I mean hvm_cr4_guest_valid_bits() doesn't return with X86_CR4_UMIP set,
> and I manually modified my "Expose UMIP" patch to test this.

Ah ok.

>>> +    else
>>> +        test_umip(false, true);
>>> +
>>> +    if ( !cpu_has_umip )
>>> +    {
>>> +        xtf_skip("UMIP is not supported, skip the rest of test\n");
>> Since I pointed you at the cpuid-faulting test case, I've had my code
>> reviewed by someone in our testing team, and a logical issue was found. 
>> (As it turns out, when CPUID faulting is not available, writes
>> attempting to enable it are squashed and appear to have succeeded.  I'll
>> fix that bug in due course.)
>>
>> At this point, we should check that attempting to set CR4.UMIP is
>> prohibited.
>>
> Ok, I will add something like:
>
> 	if ( !cpu_has_umip )
> 	{
> 	     if (!write_cr4_safe(cr4 | X86_CR4_UMIP))
> 	         xtf_fail("UMIP unsupported, but setting CR4 bit succeeds");
> 	     else
> 	         xtf_skip("UMIP is not supported, skip the rest of test\n");
> 	}
>
> Thoughts?

I'd personally go with this, because I think it is slightly clearer
logic to follow.

if ( !cpu_has_umip )
{
    xtf_skip("UMIP is not supported, skip the rest of test\n");

    if ( !write_cr4_safe(cr4 | X86_CR4_UMIP) )
        xtf_fail("UMIP unsupported, but setting CR4 bit succeeded\n");
}

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel