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

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

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

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