| Message ID | 1559677885-10731-2-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libx86: Fuzzing harness | expand |
>>> 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
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
>>> 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
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>
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(-)