| Message ID | 20170720052921.31586-2-boqun.feng@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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: + */
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