Re: [PATCH 3/3] tools/cpu-policy: Add unit tests and a fuzzing harness

["Jan Beulich" <JBeulich@suse.com>]

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