* [PATCH 0/3] libx86: Remaining serialisation logic
@ 2019-01-04 15:33 Andrew Cooper
2019-01-04 15:33 ` [PATCH 1/3] libx86: Introduce a helper to deserialise cpuid_policy objects Andrew Cooper
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Andrew Cooper @ 2019-01-04 15:33 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Sergey Dyasli, Wei Liu, Jan Beulich, Roger Pau Monné
I've lost count of exactly which version this is, due to multiple postings of
various sub series, and several substantial rebases.
I think I've addressed all outstanding review content, so lets start from v1
again.
Andrew Cooper (2):
libx86: Introduce a helper to deserialise cpuid_policy objects
tools/cpu-policy: Add unit tests and a fuzzing harness
Roger Pau Monné (1):
libx86: introduce a helper to deserialise msr_policy objects
tools/fuzz/cpu-policy/.gitignore | 1 +
tools/fuzz/cpu-policy/Makefile | 27 ++++
tools/fuzz/cpu-policy/afl-policy-fuzzer.c | 117 ++++++++++++++
tools/tests/Makefile | 1 +
tools/tests/cpu-policy/.gitignore | 1 +
tools/tests/cpu-policy/Makefile | 27 ++++
tools/tests/cpu-policy/test-cpu-policy.c | 247 ++++++++++++++++++++++++++++++
xen/include/xen/lib/x86/cpuid.h | 23 ++-
xen/include/xen/lib/x86/msr.h | 21 +++
xen/lib/x86/cpuid.c | 106 +++++++++++++
xen/lib/x86/msr.c | 67 ++++++++
xen/lib/x86/private.h | 14 ++
12 files changed, 651 insertions(+), 1 deletion(-)
create mode 100644 tools/fuzz/cpu-policy/.gitignore
create mode 100644 tools/fuzz/cpu-policy/Makefile
create mode 100644 tools/fuzz/cpu-policy/afl-policy-fuzzer.c
create mode 100644 tools/tests/cpu-policy/.gitignore
create mode 100644 tools/tests/cpu-policy/Makefile
create mode 100644 tools/tests/cpu-policy/test-cpu-policy.c
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/3] libx86: Introduce a helper to deserialise cpuid_policy objects 2019-01-04 15:33 [PATCH 0/3] libx86: Remaining serialisation logic Andrew Cooper @ 2019-01-04 15:33 ` Andrew Cooper 2019-01-07 10:52 ` Jan Beulich 2019-01-04 15:33 ` [PATCH 2/3] libx86: introduce a helper to deserialise msr_policy objects Andrew Cooper 2019-01-04 15:33 ` [PATCH 3/3] tools/cpu-policy: Add unit tests and a fuzzing harness Andrew Cooper 2 siblings, 1 reply; 10+ messages in thread From: Andrew Cooper @ 2019-01-04 15:33 UTC (permalink / raw) To: Xen-devel Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, Jan Beulich, Sergey Dyasli As with the serialise side, Xen's copy_from_guest API is used, with a compatibility wrapper for the userspace build. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wei.liu2@citrix.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Sergey Dyasli <sergey.dyasli@citrix.com> --- xen/include/xen/lib/x86/cpuid.h | 21 ++++++++ xen/lib/x86/cpuid.c | 106 ++++++++++++++++++++++++++++++++++++++++ xen/lib/x86/private.h | 14 ++++++ 3 files changed, 141 insertions(+) diff --git a/xen/include/xen/lib/x86/cpuid.h b/xen/include/xen/lib/x86/cpuid.h index 22d43ef..767a33b 100644 --- a/xen/include/xen/lib/x86/cpuid.h +++ b/xen/include/xen/lib/x86/cpuid.h @@ -319,6 +319,27 @@ typedef xen_cpuid_leaf_t cpuid_leaf_buffer_t[]; int x86_cpuid_copy_to_buffer(const struct cpuid_policy *policy, cpuid_leaf_buffer_t leaves, uint32_t *nr_entries); +/** + * Unserialise a cpuid_policy object from an array of cpuid leaves. + * + * @param policy The cpuid_policy to unserialise into. + * @param leaves The array of leaves to unserialise from. + * @param nr_entries The number of entries in 'leaves'. + * @param err_leaf Optional hint filled on error. + * @param err_subleaf Optional hint filled on error. + * @returns -errno + * + * Reads at most CPUID_MAX_SERIALISED_LEAVES. May return -ERANGE if an + * incoming leaf is out of range of cpuid_policy, in which case the optional + * err_* pointers are filled to aid diagnostics. + * + * No content validation of in-range leaves is performed. + */ +int x86_cpuid_copy_from_buffer(struct cpuid_policy *policy, + const cpuid_leaf_buffer_t leaves, + uint32_t nr_entries, uint32_t *err_leaf, + uint32_t *err_subleaf); + #endif /* !XEN_LIB_X86_CPUID_H */ /* diff --git a/xen/lib/x86/cpuid.c b/xen/lib/x86/cpuid.c index 5a3159b..7fc4148 100644 --- a/xen/lib/x86/cpuid.c +++ b/xen/lib/x86/cpuid.c @@ -233,6 +233,112 @@ int x86_cpuid_copy_to_buffer(const struct cpuid_policy *p, return 0; } +int x86_cpuid_copy_from_buffer(struct cpuid_policy *p, + const cpuid_leaf_buffer_t leaves, + uint32_t nr_entries, uint32_t *err_leaf, + uint32_t *err_subleaf) +{ + unsigned int i; + xen_cpuid_leaf_t data; + struct cpuid_leaf *l = (void *)&data.a; + + /* + * A well formed caller is expected pass an array with leaves in order, + * and without any repetitions. However, due to per-vendor differences, + * and in the case of upgrade or levelled scenarios, we typically expect + * fewer than MAX leaves to be passed. + * + * Detecting repeated entries is prohibitively complicated, so we don't + * bother. That said, one way or another if more than MAX leaves are + * passed, something is wrong. + */ + if ( nr_entries > CPUID_MAX_SERIALISED_LEAVES ) + return -E2BIG; + + for ( i = 0; i < nr_entries; ++i ) + { + if ( copy_from_buffer_offset(&data, leaves, i, 1) ) + return -EFAULT; + + switch ( data.leaf ) + { + case 0 ... ARRAY_SIZE(p->basic.raw) - 1: + switch ( data.leaf ) + { + case 0x4: + if ( data.subleaf >= ARRAY_SIZE(p->cache.raw) ) + goto out_of_range; + + p->cache.raw[data.subleaf] = *l; + break; + + case 0x7: + if ( data.subleaf >= ARRAY_SIZE(p->feat.raw) ) + goto out_of_range; + + p->feat.raw[data.subleaf] = *l; + break; + + case 0xb: + if ( data.subleaf >= ARRAY_SIZE(p->topo.raw) ) + goto out_of_range; + + p->topo.raw[data.subleaf] = *l; + break; + + case 0xd: + if ( data.subleaf >= ARRAY_SIZE(p->xstate.raw) ) + goto out_of_range; + + p->xstate.raw[data.subleaf] = *l; + break; + + default: + if ( data.subleaf != XEN_CPUID_NO_SUBLEAF ) + goto out_of_range; + + p->basic.raw[data.leaf] = *l; + break; + } + break; + + case 0x40000000: + if ( data.subleaf != XEN_CPUID_NO_SUBLEAF ) + goto out_of_range; + + p->hv_limit = l->a; + break; + + case 0x40000100: + if ( data.subleaf != XEN_CPUID_NO_SUBLEAF ) + goto out_of_range; + + p->hv2_limit = l->a; + break; + + case 0x80000000 ... 0x80000000 + ARRAY_SIZE(p->extd.raw) - 1: + if ( data.subleaf != XEN_CPUID_NO_SUBLEAF ) + goto out_of_range; + + p->extd.raw[data.leaf & 0xffff] = *l; + break; + + default: + goto out_of_range; + } + } + + return 0; + + out_of_range: + if ( err_leaf ) + *err_leaf = data.leaf; + if ( err_subleaf ) + *err_subleaf = data.subleaf; + + return -ERANGE; +} + /* * Local variables: * mode: C diff --git a/xen/lib/x86/private.h b/xen/lib/x86/private.h index 3ee99aa..e0ff2da 100644 --- a/xen/lib/x86/private.h +++ b/xen/lib/x86/private.h @@ -12,6 +12,7 @@ #include <asm/msr-index.h> #define copy_to_buffer_offset copy_to_guest_offset +#define copy_from_buffer_offset copy_from_guest_offset #else @@ -44,6 +45,19 @@ static inline bool test_bit(unsigned int bit, const void *vaddr) 0; \ }) +/* memcpy(), but with copy_from_guest_offset()'s API. */ +#define copy_from_buffer_offset(dst, src, index, nr) \ +({ \ + const typeof(*(src)) *src_ = (src); \ + typeof(*(dst)) *dst_ = (dst); \ + typeof(index) index_ = (index); \ + typeof(nr) nr_ = (nr), i_; \ + \ + for ( i_ = 0; i_ < nr_; i_++ ) \ + dst_[i_] = src_[index_ + i_]; \ + 0; \ +}) + #endif /* __XEN__ */ #endif /* XEN_LIB_X86_PRIVATE_H */ -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] libx86: Introduce a helper to deserialise cpuid_policy objects 2019-01-04 15:33 ` [PATCH 1/3] libx86: Introduce a helper to deserialise cpuid_policy objects Andrew Cooper @ 2019-01-07 10:52 ` Jan Beulich 2019-02-25 11:41 ` Andrew Cooper 0 siblings, 1 reply; 10+ messages in thread From: Jan Beulich @ 2019-01-07 10:52 UTC (permalink / raw) To: Andrew Cooper; +Cc: Sergey Dyasli, Xen-devel, Wei Liu, Roger Pau Monne >>> On 04.01.19 at 16:33, <andrew.cooper3@citrix.com> wrote: > --- a/xen/include/xen/lib/x86/cpuid.h > +++ b/xen/include/xen/lib/x86/cpuid.h > @@ -319,6 +319,27 @@ typedef xen_cpuid_leaf_t cpuid_leaf_buffer_t[]; > int x86_cpuid_copy_to_buffer(const struct cpuid_policy *policy, > cpuid_leaf_buffer_t leaves, uint32_t *nr_entries); > > +/** > + * Unserialise a cpuid_policy object from an array of cpuid leaves. > + * > + * @param policy The cpuid_policy to unserialise into. > + * @param leaves The array of leaves to unserialise from. > + * @param nr_entries The number of entries in 'leaves'. > + * @param err_leaf Optional hint filled on error. > + * @param err_subleaf Optional hint filled on error. > + * @returns -errno > + * > + * Reads at most CPUID_MAX_SERIALISED_LEAVES. May return -ERANGE if an > + * incoming leaf is out of range of cpuid_policy, in which case the optional > + * err_* pointers are filled to aid diagnostics. > + * > + * No content validation of in-range leaves is performed. > + */ > +int x86_cpuid_copy_from_buffer(struct cpuid_policy *policy, > + const cpuid_leaf_buffer_t leaves, > + uint32_t nr_entries, uint32_t *err_leaf, > + uint32_t *err_subleaf); > + > #endif /* !XEN_LIB_X86_CPUID_H */ > > /* > diff --git a/xen/lib/x86/cpuid.c b/xen/lib/x86/cpuid.c > index 5a3159b..7fc4148 100644 > --- a/xen/lib/x86/cpuid.c > +++ b/xen/lib/x86/cpuid.c > @@ -233,6 +233,112 @@ int x86_cpuid_copy_to_buffer(const struct cpuid_policy > *p, > return 0; > } > > +int x86_cpuid_copy_from_buffer(struct cpuid_policy *p, > + const cpuid_leaf_buffer_t leaves, > + uint32_t nr_entries, uint32_t *err_leaf, > + uint32_t *err_subleaf) > +{ > + unsigned int i; > + xen_cpuid_leaf_t data; > + struct cpuid_leaf *l = (void *)&data.a; I'd find this cast a little less worrying if you used container_of(). But even then I dislike this well hidden assumption of similar layouts of struct cpuid_leaf and the latter parts of struct xen_cpuid_leaf. Also it looks as if this could be a pointer to const. > + /* > + * A well formed caller is expected pass an array with leaves in order, ... expected to pass ... > + * and without any repetitions. However, due to per-vendor differences, > + * and in the case of upgrade or levelled scenarios, we typically expect > + * fewer than MAX leaves to be passed. > + * > + * Detecting repeated entries is prohibitively complicated, so we don't > + * bother. That said, one way or another if more than MAX leaves are > + * passed, something is wrong. > + */ > + if ( nr_entries > CPUID_MAX_SERIALISED_LEAVES ) > + return -E2BIG; > + > + for ( i = 0; i < nr_entries; ++i ) > + { > + if ( copy_from_buffer_offset(&data, leaves, i, 1) ) > + return -EFAULT; > + > + switch ( data.leaf ) > + { > + case 0 ... ARRAY_SIZE(p->basic.raw) - 1: > + switch ( data.leaf ) > + { > + case 0x4: > + if ( data.subleaf >= ARRAY_SIZE(p->cache.raw) ) > + goto out_of_range; > + > + p->cache.raw[data.subleaf] = *l; Do you not want to use array_index_nospec() here and below? > + break; > + > + case 0x7: > + if ( data.subleaf >= ARRAY_SIZE(p->feat.raw) ) > + goto out_of_range; > + > + p->feat.raw[data.subleaf] = *l; > + break; > + > + case 0xb: > + if ( data.subleaf >= ARRAY_SIZE(p->topo.raw) ) > + goto out_of_range; > + > + p->topo.raw[data.subleaf] = *l; > + break; > + > + case 0xd: > + if ( data.subleaf >= ARRAY_SIZE(p->xstate.raw) ) > + goto out_of_range; > + > + p->xstate.raw[data.subleaf] = *l; > + break; > + > + default: > + if ( data.subleaf != XEN_CPUID_NO_SUBLEAF ) > + goto out_of_range; > + > + p->basic.raw[data.leaf] = *l; > + break; > + } > + break; > + > + case 0x40000000: > + if ( data.subleaf != XEN_CPUID_NO_SUBLEAF ) > + goto out_of_range; > + > + p->hv_limit = l->a; > + break; > + > + case 0x40000100: > + if ( data.subleaf != XEN_CPUID_NO_SUBLEAF ) > + goto out_of_range; > + > + p->hv2_limit = l->a; > + break; > + > + case 0x80000000 ... 0x80000000 + ARRAY_SIZE(p->extd.raw) - 1: > + if ( data.subleaf != XEN_CPUID_NO_SUBLEAF ) > + goto out_of_range; > + > + p->extd.raw[data.leaf & 0xffff] = *l; > + break; > + > + default: > + goto out_of_range; Any chance I could talk you into moving the label right here, eliminating the ugly (to me at least) error handling code after the main return point of the function? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] libx86: Introduce a helper to deserialise cpuid_policy objects 2019-01-07 10:52 ` Jan Beulich @ 2019-02-25 11:41 ` Andrew Cooper 0 siblings, 0 replies; 10+ messages in thread From: Andrew Cooper @ 2019-02-25 11:41 UTC (permalink / raw) To: Jan Beulich; +Cc: Sergey Dyasli, Xen-devel, Wei Liu, Roger Pau Monne On 07/01/2019 10:52, Jan Beulich wrote: >> diff --git a/xen/lib/x86/cpuid.c b/xen/lib/x86/cpuid.c >> index 5a3159b..7fc4148 100644 >> --- a/xen/lib/x86/cpuid.c >> +++ b/xen/lib/x86/cpuid.c >> @@ -233,6 +233,112 @@ int x86_cpuid_copy_to_buffer(const struct cpuid_policy >> *p, >> return 0; >> } >> >> +int x86_cpuid_copy_from_buffer(struct cpuid_policy *p, >> + const cpuid_leaf_buffer_t leaves, >> + uint32_t nr_entries, uint32_t *err_leaf, >> + uint32_t *err_subleaf) >> +{ >> + unsigned int i; >> + xen_cpuid_leaf_t data; >> + struct cpuid_leaf *l = (void *)&data.a; > I'd find this cast a little less worrying if you used container_of(). But > even then I dislike this well hidden assumption of similar layouts > of struct cpuid_leaf and the latter parts of struct xen_cpuid_leaf. > > Also it looks as if this could be a pointer to const. I've found a different way of doing this which actually compiles smaller (contrary to expectation). >> + /* >> + * A well formed caller is expected pass an array with leaves in order, > ... expected to pass ... Fixed. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] libx86: introduce a helper to deserialise msr_policy objects 2019-01-04 15:33 [PATCH 0/3] libx86: Remaining serialisation logic Andrew Cooper 2019-01-04 15:33 ` [PATCH 1/3] libx86: Introduce a helper to deserialise cpuid_policy objects Andrew Cooper @ 2019-01-04 15:33 ` Andrew Cooper 2019-01-07 10:55 ` Jan Beulich 2019-01-04 15:33 ` [PATCH 3/3] tools/cpu-policy: Add unit tests and a fuzzing harness Andrew Cooper 2 siblings, 1 reply; 10+ messages in thread From: Andrew Cooper @ 2019-01-04 15:33 UTC (permalink / raw) To: Xen-devel Cc: Sergey Dyasli, Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné From: Roger Pau Monné <roger.pau@citrix.com> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wei.liu2@citrix.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Sergey Dyasli <sergey.dyasli@citrix.com> --- xen/include/xen/lib/x86/msr.h | 21 ++++++++++++++ xen/lib/x86/msr.c | 67 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/xen/include/xen/lib/x86/msr.h b/xen/include/xen/lib/x86/msr.h index e2cfbb1..6236622 100644 --- a/xen/include/xen/lib/x86/msr.h +++ b/xen/include/xen/lib/x86/msr.h @@ -48,6 +48,27 @@ typedef xen_msr_entry_t msr_entry_buffer_t[]; int x86_msr_copy_to_buffer(const struct msr_policy *policy, msr_entry_buffer_t msrs, uint32_t *nr_entries); +/** + * Unserialise an msr_policy object from an array of msrs. + * + * @param policy The msr_policy object to unserialise into. + * @param msrs The array of msrs to unserialise from. + * @param nr_entries The number of entries in 'msrs'. + * @param err_msr Optional hint filled on error. + * @returns -errno + * + * Reads at most MSR_MAX_SERIALISED_ENTRIES. May fail for a number of reasons + * based on the content in an individual 'msrs' entry, including the MSR index + * not being valid in the policy, the flags field being nonzero, or if the + * value provided would truncate when stored in the policy. In such cases, + * the optional err_* pointer is filled in to aid diagnostics. + * + * No content validation is performed on the data stored in the policy object. + */ +int x86_msr_copy_from_buffer(struct msr_policy *policy, + const msr_entry_buffer_t msrs, uint32_t nr_entries, + uint32_t *err_msr); + #endif /* !XEN_LIB_X86_MSR_H */ /* diff --git a/xen/lib/x86/msr.c b/xen/lib/x86/msr.c index 60fb567..e498124 100644 --- a/xen/lib/x86/msr.c +++ b/xen/lib/x86/msr.c @@ -47,6 +47,73 @@ int x86_msr_copy_to_buffer(const struct msr_policy *p, return 0; } +int x86_msr_copy_from_buffer(struct msr_policy *p, + const msr_entry_buffer_t msrs, uint32_t nr_entries, + uint32_t *err_msr) +{ + unsigned int i; + xen_msr_entry_t data; + int rc; + + /* + * A well formed caller is expected pass an array with entries in order, + * and without any repetitions. However, due to per-vendor differences, + * and in the case of upgrade or levelled scenarios, we typically expect + * fewer than MAX entries to be passed. + * + * Detecting repeated entries is prohibitively complicated, so we don't + * bother. That said, one way or another if more than MAX entries are + * passed, something is wrong. + */ + if ( nr_entries > MSR_MAX_SERIALISED_ENTRIES ) + return -E2BIG; + + for ( i = 0; i < nr_entries; i++ ) + { + if ( copy_from_buffer_offset(&data, msrs, i, 1) ) + return -EFAULT; + + if ( data.flags ) /* .flags MBZ */ + { + rc = -EINVAL; + goto err; + } + + switch ( data.idx ) + { + /* + * Assign data.val to 'field', checking for truncation if the + * backing storage for 'field' is smaller than uint64_t + */ +#define ASSIGN(field) \ +({ \ + if ( (typeof(field))data.val != data.val ) \ + { \ + rc = -EOVERFLOW; \ + goto err; \ + } \ + field = data.val; \ +}) + + case MSR_INTEL_PLATFORM_INFO: ASSIGN(p->plaform_info.raw); break; + +#undef ASSIGN + + default: + rc = -ERANGE; + goto err; + } + } + + return 0; + + err: + if ( err_msr ) + *err_msr = data.idx; + + return rc; +} + /* * Local variables: * mode: C -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] libx86: introduce a helper to deserialise msr_policy objects 2019-01-04 15:33 ` [PATCH 2/3] libx86: introduce a helper to deserialise msr_policy objects Andrew Cooper @ 2019-01-07 10:55 ` Jan Beulich 0 siblings, 0 replies; 10+ messages in thread From: Jan Beulich @ 2019-01-07 10:55 UTC (permalink / raw) To: Andrew Cooper; +Cc: Sergey Dyasli, Xen-devel, Wei Liu, Roger Pau Monne >>> On 04.01.19 at 16:33, <andrew.cooper3@citrix.com> wrote: > From: Roger Pau Monné <roger.pau@citrix.com> > > Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Where applicable, same comments here as for patch 1. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] tools/cpu-policy: Add unit tests and a fuzzing harness 2019-01-04 15:33 [PATCH 0/3] libx86: Remaining serialisation logic Andrew Cooper 2019-01-04 15:33 ` [PATCH 1/3] libx86: Introduce a helper to deserialise cpuid_policy objects Andrew Cooper 2019-01-04 15:33 ` [PATCH 2/3] libx86: introduce a helper to deserialise msr_policy objects Andrew Cooper @ 2019-01-04 15:33 ` Andrew Cooper 2019-01-07 11:19 ` Jan Beulich 2 siblings, 1 reply; 10+ messages in thread From: Andrew Cooper @ 2019-01-04 15:33 UTC (permalink / raw) To: Xen-devel Cc: Andrew Cooper, Sergey Dyasli, Wei Liu, Jan Beulich, Roger Pau Monné The AFL harness currently notices that there are cases where we optimse the serialised stream by omitting data beyond the various maximum leaves. Both sets of tests will be extended with further libx86 work. Fix the sorting of the CPUID_GUEST_NR_* constants, noticed while writing the unit tests. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wei.liu2@citrix.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Sergey Dyasli <sergey.dyasli@citrix.com> --- tools/fuzz/cpu-policy/.gitignore | 1 + tools/fuzz/cpu-policy/Makefile | 27 ++++ tools/fuzz/cpu-policy/afl-policy-fuzzer.c | 117 ++++++++++++++ tools/tests/Makefile | 1 + tools/tests/cpu-policy/.gitignore | 1 + tools/tests/cpu-policy/Makefile | 27 ++++ tools/tests/cpu-policy/test-cpu-policy.c | 247 ++++++++++++++++++++++++++++++ xen/include/xen/lib/x86/cpuid.h | 2 +- 8 files changed, 422 insertions(+), 1 deletion(-) create mode 100644 tools/fuzz/cpu-policy/.gitignore create mode 100644 tools/fuzz/cpu-policy/Makefile create mode 100644 tools/fuzz/cpu-policy/afl-policy-fuzzer.c create mode 100644 tools/tests/cpu-policy/.gitignore create mode 100644 tools/tests/cpu-policy/Makefile create mode 100644 tools/tests/cpu-policy/test-cpu-policy.c diff --git a/tools/fuzz/cpu-policy/.gitignore b/tools/fuzz/cpu-policy/.gitignore new file mode 100644 index 0000000..b0e0bdf --- /dev/null +++ b/tools/fuzz/cpu-policy/.gitignore @@ -0,0 +1 @@ +afl-policy-fuzzer diff --git a/tools/fuzz/cpu-policy/Makefile b/tools/fuzz/cpu-policy/Makefile new file mode 100644 index 0000000..a25778b --- /dev/null +++ b/tools/fuzz/cpu-policy/Makefile @@ -0,0 +1,27 @@ +XEN_ROOT = $(CURDIR)/../../.. +include $(XEN_ROOT)/tools/Rules.mk + +.PHONY: all +all: afl-policy-fuzzer + +.PHONY: clean +clean: + $(RM) -f -- *.o .*.d .*.d2 afl-policy-fuzzer + +.PHONY: distclean +distclean: clean + $(RM) -f -- *~ + +.PHONY: install +install: all + +.PHONY: uninstall + +CFLAGS += -Werror $(CFLAGS_xeninclude) -D__XEN_TOOLS__ + +vpath %.c ../../../xen/lib/x86 + +afl-policy-fuzzer: afl-policy-fuzzer.o msr.o cpuid.o + $(CC) $(CFLAGS) $^ -o $@ + +-include $(DEPS_INCLUDE) diff --git a/tools/fuzz/cpu-policy/afl-policy-fuzzer.c b/tools/fuzz/cpu-policy/afl-policy-fuzzer.c new file mode 100644 index 0000000..557df81 --- /dev/null +++ b/tools/fuzz/cpu-policy/afl-policy-fuzzer.c @@ -0,0 +1,117 @@ +#include <assert.h> +#include <stdbool.h> +#include <stdint.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +#include <xen-tools/libs.h> +#include <xen/lib/x86/cpuid.h> +#include <xen/lib/x86/msr.h> +#include <xen/domctl.h> + +void check_cpuid(const struct cpuid_policy *cp) +{ + struct cpuid_policy new = {}; + xen_cpuid_leaf_t *leaves = malloc(CPUID_MAX_SERIALISED_LEAVES * + sizeof(xen_cpuid_leaf_t)); + unsigned int nr = CPUID_MAX_SERIALISED_LEAVES; + int rc; + + if ( !leaves ) + return; + + rc = x86_cpuid_copy_to_buffer(cp, leaves, &nr); + assert(rc == 0); + assert(nr <= CPUID_MAX_SERIALISED_LEAVES); + + rc = x86_cpuid_copy_from_buffer(&new, leaves, nr, NULL, NULL); + assert(rc == 0); + assert(memcmp(cp, &new, sizeof(*cp)) == 0); + + free(leaves); +} + +void check_msr(const struct msr_policy *mp) +{ + struct msr_policy new = {}; + xen_msr_entry_t *msrs = malloc(MSR_MAX_SERIALISED_ENTRIES * + sizeof(xen_msr_entry_t)); + unsigned int nr = MSR_MAX_SERIALISED_ENTRIES; + int rc; + + if ( !msrs ) + return; + + rc = x86_msr_copy_to_buffer(mp, msrs, &nr); + assert(rc == 0); + assert(nr <= MSR_MAX_SERIALISED_ENTRIES); + + rc = x86_msr_copy_from_buffer(&new, msrs, nr, NULL); + assert(rc == 0); + assert(memcmp(mp, &new, sizeof(*mp)) == 0); + + free(msrs); +} + +int main(int argc, char **argv) +{ + FILE *fp = NULL; + struct cpuid_policy *cp = NULL; + struct msr_policy *mp = NULL; + + setbuf(stdin, NULL); + setbuf(stdout, NULL); + + if ( argc == 1 ) + { + printf("Using stdin\n"); + fp = stdin; + } + +#ifdef __AFL_HAVE_MANUAL_CONTROL + __AFL_INIT(); + while ( __AFL_LOOP(1000) ) +#endif + { + if ( fp != stdin ) + { + printf("Opening file '%s'\n", argv[1]); + fp = fopen(argv[1], "rb"); + + if ( !fp ) + { + perror("fopen"); + exit(-1); + } + } + + cp = calloc(1, sizeof(*cp)); + mp = calloc(1, sizeof(*mp)); + if ( !cp || !mp ) + goto skip; + + fread(cp, sizeof(*cp), 1, fp); + fread(mp, sizeof(*mp), 1, fp); + + if ( !feof(fp) ) + goto skip; + + check_cpuid(cp); + check_msr(mp); + + skip: + free(cp); + cp = NULL; + free(mp); + mp = NULL; + + if ( fp != stdin ) + { + fclose(fp); + fp = NULL; + } + } + + return 0; +} diff --git a/tools/tests/Makefile b/tools/tests/Makefile index a9fc50d..067a380 100644 --- a/tools/tests/Makefile +++ b/tools/tests/Makefile @@ -5,6 +5,7 @@ CFLAGS += $(CFLAGS_libxenctrl) LDLIBS += $(LDLIBS_libxenctrl) SUBDIRS-y := +SUBDIRS-$(CONFIG_X86) += cpu-policy SUBDIRS-$(CONFIG_X86) += mce-test SUBDIRS-y += mem-sharing ifeq ($(XEN_TARGET_ARCH),__fixme__) diff --git a/tools/tests/cpu-policy/.gitignore b/tools/tests/cpu-policy/.gitignore new file mode 100644 index 0000000..83bdb6b --- /dev/null +++ b/tools/tests/cpu-policy/.gitignore @@ -0,0 +1 @@ +test-cpu-policy diff --git a/tools/tests/cpu-policy/Makefile b/tools/tests/cpu-policy/Makefile new file mode 100644 index 0000000..a029165 --- /dev/null +++ b/tools/tests/cpu-policy/Makefile @@ -0,0 +1,27 @@ +XEN_ROOT = $(CURDIR)/../../.. +include $(XEN_ROOT)/tools/Rules.mk + +.PHONY: all +all: test-cpu-policy + +.PHONY: clean +clean: + $(RM) -f -- *.o .*.d .*.d2 test-cpu-policy + +.PHONY: distclean +distclean: clean + $(RM) -f -- *~ + +.PHONY: install +install: all + +.PHONY: uninstall + +CFLAGS += -Werror $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -O3 + +vpath %.c ../../../xen/lib/x86 + +test-cpu-policy: test-cpu-policy.o msr.o cpuid.o + $(CC) $(CFLAGS) $^ -o $@ + +-include $(DEPS_INCLUDE) diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c new file mode 100644 index 0000000..7414ac7 --- /dev/null +++ b/tools/tests/cpu-policy/test-cpu-policy.c @@ -0,0 +1,247 @@ +#include <assert.h> +#include <errno.h> +#include <stdbool.h> +#include <stdint.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +#include <xen-tools/libs.h> +#include <xen/lib/x86/cpuid.h> +#include <xen/lib/x86/msr.h> +#include <xen/domctl.h> + +static void test_cpuid_serialise_success(void) +{ + static const struct test { + struct cpuid_policy p; + const char *name; + unsigned int nr_leaves; + } tests[] = { + { + .name = "empty policy", + .nr_leaves = 4, + }, + }; + unsigned int i; + + printf("Testing CPUID serialise success:\n"); + + for ( i = 0; i < ARRAY_SIZE(tests); ++i ) + { + const struct test *t = &tests[i]; + unsigned int nr = t->nr_leaves; + xen_cpuid_leaf_t *leaves = malloc(nr * sizeof(*leaves)); + int rc; + + if ( !leaves ) + goto test_done; + + rc = x86_cpuid_copy_to_buffer(&t->p, leaves, &nr); + + if ( rc != 0 ) + { + printf(" Test %s, expected rc 0, got %d\n", + t->name, rc); + goto test_done; + } + + if ( nr != t->nr_leaves ) + { + printf(" Test %s, expected %u leaves, got %u\n", + t->name, t->nr_leaves, nr); + goto test_done; + } + + test_done: + free(leaves); + } +} + +static void test_msr_serialise_success(void) +{ + static const struct test { + struct msr_policy p; + const char *name; + unsigned int nr_msrs; + } tests[] = { + { + .name = "empty policy", + .nr_msrs = MSR_MAX_SERIALISED_ENTRIES, + }, + }; + unsigned int i; + + printf("Testing MSR serialise success:\n"); + + for ( i = 0; i < ARRAY_SIZE(tests); ++i ) + { + const struct test *t = &tests[i]; + unsigned int nr = t->nr_msrs; + xen_msr_entry_t *msrs = malloc(nr * sizeof(*msrs)); + int rc; + + if ( !msrs ) + goto test_done; + + rc = x86_msr_copy_to_buffer(&t->p, msrs, &nr); + + if ( rc != 0 ) + { + printf(" Test %s, expected rc 0, got %d\n", + t->name, rc); + goto test_done; + } + + if ( nr != t->nr_msrs ) + { + printf(" Test %s, expected %u msrs, got %u\n", + t->name, t->nr_msrs, nr); + goto test_done; + } + + test_done: + free(msrs); + } +} + +static void test_cpuid_deserialise_failure(void) +{ + static const struct test { + const char *name; + xen_cpuid_leaf_t leaf; + } tests[] = { + { + .name = "incorrect basic subleaf", + .leaf = { .leaf = 0, .subleaf = 0 }, + }, + { + .name = "incorrect hv1 subleaf", + .leaf = { .leaf = 0x40000000, .subleaf = 0 }, + }, + { + .name = "incorrect hv2 subleaf", + .leaf = { .leaf = 0x40000100, .subleaf = 0 }, + }, + { + .name = "incorrect extd subleaf", + .leaf = { .leaf = 0x80000000, .subleaf = 0 }, + }, + { + .name = "OoB basic leaf", + .leaf = { .leaf = CPUID_GUEST_NR_BASIC }, + }, + { + .name = "OoB cache leaf", + .leaf = { .leaf = 0x4, .subleaf = CPUID_GUEST_NR_CACHE }, + }, + { + .name = "OoB feat leaf", + .leaf = { .leaf = 0x7, .subleaf = CPUID_GUEST_NR_FEAT }, + }, + { + .name = "OoB topo leaf", + .leaf = { .leaf = 0xb, .subleaf = CPUID_GUEST_NR_TOPO }, + }, + { + .name = "OoB xstate leaf", + .leaf = { .leaf = 0xd, .subleaf = CPUID_GUEST_NR_XSTATE }, + }, + { + .name = "OoB extd leaf", + .leaf = { .leaf = 0x80000000 | CPUID_GUEST_NR_EXTD }, + }, + }; + unsigned int i; + + printf("Testing CPUID deserialise failure:\n"); + + for ( i = 0; i < ARRAY_SIZE(tests); ++i ) + { + const struct test *t = &tests[i]; + uint32_t err_leaf = ~0u, err_subleaf = ~0u; + int rc; + + rc = x86_cpuid_copy_from_buffer(NULL, &t->leaf, 1, + &err_leaf, &err_subleaf); + + if ( rc != -ERANGE ) + { + printf(" Test %s, expected rc %d, got %d\n", + t->name, -ERANGE, rc); + continue; + } + + if ( err_leaf != t->leaf.leaf || err_subleaf != t->leaf.subleaf ) + { + printf(" Test %s, expected err %08x:%08x, got %08x:%08x\n", + t->name, t->leaf.leaf, t->leaf.subleaf, + err_leaf, err_subleaf); + continue; + } + } +} + +static void test_msr_deserialise_failure(void) +{ + static const struct test { + const char *name; + xen_msr_entry_t msr; + int rc; + } tests[] = { + { + .name = "bad msr index", + .msr = { .idx = 0xdeadc0de }, + .rc = -ERANGE, + }, + { + .name = "nonzero flags", + .msr = { .idx = 0xce, .flags = 1 }, + .rc = -EINVAL, + }, + { + .name = "truncated val", + .msr = { .idx = 0xce, .val = ~0ull }, + .rc = -EOVERFLOW, + }, + }; + unsigned int i; + + printf("Testing MSR deserialise failure:\n"); + + for ( i = 0; i < ARRAY_SIZE(tests); ++i ) + { + const struct test *t = &tests[i]; + uint32_t err_msr = ~0u; + int rc; + + rc = x86_msr_copy_from_buffer(NULL, &t->msr, 1, &err_msr); + + if ( rc != t->rc ) + { + printf(" Test %s, expected rc %d, got %d\n", + t->name, t->rc, rc); + continue; + } + + if ( err_msr != t->msr.idx ) + { + printf(" Test %s, expected err_msr %#x, got %#x\n", + t->name, t->msr.idx, err_msr); + continue; + } + } +} + +int main(int argc, char **argv) +{ + printf("CPU Policy unit tests\n"); + + test_cpuid_serialise_success(); + test_msr_serialise_success(); + + test_cpuid_deserialise_failure(); + test_msr_deserialise_failure(); + + return 0; +} diff --git a/xen/include/xen/lib/x86/cpuid.h b/xen/include/xen/lib/x86/cpuid.h index 767a33b..95b37b6 100644 --- a/xen/include/xen/lib/x86/cpuid.h +++ b/xen/include/xen/lib/x86/cpuid.h @@ -66,8 +66,8 @@ static inline void cpuid_count_leaf( #undef XCHG #define CPUID_GUEST_NR_BASIC (0xdu + 1) -#define CPUID_GUEST_NR_FEAT (0u + 1) #define CPUID_GUEST_NR_CACHE (5u + 1) +#define CPUID_GUEST_NR_FEAT (0u + 1) #define CPUID_GUEST_NR_TOPO (1u + 1) #define CPUID_GUEST_NR_XSTATE (62u + 1) #define CPUID_GUEST_NR_EXTD_INTEL (0x8u + 1) -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] tools/cpu-policy: Add unit tests and a fuzzing harness 2019-01-04 15:33 ` [PATCH 3/3] tools/cpu-policy: Add unit tests and a fuzzing harness Andrew Cooper @ 2019-01-07 11:19 ` Jan Beulich 2019-02-25 11:56 ` Andrew Cooper 0 siblings, 1 reply; 10+ messages in thread From: Jan Beulich @ 2019-01-07 11:19 UTC (permalink / raw) To: Andrew Cooper; +Cc: Sergey Dyasli, Xen-devel, Wei Liu, Roger Pau Monne >>> On 04.01.19 at 16:33, <andrew.cooper3@citrix.com> wrote: > The AFL harness currently notices that there are cases where we optimse the > serialised stream by omitting data beyond the various maximum leaves. > > Both sets of tests will be extended with further libx86 work. > > Fix the sorting of the CPUID_GUEST_NR_* constants, noticed while writing the > unit tests. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Wei Liu <wei.liu2@citrix.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Sergey Dyasli <sergey.dyasli@citrix.com> > --- > tools/fuzz/cpu-policy/.gitignore | 1 + > tools/fuzz/cpu-policy/Makefile | 27 ++++ > tools/fuzz/cpu-policy/afl-policy-fuzzer.c | 117 ++++++++++++++ > tools/tests/Makefile | 1 + > tools/tests/cpu-policy/.gitignore | 1 + Did we somehow come to the conclusion that the central .gitignore at the root of the tree is not the way to go in the future? > --- /dev/null > +++ b/tools/tests/cpu-policy/test-cpu-policy.c > @@ -0,0 +1,247 @@ > +#include <assert.h> > +#include <errno.h> > +#include <stdbool.h> > +#include <stdint.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <string.h> > + > +#include <xen-tools/libs.h> > +#include <xen/lib/x86/cpuid.h> > +#include <xen/lib/x86/msr.h> > +#include <xen/domctl.h> > + > +static void test_cpuid_serialise_success(void) > +{ > + static const struct test { > + struct cpuid_policy p; > + const char *name; > + unsigned int nr_leaves; > + } tests[] = { > + { > + .name = "empty policy", > + .nr_leaves = 4, > + }, > + }; > + unsigned int i; > + > + printf("Testing CPUID serialise success:\n"); > + > + for ( i = 0; i < ARRAY_SIZE(tests); ++i ) > + { > + const struct test *t = &tests[i]; > + unsigned int nr = t->nr_leaves; > + xen_cpuid_leaf_t *leaves = malloc(nr * sizeof(*leaves)); > + int rc; > + > + if ( !leaves ) > + goto test_done; Shouldn't you leave some indication of the test not having got run? > +static void test_cpuid_deserialise_failure(void) > +{ > + static const struct test { > + const char *name; > + xen_cpuid_leaf_t leaf; > + } tests[] = { > + { > + .name = "incorrect basic subleaf", > + .leaf = { .leaf = 0, .subleaf = 0 }, > + }, > + { > + .name = "incorrect hv1 subleaf", > + .leaf = { .leaf = 0x40000000, .subleaf = 0 }, > + }, > + { > + .name = "incorrect hv2 subleaf", > + .leaf = { .leaf = 0x40000100, .subleaf = 0 }, > + }, > + { > + .name = "incorrect extd subleaf", > + .leaf = { .leaf = 0x80000000, .subleaf = 0 }, > + }, > + { > + .name = "OoB basic leaf", > + .leaf = { .leaf = CPUID_GUEST_NR_BASIC }, > + }, > + { > + .name = "OoB cache leaf", > + .leaf = { .leaf = 0x4, .subleaf = CPUID_GUEST_NR_CACHE }, > + }, > + { > + .name = "OoB feat leaf", > + .leaf = { .leaf = 0x7, .subleaf = CPUID_GUEST_NR_FEAT }, > + }, > + { > + .name = "OoB topo leaf", > + .leaf = { .leaf = 0xb, .subleaf = CPUID_GUEST_NR_TOPO }, > + }, > + { > + .name = "OoB xstate leaf", > + .leaf = { .leaf = 0xd, .subleaf = CPUID_GUEST_NR_XSTATE }, > + }, > + { > + .name = "OoB extd leaf", > + .leaf = { .leaf = 0x80000000 | CPUID_GUEST_NR_EXTD }, > + }, > + }; > + unsigned int i; > + > + printf("Testing CPUID deserialise failure:\n"); > + > + for ( i = 0; i < ARRAY_SIZE(tests); ++i ) > + { > + const struct test *t = &tests[i]; > + uint32_t err_leaf = ~0u, err_subleaf = ~0u; > + int rc; > + > + rc = x86_cpuid_copy_from_buffer(NULL, &t->leaf, 1, > + &err_leaf, &err_subleaf); > + > + if ( rc != -ERANGE ) > + { > + printf(" Test %s, expected rc %d, got %d\n", > + t->name, -ERANGE, rc); > + continue; Perhaps drop this? The subsequent test ought to apply regardless of error code. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] tools/cpu-policy: Add unit tests and a fuzzing harness 2019-01-07 11:19 ` Jan Beulich @ 2019-02-25 11:56 ` Andrew Cooper 2019-02-25 12:23 ` Jan Beulich 0 siblings, 1 reply; 10+ messages in thread From: Andrew Cooper @ 2019-02-25 11:56 UTC (permalink / raw) To: Jan Beulich; +Cc: Sergey Dyasli, Xen-devel, Wei Liu, Roger Pau Monne [-- Attachment #1.1: Type: text/plain, Size: 5184 bytes --] On 07/01/2019 11:19, Jan Beulich wrote: >>>> On 04.01.19 at 16:33, <andrew.cooper3@citrix.com> wrote: >> The AFL harness currently notices that there are cases where we optimse the >> serialised stream by omitting data beyond the various maximum leaves. >> >> Both sets of tests will be extended with further libx86 work. >> >> Fix the sorting of the CPUID_GUEST_NR_* constants, noticed while writing the >> unit tests. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Wei Liu <wei.liu2@citrix.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Sergey Dyasli <sergey.dyasli@citrix.com> >> --- >> tools/fuzz/cpu-policy/.gitignore | 1 + >> tools/fuzz/cpu-policy/Makefile | 27 ++++ >> tools/fuzz/cpu-policy/afl-policy-fuzzer.c | 117 ++++++++++++++ >> tools/tests/Makefile | 1 + >> tools/tests/cpu-policy/.gitignore | 1 + > Did we somehow come to the conclusion that the central .gitignore > at the root of the tree is not the way to go in the future? We've already got several examples in the tree. andrewcoop@andrewcoop:/local/xen.git$ git ls-files | grep gitignore .gitignore tools/tests/vhpet/.gitignore xen/tools/kconfig/.gitignore xen/tools/kconfig/lxdialog/.gitignore xen/xsm/flask/.gitignore As for the pro's of using split ignores, fewer collisions for backported changes, and no forgetting to update the root .gitconfig when you move directories. > >> --- /dev/null >> +++ b/tools/tests/cpu-policy/test-cpu-policy.c >> @@ -0,0 +1,247 @@ >> +#include <assert.h> >> +#include <errno.h> >> +#include <stdbool.h> >> +#include <stdint.h> >> +#include <stdio.h> >> +#include <stdlib.h> >> +#include <string.h> >> + >> +#include <xen-tools/libs.h> >> +#include <xen/lib/x86/cpuid.h> >> +#include <xen/lib/x86/msr.h> >> +#include <xen/domctl.h> >> + >> +static void test_cpuid_serialise_success(void) >> +{ >> + static const struct test { >> + struct cpuid_policy p; >> + const char *name; >> + unsigned int nr_leaves; >> + } tests[] = { >> + { >> + .name = "empty policy", >> + .nr_leaves = 4, >> + }, >> + }; >> + unsigned int i; >> + >> + printf("Testing CPUID serialise success:\n"); >> + >> + for ( i = 0; i < ARRAY_SIZE(tests); ++i ) >> + { >> + const struct test *t = &tests[i]; >> + unsigned int nr = t->nr_leaves; >> + xen_cpuid_leaf_t *leaves = malloc(nr * sizeof(*leaves)); >> + int rc; >> + >> + if ( !leaves ) >> + goto test_done; > Shouldn't you leave some indication of the test not having got run? I've swapped this for a hard error. Its not going to fail in practice. > >> +static void test_cpuid_deserialise_failure(void) >> +{ >> + static const struct test { >> + const char *name; >> + xen_cpuid_leaf_t leaf; >> + } tests[] = { >> + { >> + .name = "incorrect basic subleaf", >> + .leaf = { .leaf = 0, .subleaf = 0 }, >> + }, >> + { >> + .name = "incorrect hv1 subleaf", >> + .leaf = { .leaf = 0x40000000, .subleaf = 0 }, >> + }, >> + { >> + .name = "incorrect hv2 subleaf", >> + .leaf = { .leaf = 0x40000100, .subleaf = 0 }, >> + }, >> + { >> + .name = "incorrect extd subleaf", >> + .leaf = { .leaf = 0x80000000, .subleaf = 0 }, >> + }, >> + { >> + .name = "OoB basic leaf", >> + .leaf = { .leaf = CPUID_GUEST_NR_BASIC }, >> + }, >> + { >> + .name = "OoB cache leaf", >> + .leaf = { .leaf = 0x4, .subleaf = CPUID_GUEST_NR_CACHE }, >> + }, >> + { >> + .name = "OoB feat leaf", >> + .leaf = { .leaf = 0x7, .subleaf = CPUID_GUEST_NR_FEAT }, >> + }, >> + { >> + .name = "OoB topo leaf", >> + .leaf = { .leaf = 0xb, .subleaf = CPUID_GUEST_NR_TOPO }, >> + }, >> + { >> + .name = "OoB xstate leaf", >> + .leaf = { .leaf = 0xd, .subleaf = CPUID_GUEST_NR_XSTATE }, >> + }, >> + { >> + .name = "OoB extd leaf", >> + .leaf = { .leaf = 0x80000000 | CPUID_GUEST_NR_EXTD }, >> + }, >> + }; >> + unsigned int i; >> + >> + printf("Testing CPUID deserialise failure:\n"); >> + >> + for ( i = 0; i < ARRAY_SIZE(tests); ++i ) >> + { >> + const struct test *t = &tests[i]; >> + uint32_t err_leaf = ~0u, err_subleaf = ~0u; >> + int rc; >> + >> + rc = x86_cpuid_copy_from_buffer(NULL, &t->leaf, 1, >> + &err_leaf, &err_subleaf); >> + >> + if ( rc != -ERANGE ) >> + { >> + printf(" Test %s, expected rc %d, got %d\n", >> + t->name, -ERANGE, rc); >> + continue; > Perhaps drop this? The subsequent test ought to apply regardless > of error code. The common case is no failures at all. However, once something has gone wrong, spewing cascade errors gets in the way, rather than being helpful. ~Andrew [-- Attachment #1.2: Type: text/html, Size: 6856 bytes --] [-- Attachment #2: Type: text/plain, Size: 157 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] tools/cpu-policy: Add unit tests and a fuzzing harness 2019-02-25 11:56 ` Andrew Cooper @ 2019-02-25 12:23 ` Jan Beulich 0 siblings, 0 replies; 10+ messages in thread From: Jan Beulich @ 2019-02-25 12:23 UTC (permalink / raw) To: Andrew Cooper; +Cc: Sergey Dyasli, Xen-devel, Wei Liu, Roger Pau Monne >>> On 25.02.19 at 12:56, <andrew.cooper3@citrix.com> wrote: > On 07/01/2019 11:19, Jan Beulich wrote: >>>>> On 04.01.19 at 16:33, <andrew.cooper3@citrix.com> wrote: >>> The AFL harness currently notices that there are cases where we optimse the >>> serialised stream by omitting data beyond the various maximum leaves. >>> >>> Both sets of tests will be extended with further libx86 work. >>> >>> Fix the sorting of the CPUID_GUEST_NR_* constants, noticed while writing the >>> unit tests. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> --- >>> CC: Jan Beulich <JBeulich@suse.com> >>> CC: Wei Liu <wei.liu2@citrix.com> >>> CC: Roger Pau Monné <roger.pau@citrix.com> >>> CC: Sergey Dyasli <sergey.dyasli@citrix.com> >>> --- >>> tools/fuzz/cpu-policy/.gitignore | 1 + >>> tools/fuzz/cpu-policy/Makefile | 27 ++++ >>> tools/fuzz/cpu-policy/afl-policy-fuzzer.c | 117 ++++++++++++++ >>> tools/tests/Makefile | 1 + >>> tools/tests/cpu-policy/.gitignore | 1 + >> Did we somehow come to the conclusion that the central .gitignore >> at the root of the tree is not the way to go in the future? > > We've already got several examples in the tree. > > andrewcoop@andrewcoop:/local/xen.git$ git ls-files | grep gitignore > .gitignore > tools/tests/vhpet/.gitignore > xen/tools/kconfig/.gitignore > xen/tools/kconfig/lxdialog/.gitignore > xen/xsm/flask/.gitignore > > As for the pro's of using split ignores, fewer collisions for backported > changes, and no forgetting to update the root .gitconfig when you move > directories. Well, I certainly appreciate this, and my comment wasn't meant as plain opposition. At the time I was simply wondering whether the few instances we have already aren't more like an accident. And I didn't recall any discussion towards moving away from the centralized model. >>> +static void test_cpuid_deserialise_failure(void) >>> +{ >>> + static const struct test { >>> + const char *name; >>> + xen_cpuid_leaf_t leaf; >>> + } tests[] = { >>> + { >>> + .name = "incorrect basic subleaf", >>> + .leaf = { .leaf = 0, .subleaf = 0 }, >>> + }, >>> + { >>> + .name = "incorrect hv1 subleaf", >>> + .leaf = { .leaf = 0x40000000, .subleaf = 0 }, >>> + }, >>> + { >>> + .name = "incorrect hv2 subleaf", >>> + .leaf = { .leaf = 0x40000100, .subleaf = 0 }, >>> + }, >>> + { >>> + .name = "incorrect extd subleaf", >>> + .leaf = { .leaf = 0x80000000, .subleaf = 0 }, >>> + }, >>> + { >>> + .name = "OoB basic leaf", >>> + .leaf = { .leaf = CPUID_GUEST_NR_BASIC }, >>> + }, >>> + { >>> + .name = "OoB cache leaf", >>> + .leaf = { .leaf = 0x4, .subleaf = CPUID_GUEST_NR_CACHE }, >>> + }, >>> + { >>> + .name = "OoB feat leaf", >>> + .leaf = { .leaf = 0x7, .subleaf = CPUID_GUEST_NR_FEAT }, >>> + }, >>> + { >>> + .name = "OoB topo leaf", >>> + .leaf = { .leaf = 0xb, .subleaf = CPUID_GUEST_NR_TOPO }, >>> + }, >>> + { >>> + .name = "OoB xstate leaf", >>> + .leaf = { .leaf = 0xd, .subleaf = CPUID_GUEST_NR_XSTATE }, >>> + }, >>> + { >>> + .name = "OoB extd leaf", >>> + .leaf = { .leaf = 0x80000000 | CPUID_GUEST_NR_EXTD }, >>> + }, >>> + }; >>> + unsigned int i; >>> + >>> + printf("Testing CPUID deserialise failure:\n"); >>> + >>> + for ( i = 0; i < ARRAY_SIZE(tests); ++i ) >>> + { >>> + const struct test *t = &tests[i]; >>> + uint32_t err_leaf = ~0u, err_subleaf = ~0u; >>> + int rc; >>> + >>> + rc = x86_cpuid_copy_from_buffer(NULL, &t->leaf, 1, >>> + &err_leaf, &err_subleaf); >>> + >>> + if ( rc != -ERANGE ) >>> + { >>> + printf(" Test %s, expected rc %d, got %d\n", >>> + t->name, -ERANGE, rc); >>> + continue; >> Perhaps drop this? The subsequent test ought to apply regardless >> of error code. > > The common case is no failures at all. However, once something has gone > wrong, spewing cascade errors gets in the way, rather than being helpful. "Cascade failures" to me are ones where the latter is a result of something earlier having gone wrong. Iirc this isn't the case with the subsequent test(s) here, unless something's fundamentally wrong. Seeing which particular case doesn't work plus all of the ones which do can also be helpful. But as indicated by me using "perhaps" in the original reply - I won't insist, as in the end something will need to be done anyway until everything succeeds. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-02-25 12:23 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-04 15:33 [PATCH 0/3] libx86: Remaining serialisation logic Andrew Cooper 2019-01-04 15:33 ` [PATCH 1/3] libx86: Introduce a helper to deserialise cpuid_policy objects Andrew Cooper 2019-01-07 10:52 ` Jan Beulich 2019-02-25 11:41 ` Andrew Cooper 2019-01-04 15:33 ` [PATCH 2/3] libx86: introduce a helper to deserialise msr_policy objects Andrew Cooper 2019-01-07 10:55 ` Jan Beulich 2019-01-04 15:33 ` [PATCH 3/3] tools/cpu-policy: Add unit tests and a fuzzing harness Andrew Cooper 2019-01-07 11:19 ` Jan Beulich 2019-02-25 11:56 ` Andrew Cooper 2019-02-25 12:23 ` Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.