[1/2] libx86: Helper for clearing out-of-range CPUID leaves
diff mbox series

Message ID 1559677885-10731-2-git-send-email-andrew.cooper3@citrix.com
State New
Headers show
Series
  • libx86: Fuzzing harness
Related show

Commit Message

Andrew Cooper June 4, 2019, 7:51 p.m. UTC
When merging a levelled policy, stale out-of-range leaves may remain.
Introduce a helper to clear them, and test a number of the subtle corner
cases.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 tools/tests/cpu-policy/test-cpu-policy.c | 161 ++++++++++++++++++++++++++++++-
 xen/include/xen/lib/x86/cpuid.h          |  11 +++
 xen/lib/x86/cpuid.c                      |  59 +++++++++++
 xen/lib/x86/private.h                    |   1 +
 4 files changed, 230 insertions(+), 2 deletions(-)

Comments

Jan Beulich June 5, 2019, 9:45 a.m. UTC | #1
>>> On 04.06.19 at 21:51, <andrew.cooper3@citrix.com> wrote:
> @@ -163,6 +170,58 @@ void x86_cpuid_policy_fill_native(struct cpuid_policy *p)
>      recalculate_synth(p);
>  }
>  
> +void x86_cpuid_policy_clear_out_of_range_leaves(struct cpuid_policy *p)
> +{
> +    unsigned int i;
> +
> +    zero_leaves(p->basic.raw, p->basic.max_leaf + 1,
> +                ARRAY_SIZE(p->basic.raw) - 1);
> +
> +    if ( p->basic.max_leaf < 4 )
> +        memset(p->cache.raw, 0, sizeof(p->cache.raw));
> +    else
> +    {
> +        for ( i = 0; (i < ARRAY_SIZE(p->cache.raw) &&
> +                      p->cache.subleaf[i].type); ++i )
> +            ;
> +
> +        zero_leaves(p->cache.raw, i + 1,
> +                    ARRAY_SIZE(p->cache.raw) - 1);

Do you really want "i + 1" here? Wouldn't it be better to fully zap
subleaf i as well, when its type is zero? Same for leaf 0xb then.

> +    }
> +
> +    if ( p->basic.max_leaf < 7 )
> +        memset(p->feat.raw, 0, sizeof(p->feat.raw));
> +    else
> +        zero_leaves(p->feat.raw, p->feat.max_subleaf + 1,
> +                    ARRAY_SIZE(p->feat.raw) - 1);
> +
> +    if ( p->basic.max_leaf < 0xb )
> +        memset(p->topo.raw, 0, sizeof(p->topo.raw));
> +    else
> +    {
> +        for ( i = 0; (i < ARRAY_SIZE(p->topo.raw) &&
> +                      p->topo.subleaf[i].type); ++i )
> +            ;
> +
> +        zero_leaves(p->topo.raw, i + 1,
> +                    ARRAY_SIZE(p->topo.raw) - 1);
> +    }
> +
> +    if ( p->basic.max_leaf < 0xd || !cpuid_policy_xstates(p) )
> +        memset(p->xstate.raw, 0, sizeof(p->xstate.raw));
> +    else
> +    {
> +        /* First two leaves always valid.  Rest depend on xstates. */
> +        i = max(2, 64 - __builtin_clzll(cpuid_policy_xstates(p)));
> +
> +        zero_leaves(p->xstate.raw, i,
> +                    ARRAY_SIZE(p->xstate.raw) - 1);
> +    }

In x86_cpuid_policy_fill_native() you're using 63 as the loop
bound, guaranteeing to ignore bit 63 in case it gains a meaning.
Here and in x86_cpuid_copy_to_buffer() you don't. I'm slightly
worried that these code sequences will need changing (with no
way to easily notice) when CPUID_GUEST_NR_XSTATE gets
increased. But I'm not going to insist - for now the code is fine
as is (afaict).

Having reached the end of the patch and seeing the title of
patch 2 - is there no need to use this function anywhere
outside the fuzzing harness?

Jan
Andrew Cooper June 5, 2019, 11:26 a.m. UTC | #2
On 05/06/2019 10:45, Jan Beulich wrote:
>>>> On 04.06.19 at 21:51, <andrew.cooper3@citrix.com> wrote:
>> @@ -163,6 +170,58 @@ void x86_cpuid_policy_fill_native(struct cpuid_policy *p)
>>      recalculate_synth(p);
>>  }
>>  
>> +void x86_cpuid_policy_clear_out_of_range_leaves(struct cpuid_policy *p)
>> +{
>> +    unsigned int i;
>> +
>> +    zero_leaves(p->basic.raw, p->basic.max_leaf + 1,
>> +                ARRAY_SIZE(p->basic.raw) - 1);
>> +
>> +    if ( p->basic.max_leaf < 4 )
>> +        memset(p->cache.raw, 0, sizeof(p->cache.raw));
>> +    else
>> +    {
>> +        for ( i = 0; (i < ARRAY_SIZE(p->cache.raw) &&
>> +                      p->cache.subleaf[i].type); ++i )
>> +            ;
>> +
>> +        zero_leaves(p->cache.raw, i + 1,
>> +                    ARRAY_SIZE(p->cache.raw) - 1);
> Do you really want "i + 1" here? Wouldn't it be better to fully zap
> subleaf i as well, when its type is zero? Same for leaf 0xb then.

This is an awkward corner case which the manual doesn't cover adequately.

"If ECX contains an invalid sub leaf index, EAX/EBX/ECX/EDX return 0.
Sub-leaf index n+1 is invalid if sub-leaf n returns EAX[4:0] as 0."

which makes the leaf with type 0 valid, rather than invalid.

That said, from a "how it is likely to work in hardware" point of view,
I highly suspect that a real CPUID instruction doesn't store anything
for the first leaf with type 0, and relies on the "return all zeroes"
property.

Therefore, I will follow your suggestion and adjust the testcases
accordingly.

>
>> +    }
>> +
>> +    if ( p->basic.max_leaf < 7 )
>> +        memset(p->feat.raw, 0, sizeof(p->feat.raw));
>> +    else
>> +        zero_leaves(p->feat.raw, p->feat.max_subleaf + 1,
>> +                    ARRAY_SIZE(p->feat.raw) - 1);
>> +
>> +    if ( p->basic.max_leaf < 0xb )
>> +        memset(p->topo.raw, 0, sizeof(p->topo.raw));
>> +    else
>> +    {
>> +        for ( i = 0; (i < ARRAY_SIZE(p->topo.raw) &&
>> +                      p->topo.subleaf[i].type); ++i )
>> +            ;
>> +
>> +        zero_leaves(p->topo.raw, i + 1,
>> +                    ARRAY_SIZE(p->topo.raw) - 1);
>> +    }
>> +
>> +    if ( p->basic.max_leaf < 0xd || !cpuid_policy_xstates(p) )
>> +        memset(p->xstate.raw, 0, sizeof(p->xstate.raw));
>> +    else
>> +    {
>> +        /* First two leaves always valid.  Rest depend on xstates. */
>> +        i = max(2, 64 - __builtin_clzll(cpuid_policy_xstates(p)));
>> +
>> +        zero_leaves(p->xstate.raw, i,
>> +                    ARRAY_SIZE(p->xstate.raw) - 1);
>> +    }
> In x86_cpuid_policy_fill_native() you're using 63 as the loop
> bound, guaranteeing to ignore bit 63 in case it gains a meaning.

It is currently "Reserved for XCR0 bit vector expansion", which will
almost certainly be used in a similar manor to the
secondary_exec_control_enable bit in VT-x.

> Here and in x86_cpuid_copy_to_buffer() you don't. I'm slightly
> worried that these code sequences will need changing (with no
> way to easily notice) when CPUID_GUEST_NR_XSTATE gets
> increased. But I'm not going to insist - for now the code is fine
> as is (afaict).

I think the code sequences are going to have to change no-matter-what. 
It is also safe at the moment because of the ARRAY_SIZE() expression
stopping at bit 62, which was LWP.

If this were to change, it would also have to include a min(63, ...)
expression, because 64 - clz() is the correct expression for finding the
first leaf to clobber in the general case.

Would a BUILD_BUG_ON(ARRAY_SIZE(p->xstate.raw) > 63) be ok in the
interim?  I'd prefer where possible to not build in assumptions based on
the array size.

> Having reached the end of the patch and seeing the title of
> patch 2 - is there no need to use this function anywhere
> outside the fuzzing harness?

Not yet, because you also disliked having patches with stub functionality.

XEN_DOMCTL_set_cpu_policy needs it, because for one common usecase, the
domain currently has a max policy, and the toolstack is handing a
levelled-down policy.  This needs to be taken into account before the
check of "is the new policy well formed and compatible?" takes place.

~Andrew
Jan Beulich June 5, 2019, 1:01 p.m. UTC | #3
>>> On 05.06.19 at 13:26, <andrew.cooper3@citrix.com> wrote:
> On 05/06/2019 10:45, Jan Beulich wrote:
>>>>> On 04.06.19 at 21:51, <andrew.cooper3@citrix.com> wrote:
>>> @@ -163,6 +170,58 @@ void x86_cpuid_policy_fill_native(struct cpuid_policy 
> *p)
>>>      recalculate_synth(p);
>>>  }
>>>  
>>> +void x86_cpuid_policy_clear_out_of_range_leaves(struct cpuid_policy *p)
>>> +{
>>> +    unsigned int i;
>>> +
>>> +    zero_leaves(p->basic.raw, p->basic.max_leaf + 1,
>>> +                ARRAY_SIZE(p->basic.raw) - 1);
>>> +
>>> +    if ( p->basic.max_leaf < 4 )
>>> +        memset(p->cache.raw, 0, sizeof(p->cache.raw));
>>> +    else
>>> +    {
>>> +        for ( i = 0; (i < ARRAY_SIZE(p->cache.raw) &&
>>> +                      p->cache.subleaf[i].type); ++i )
>>> +            ;
>>> +
>>> +        zero_leaves(p->cache.raw, i + 1,
>>> +                    ARRAY_SIZE(p->cache.raw) - 1);
>> Do you really want "i + 1" here? Wouldn't it be better to fully zap
>> subleaf i as well, when its type is zero? Same for leaf 0xb then.
> 
> This is an awkward corner case which the manual doesn't cover adequately.
> 
> "If ECX contains an invalid sub leaf index, EAX/EBX/ECX/EDX return 0.
> Sub-leaf index n+1 is invalid if sub-leaf n returns EAX[4:0] as 0."
> 
> which makes the leaf with type 0 valid, rather than invalid.
> 
> That said, from a "how it is likely to work in hardware" point of view,
> I highly suspect that a real CPUID instruction doesn't store anything
> for the first leaf with type 0, and relies on the "return all zeroes"
> property.
> 
> Therefore, I will follow your suggestion and adjust the testcases
> accordingly.
> 
>>
>>> +    }
>>> +
>>> +    if ( p->basic.max_leaf < 7 )
>>> +        memset(p->feat.raw, 0, sizeof(p->feat.raw));
>>> +    else
>>> +        zero_leaves(p->feat.raw, p->feat.max_subleaf + 1,
>>> +                    ARRAY_SIZE(p->feat.raw) - 1);
>>> +
>>> +    if ( p->basic.max_leaf < 0xb )
>>> +        memset(p->topo.raw, 0, sizeof(p->topo.raw));
>>> +    else
>>> +    {
>>> +        for ( i = 0; (i < ARRAY_SIZE(p->topo.raw) &&
>>> +                      p->topo.subleaf[i].type); ++i )
>>> +            ;
>>> +
>>> +        zero_leaves(p->topo.raw, i + 1,
>>> +                    ARRAY_SIZE(p->topo.raw) - 1);
>>> +    }
>>> +
>>> +    if ( p->basic.max_leaf < 0xd || !cpuid_policy_xstates(p) )
>>> +        memset(p->xstate.raw, 0, sizeof(p->xstate.raw));
>>> +    else
>>> +    {
>>> +        /* First two leaves always valid.  Rest depend on xstates. */
>>> +        i = max(2, 64 - __builtin_clzll(cpuid_policy_xstates(p)));
>>> +
>>> +        zero_leaves(p->xstate.raw, i,
>>> +                    ARRAY_SIZE(p->xstate.raw) - 1);
>>> +    }
>> In x86_cpuid_policy_fill_native() you're using 63 as the loop
>> bound, guaranteeing to ignore bit 63 in case it gains a meaning.
> 
> It is currently "Reserved for XCR0 bit vector expansion", which will
> almost certainly be used in a similar manor to the
> secondary_exec_control_enable bit in VT-x.
> 
>> Here and in x86_cpuid_copy_to_buffer() you don't. I'm slightly
>> worried that these code sequences will need changing (with no
>> way to easily notice) when CPUID_GUEST_NR_XSTATE gets
>> increased. But I'm not going to insist - for now the code is fine
>> as is (afaict).
> 
> I think the code sequences are going to have to change no-matter-what. 
> It is also safe at the moment because of the ARRAY_SIZE() expression
> stopping at bit 62, which was LWP.
> 
> If this were to change, it would also have to include a min(63, ...)
> expression, because 64 - clz() is the correct expression for finding the
> first leaf to clobber in the general case.
> 
> Would a BUILD_BUG_ON(ARRAY_SIZE(p->xstate.raw) > 63) be ok in the
> interim?  I'd prefer where possible to not build in assumptions based on
> the array size.

Yes, that'd be fine with me. Perhaps, albeit unrelated, I could
talk you into also inserting such a statement at the other place
identified?

With the agreed upon adjustments made
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Patch
diff mbox series

diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
index fd96c0b..ca2d8d3 100644
--- a/tools/tests/cpu-policy/test-cpu-policy.c
+++ b/tools/tests/cpu-policy/test-cpu-policy.c
@@ -20,6 +20,17 @@  static unsigned int nr_failures;
     printf(fmt, ##__VA_ARGS__);                 \
 })
 
