|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH XTF] Functional: Add a UMIP test
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@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |