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