+#define memdup(ptr)                             \
+({                                              \
+    typeof(*(ptr)) *p_ = (ptr);                 \
+    void *n_ = malloc(sizeof(*p_));             \
+                                                \
+    if ( !n_ )                                  \
+        err(1, "%s malloc failure", __func__);  \
+                                                \
+    memcpy(n_, p_, sizeof(*p_));                \
+})
+
 static void test_vendor_identification(void)
 {
     static const struct test {
@@ -345,6 +356,151 @@  static void test_msr_deserialise_failure(void)
     }
 }
 
+static void test_cpuid_out_of_range_clearing(void)
+{
+    static const struct test {
+        const char *name;
+        unsigned int nr_markers;
+        struct cpuid_policy p;
+    } tests[] = {
+        {
+            .name = "basic",
+            .nr_markers = 1,
+            .p = {
+                /* Retains marker in leaf 0.  Clears others. */
+                .basic.max_leaf = 0,
+                .basic.vendor_ebx = 0xc2,
+
+                .basic.raw_fms = 0xc2,
+                .cache.raw[0].a = 0xc2,
+                .feat.raw[0].a = 0xc2,
+                .topo.raw[0].a = 0xc2,
+                .xstate.raw[0].a = 0xc2,
+                .xstate.raw[1].a = 0xc2,
+            },
+        },
+        {
+            .name = "cache",
+            .nr_markers = 1,
+            .p = {
+                /* Retains marker in subleaf 0.  Clears others. */
+                .basic.max_leaf = 4,
+                .cache.raw[0].b = 0xc2,
+
+                .cache.raw[1].b = 0xc2,
+                .feat.raw[0].a = 0xc2,
+                .topo.raw[0].a = 0xc2,
+                .xstate.raw[0].a = 0xc2,
+                .xstate.raw[1].a = 0xc2,
+            },
+        },
+        {
+            .name = "feat",
+            .nr_markers = 1,
+            .p = {
+                /* Retains marker in subleaf 0.  Clears others. */
+                .basic.max_leaf = 7,
+                .feat.raw[0].b = 0xc2,
+
+                .feat.raw[1].b = 0xc2,
+                .topo.raw[0].a = 0xc2,
+                .xstate.raw[0].a = 0xc2,
+                .xstate.raw[1].a = 0xc2,
+            },
+        },
+        {
+            .name = "topo",
+            .nr_markers = 1,
+            .p = {
+                /* Retains marker in subleaf 0.  Clears others. */
+                .basic.max_leaf = 0xb,
+                .topo.raw[0].b = 0xc2,
+
+                .topo.raw[1].b = 0xc2,
+                .xstate.raw[0].a = 0xc2,
+                .xstate.raw[1].a = 0xc2,
+            },
+        },
+        {
+            .name = "xstate x87",
+            .nr_markers = 2,
+            .p = {
+                /* First two subleaves always valid.  Others cleared. */
+                .basic.max_leaf = 0xd,
+                .xstate.raw[0].a = 1,
+                .xstate.raw[0].b = 0xc2,
+                .xstate.raw[1].b = 0xc2,
+
+                .xstate.raw[2].b = 0xc2,
+                .xstate.raw[3].b = 0xc2,
+            },
+        },
+        {
+            .name = "xstate sse",
+            .nr_markers = 2,
+            .p = {
+                /* First two subleaves always valid.  Others cleared. */
+                .basic.max_leaf = 0xd,
+                .xstate.raw[0].a = 2,
+                .xstate.raw[0].b = 0xc2,
+                .xstate.raw[1].b = 0xc2,
+
+                .xstate.raw[2].b = 0xc2,
+                .xstate.raw[3].b = 0xc2,
+            },
+        },
+        {
+            .name = "xstate avx",
+            .nr_markers = 3,
+            .p = {
+                /* Third subleaf also valid.  Others cleared. */
+                .basic.max_leaf = 0xd,
+                .xstate.raw[0].a = 7,
+                .xstate.raw[0].b = 0xc2,
+                .xstate.raw[1].b = 0xc2,
+                .xstate.raw[2].b = 0xc2,
+
+                .xstate.raw[3].b = 0xc2,
+            },
+        },
+        {
+            .name = "extd",
+            .nr_markers = 1,
+            .p = {
+                /* Retains marker in leaf 0.  Clears others. */
+                .extd.max_leaf = 0,
+                .extd.vendor_ebx = 0xc2,
+
+                .extd.raw_fms = 0xc2,
+            },
+        },
+    };
+
+    printf("Testing CPUID out-of-range clearing:\n");
+
+    for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
+    {
+        const struct test *t = &tests[i];
+        struct cpuid_policy *p = memdup(&t->p);
+        void *ptr;
+        unsigned int nr_markers;
+
+        x86_cpuid_policy_clear_out_of_range_leaves(p);
+
+        /* Count the number of 0xc2's still remaining. */
+        for ( ptr = p, nr_markers = 0;
+              (ptr = memchr(ptr, 0xc2, (void *)p + sizeof(*p) - ptr));
+              ptr++, nr_markers++ )
+            ;
+
+        if ( nr_markers != t->nr_markers )
+             fail("  Test %s fail - expected %u markers, got %u\n",
+                  t->name, t->nr_markers, nr_markers);
+
+        free(p);
+    }
+}
+
 int main(int argc, char **argv)
 {
     printf("CPU Policy unit tests\n");
@@ -352,9 +508,10 @@  int main(int argc, char **argv)
     test_vendor_identification();
 
     test_cpuid_serialise_success();
-    test_msr_serialise_success();
-
     test_cpuid_deserialise_failure();
+    test_cpuid_out_of_range_clearing();
+
+    test_msr_serialise_success();
     test_msr_deserialise_failure();
 
     if ( nr_failures )
diff --git a/xen/include/xen/lib/x86/cpuid.h b/xen/include/xen/lib/x86/cpuid.h
index ed7d7b4..2618598 100644
--- a/xen/include/xen/lib/x86/cpuid.h
+++ b/xen/include/xen/lib/x86/cpuid.h
@@ -331,6 +331,17 @@  const uint32_t *x86_cpuid_lookup_deep_deps(uint32_t feature);
  */
 void x86_cpuid_policy_fill_native(struct cpuid_policy *p);
 
+/**
+ * Clear leaf data beyond the policies max leaf/subleaf settings.
+ *
+ * Policy serialisation purposefully omits out-of-range leaves, because there
+ * are a large number of them due to vendor differences.  However, when
+ * constructing new policies (e.g. levelling down), it is possible to end up
+ * with out-of-range leaves with stale content in them.  This helper clears
+ * them.
+ */
+void x86_cpuid_policy_clear_out_of_range_leaves(struct cpuid_policy *p);
+
 #ifdef __XEN__
 #include <public/arch-x86/xen.h>
 typedef XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_leaf_buffer_t;
diff --git a/xen/lib/x86/cpuid.c b/xen/lib/x86/cpuid.c
index a82cdb2..0f99fcb 100644
--- a/xen/lib/x86/cpuid.c
+++ b/xen/lib/x86/cpuid.c
@@ -2,6 +2,13 @@ 
 
 #include <xen/lib/x86/cpuid.h>
 
+static void zero_leaves(struct cpuid_leaf *l,
+                        unsigned int first, unsigned int last)
+{
+    if ( first <= last )
+        memset(&l[first], 0, sizeof(*l) * (last - first + 1));
+}
+
 unsigned int x86_cpuid_lookup_vendor(uint32_t ebx, uint32_t ecx, uint32_t edx)
 {
     switch ( ebx )
@@ -163,6 +170,58 @@  void x86_cpuid_policy_fill_native(struct cpuid_policy *p)
     recalculate_synth(p);
 }
 
+void x86_cpuid_policy_clear_out_of_range_leaves(struct cpuid_policy *p)
+{
+    unsigned int i;
+
+    zero_leaves(p->basic.raw, p->basic.max_leaf + 1,
+                ARRAY_SIZE(p->basic.raw) - 1);
+
+    if ( p->basic.max_leaf < 4 )
+        memset(p->cache.raw, 0, sizeof(p->cache.raw));
+    else
+    {
+        for ( i = 0; (i < ARRAY_SIZE(p->cache.raw) &&
+                      p->cache.subleaf[i].type); ++i )
+            ;
+
+        zero_leaves(p->cache.raw, i + 1,
+                    ARRAY_SIZE(p->cache.raw) - 1);
+    }
+
+    if ( p->basic.max_leaf < 7 )
+        memset(p->feat.raw, 0, sizeof(p->feat.raw));
+    else
+        zero_leaves(p->feat.raw, p->feat.max_subleaf + 1,
+                    ARRAY_SIZE(p->feat.raw) - 1);
+
+    if ( p->basic.max_leaf < 0xb )
+        memset(p->topo.raw, 0, sizeof(p->topo.raw));
+    else
+    {
+        for ( i = 0; (i < ARRAY_SIZE(p->topo.raw) &&
+                      p->topo.subleaf[i].type); ++i )
+            ;
+
+        zero_leaves(p->topo.raw, i + 1,
+                    ARRAY_SIZE(p->topo.raw) - 1);
+    }
+
+    if ( p->basic.max_leaf < 0xd || !cpuid_policy_xstates(p) )
+        memset(p->xstate.raw, 0, sizeof(p->xstate.raw));
+    else
+    {
+        /* First two leaves always valid.  Rest depend on xstates. */
+        i = max(2, 64 - __builtin_clzll(cpuid_policy_xstates(p)));
+
+        zero_leaves(p->xstate.raw, i,
+                    ARRAY_SIZE(p->xstate.raw) - 1);
+    }
+
+    zero_leaves(p->extd.raw, (p->extd.max_leaf & 0xffff) + 1,
+                ARRAY_SIZE(p->extd.raw) - 1);
+}
+
 const uint32_t *x86_cpuid_lookup_deep_deps(uint32_t feature)
 {
     static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
diff --git a/xen/lib/x86/private.h b/xen/lib/x86/private.h
index f5b195e..b793181 100644
--- a/xen/lib/x86/private.h
+++ b/xen/lib/x86/private.h
@@ -21,6 +21,7 @@ 
 #include <inttypes.h>
 #include <stdbool.h>
 #include <stddef.h>
+#include <string.h>
 
 #include <xen/asm/msr-index.h>
 #include <xen/asm/x86-vendors.h>