[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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.