Re: [Xen-devel] [PATCH XTF] Functional: Add a UMIP test

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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?

> ---
>   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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel