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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxx>

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel