[Xen-devel] [PATCH 0/2] libx86: Fuzzing harness

[Xen-devel] [PATCH 0/2] libx86: Fuzzing harness

[Andrew Cooper]

These are the final pieces to getting the fuzzing harness working correctly.

I accidentally left AFL running for a week while I was travelling, so this
certified "8d 15h crash-free".

Andrew Cooper (2):
  libx86: Helper for clearing out-of-range CPUID leaves
  tools/fuzz: Add a cpu-policy fuzzing harness

 tools/fuzz/cpu-policy/.gitignore          |   1 +
 tools/fuzz/cpu-policy/Makefile            |  28 +++++
 tools/fuzz/cpu-policy/afl-policy-fuzzer.c | 187 ++++++++++++++++++++++++++++++
 tools/tests/cpu-policy/test-cpu-policy.c  | 161 ++++++++++++++++++++++++-
 xen/include/xen/lib/x86/cpuid.h           |  16 +++
 xen/lib/x86/cpuid.c                       |  66 ++++++++++-
 xen/lib/x86/private.h                     |   1 +
 7 files changed, 454 insertions(+), 6 deletions(-)
 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

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/2] libx86: Helper for clearing out-of-range CPUID leaves

[Andrew Cooper]

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

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/2] tools/fuzz: Add a cpu-policy fuzzing harness

[Andrew Cooper]

There is now enough complexity that a fuzzing harness is a good idea, and
enough supporting logic to implement one which AFL seems happy with.

Take the existing recalculate_synth() helper and export it as
x86_cpuid_policy_recalc_synth(), as it is needed by the fuzzing harness.

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/fuzz/cpu-policy/.gitignore          |   1 +
 tools/fuzz/cpu-policy/Makefile            |  28 +++++
 tools/fuzz/cpu-policy/afl-policy-fuzzer.c | 187 ++++++++++++++++++++++++++++++
 xen/include/xen/lib/x86/cpuid.h           |   5 +
 xen/lib/x86/cpuid.c                       |   7 +-
 5 files changed, 224 insertions(+), 4 deletions(-)
 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

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..41a2230
--- /dev/null
+++ b/tools/fuzz/cpu-policy/Makefile
@@ -0,0 +1,28 @@
+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__
+CFLAGS += $(APPEND_CFLAGS) -Og
+
+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..bc0cecd
--- /dev/null
+++ b/tools/fuzz/cpu-policy/afl-policy-fuzzer.c
@@ -0,0 +1,187 @@
+#include <assert.h>
+#include <stdbool.h>
+#include <stddef.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <getopt.h>
+
+#include <xen-tools/libs.h>
+#include <xen/lib/x86/cpuid.h>
+#include <xen/lib/x86/msr.h>
+#include <xen/domctl.h>
+
+static bool debug;
+
+#define EMPTY_LEAF ((struct cpuid_leaf){})
+
+static void check_cpuid(struct cpuid_policy *cp)
+{
+    struct cpuid_policy new = {};
+    size_t data_end;
+    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;
+
+    /*
+     * Clean unusable leaves.  These can't be accessed via architectural
+     * means, but may be filled by the fread() across the entire structure.
+     * Also zero the trailing padding (if any).
+     */
+    cp->basic.raw[4] = EMPTY_LEAF;
+    cp->basic.raw[7] = EMPTY_LEAF;
+    cp->basic.raw[0xb] = EMPTY_LEAF;
+    cp->basic.raw[0xd] = EMPTY_LEAF;
+    data_end = offsetof(typeof(*cp), x86_vendor) + sizeof(cp->x86_vendor);
+    if ( data_end < sizeof(*cp) )
+        memset((void *)cp + data_end, 0, sizeof(*cp) - data_end);
+
+    /*
+     * Fix up the data in the source policy which isn't expected to survive
+     * serialisation.
+     */
+    x86_cpuid_policy_clear_out_of_range_leaves(cp);
+    x86_cpuid_policy_recalc_synth(cp);
+
+    /* Serialise... */
+    rc = x86_cpuid_copy_to_buffer(cp, leaves, &nr);
+    assert(rc == 0);
+    assert(nr <= CPUID_MAX_SERIALISED_LEAVES);
+
+    /* ... and deserialise. */
+    rc = x86_cpuid_copy_from_buffer(&new, leaves, nr, NULL, NULL);
+    assert(rc == 0);
+
+    /* The result after serialisation/deserialisaion should be identical... */
+    if ( memcmp(cp, &new, sizeof(*cp)) != 0 )
+    {
+        if ( debug )
+        {
+            unsigned char *l = (void *)cp, *r = (void *)&new;
+
+            for ( size_t i = 0; i < sizeof(*cp); ++i )
+                if ( l[i] != r[i] )
+                    printf("Differ at offset %zu: %u vs %u\n",
+                           i, l[i], r[i]);
+        }
+
+        abort();
+    }
+
+    free(leaves);
+}
+
+static void check_msr(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;
+
+    setbuf(stdin, NULL);
+    setbuf(stdout, NULL);
+
+    while ( true )
+    {
+        static const struct option opts[] = {
+            { "debug", no_argument, NULL, 'd' },
+            { "help", no_argument, NULL, 'h' },
+            {},
+        };
+        int c = getopt_long(argc, argv, "hd", opts, NULL);
+
+        if ( c == -1 )
+            break;
+
+        switch ( c )
+        {
+        case 'd':
+            printf("Enabling debug\n");
+            debug = true;
+            break;
+
+        default:
+            printf("Bad option %d (%c)\n", c, c);
+            exit(-1);
+            break;
+        }
+    }
+
+    if ( optind == argc ) /* No positional parameters.  Use stdin. */
+    {
+        printf("Using stdin\n");
+        fp = stdin;
+    }
+
+#ifdef __AFL_HAVE_MANUAL_CONTROL
+    __AFL_INIT();
+    while ( __AFL_LOOP(1000) )
+#endif
+    {
+        struct cpuid_policy *cp = NULL;
+        struct msr_policy *mp = NULL;
+
+        if ( fp != stdin )
+        {
+            printf("Opening file '%s'\n", argv[optind]);
+            fp = fopen(argv[optind], "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);
+        free(mp);
+
+        if ( fp != stdin )
+        {
+            fclose(fp);
+            fp = NULL;
+        }
+    }
+
+    return 0;
+}
diff --git a/xen/include/xen/lib/x86/cpuid.h b/xen/include/xen/lib/x86/cpuid.h
index 2618598..df5946b 100644
--- a/xen/include/xen/lib/x86/cpuid.h
+++ b/xen/include/xen/lib/x86/cpuid.h
@@ -323,6 +323,11 @@ static inline uint64_t cpuid_policy_xstates(const struct cpuid_policy *p)
 const uint32_t *x86_cpuid_lookup_deep_deps(uint32_t feature);
 
 /**
+ * Recalculate the content in a CPUID policy which is derived from raw data.
+ */
+void x86_cpuid_policy_recalc_synth(struct cpuid_policy *p);
+
+/**
  * Fill a CPUID policy using the native CPUID instruction.
  *
  * No sanitisation is performed, but synthesised values are calculated.
diff --git a/xen/lib/x86/cpuid.c b/xen/lib/x86/cpuid.c
index 0f99fcb..2404fa3 100644
--- a/xen/lib/x86/cpuid.c
+++ b/xen/lib/x86/cpuid.c
@@ -53,8 +53,7 @@ const char *x86_cpuid_vendor_to_str(unsigned int vendor)
     }
 }
 
-/* Recalculate the content in a CPUID policy which is derived from raw data. */
-static void recalculate_synth(struct cpuid_policy *p)
+void x86_cpuid_policy_recalc_synth(struct cpuid_policy *p)
 {
     p->x86_vendor = x86_cpuid_lookup_vendor(
         p->basic.vendor_ebx, p->basic.vendor_ecx, p->basic.vendor_edx);
@@ -167,7 +166,7 @@ void x86_cpuid_policy_fill_native(struct cpuid_policy *p)
                            p->extd.max_leaf + 1 - 0x80000000); ++i )
         cpuid_leaf(0x80000000 + i, &p->extd.raw[i]);
 
-    recalculate_synth(p);
+    x86_cpuid_policy_recalc_synth(p);
 }
 
 void x86_cpuid_policy_clear_out_of_range_leaves(struct cpuid_policy *p)
@@ -461,7 +460,7 @@ int x86_cpuid_copy_from_buffer(struct cpuid_policy *p,
         }
     }
 
-    recalculate_synth(p);
+    x86_cpuid_policy_recalc_synth(p);
 
     return 0;
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] libx86: Helper for clearing out-of-range CPUID leaves

[Jan Beulich]

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] tools/fuzz: Add a cpu-policy fuzzing harness

[Jan Beulich]

>>> On 04.06.19 at 21:51, <andrew.cooper3@citrix.com> wrote:
> There is now enough complexity that a fuzzing harness is a good idea, and
> enough supporting logic to implement one which AFL seems happy with.
> 
> Take the existing recalculate_synth() helper and export it as
> x86_cpuid_policy_recalc_synth(), as it is needed by the fuzzing harness.
> 
> 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/fuzz/cpu-policy/.gitignore          |   1 +
>  tools/fuzz/cpu-policy/Makefile            |  28 +++++
>  tools/fuzz/cpu-policy/afl-policy-fuzzer.c | 187 ++++++++++++++++++++++++++++++

Does this want to be accompanied by a ./MAINTAINERS update
to the X86 section?

>  xen/include/xen/lib/x86/cpuid.h           |   5 +
>  xen/lib/x86/cpuid.c                       |   7 +-
>  5 files changed, 224 insertions(+), 4 deletions(-)

Acked-by: Jan Beulich <jbeulich@suse.com>
with one further remark:

> +int main(int argc, char **argv)
> +{
> +    FILE *fp = NULL;
> +
> +    setbuf(stdin, NULL);
> +    setbuf(stdout, NULL);
> +
> +    while ( true )
> +    {
> +        static const struct option opts[] = {
> +            { "debug", no_argument, NULL, 'd' },
> +            { "help", no_argument, NULL, 'h' },
> +            {},
> +        };
> +        int c = getopt_long(argc, argv, "hd", opts, NULL);
> +
> +        if ( c == -1 )
> +            break;
> +
> +        switch ( c )
> +        {
> +        case 'd':
> +            printf("Enabling debug\n");
> +            debug = true;
> +            break;
> +
> +        default:
> +            printf("Bad option %d (%c)\n", c, c);
> +            exit(-1);
> +            break;

Wouldn't 'h' (wrongly) come into here? (The break statement also looks
to be unnecessary after exit().)

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/2] libx86: Fuzzing harness

[Jan Beulich]

>>> On 04.06.19 at 21:51, <andrew.cooper3@citrix.com> wrote:
> These are the final pieces to getting the fuzzing harness working correctly.

I'm mildly confused by this statement, as it seems to imply there was
something not working correctly, when in fact there was nothing at
all - patch 2 only adds a new harness.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/2] libx86: Fuzzing harness

[Andrew Cooper]

On 05/06/2019 10:54, Jan Beulich wrote:
>>>> On 04.06.19 at 21:51, <andrew.cooper3@citrix.com> wrote:
>> These are the final pieces to getting the fuzzing harness working correctly.
> I'm mildly confused by this statement, as it seems to imply there was
> something not working correctly, when in fact there was nothing at
> all - patch 2 only adds a new harness.

If you recall, the fuzzing harness was posted previously (during the
lead-up to L1TF) in the same patch as the unit tests, with a note saying
"sometimes AFL finds assertion failures".

In the end I dropped the fuzzing harness until I'd got it into a state
where it functioned correctly, and this is the final piece which isn't
yet committed upstream.

As to your question in patch 1 - all of the new library functionality is
strictly relevant to making DOMCTL_set_cpu_policy function correctly.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] libx86: Helper for clearing out-of-range CPUID leaves

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] tools/fuzz: Add a cpu-policy fuzzing harness

[Andrew Cooper]

On 05/06/2019 10:51, Jan Beulich wrote:
>>>> On 04.06.19 at 21:51, <andrew.cooper3@citrix.com> wrote:
>> There is now enough complexity that a fuzzing harness is a good idea, and
>> enough supporting logic to implement one which AFL seems happy with.
>>
>> Take the existing recalculate_synth() helper and export it as
>> x86_cpuid_policy_recalc_synth(), as it is needed by the fuzzing harness.
>>
>> 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/fuzz/cpu-policy/.gitignore          |   1 +
>>  tools/fuzz/cpu-policy/Makefile            |  28 +++++
>>  tools/fuzz/cpu-policy/afl-policy-fuzzer.c | 187 ++++++++++++++++++++++++++++++
> Does this want to be accompanied by a ./MAINTAINERS update
> to the X86 section?

Oops yes, and similarly for tools/tests/cpu-policy/

Do you mind if I fold that change in as well?

>
>>  xen/include/xen/lib/x86/cpuid.h           |   5 +
>>  xen/lib/x86/cpuid.c                       |   7 +-
>>  5 files changed, 224 insertions(+), 4 deletions(-)
> Acked-by: Jan Beulich <jbeulich@suse.com>
> with one further remark:
>
>> +int main(int argc, char **argv)
>> +{
>> +    FILE *fp = NULL;
>> +
>> +    setbuf(stdin, NULL);
>> +    setbuf(stdout, NULL);
>> +
>> +    while ( true )
>> +    {
>> +        static const struct option opts[] = {
>> +            { "debug", no_argument, NULL, 'd' },
>> +            { "help", no_argument, NULL, 'h' },
>> +            {},
>> +        };
>> +        int c = getopt_long(argc, argv, "hd", opts, NULL);
>> +
>> +        if ( c == -1 )
>> +            break;
>> +
>> +        switch ( c )
>> +        {
>> +        case 'd':
>> +            printf("Enabling debug\n");
>> +            debug = true;
>> +            break;
>> +
>> +        default:
>> +            printf("Bad option %d (%c)\n", c, c);
>> +            exit(-1);
>> +            break;
> Wouldn't 'h' (wrongly) come into here? (The break statement also looks
> to be unnecessary after exit().)

Hmm - something got lost in a rebase.  That was supposed to be default
printing bad option, then falling through into 'h' with usage.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] libx86: Helper for clearing out-of-range CPUID leaves

[Jan Beulich]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] tools/fuzz: Add a cpu-policy fuzzing harness

[Jan Beulich]

>>> On 05.06.19 at 13:31, <andrew.cooper3@citrix.com> wrote:
> On 05/06/2019 10:51, Jan Beulich wrote:
>>>>> On 04.06.19 at 21:51, <andrew.cooper3@citrix.com> wrote:
>>> There is now enough complexity that a fuzzing harness is a good idea, and
>>> enough supporting logic to implement one which AFL seems happy with.
>>>
>>> Take the existing recalculate_synth() helper and export it as
>>> x86_cpuid_policy_recalc_synth(), as it is needed by the fuzzing harness.
>>>
>>> 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/fuzz/cpu-policy/.gitignore          |   1 +
>>>  tools/fuzz/cpu-policy/Makefile            |  28 +++++
>>>  tools/fuzz/cpu-policy/afl-policy-fuzzer.c | 187 ++++++++++++++++++++++++++++++
>> Does this want to be accompanied by a ./MAINTAINERS update
>> to the X86 section?
> 
> Oops yes, and similarly for tools/tests/cpu-policy/
> 
> Do you mind if I fold that change in as well?

Feel free.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel