Re: [Xen-devel] [PATCH RFC] xen: add undefined behaviour sanitizer

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 03/10/17 17:03, Wei Liu wrote:
> 1. Introduce Kconfig UBSAN option.
> 2. Import and adapt a bunch of hooks from Linux kernel with as little
>    modification as possible.
> 3. Provide mechanism to filter out unwanted files.
>
> This is x86 only at the moment. It should be easy to make it work on
> arm, but I don't have the test environment to figure out what files
> need to be filtered out.
>
> Currently two files are filtered out:
>
> 1. arch/x86/setup.o -- xen crashes very early without any output

With GCC 6.3 and with unaligned access checking?  If so, it is probably
the code which initially sets up the IDT.

I'm not sure enabling unaligned access checking by default is

> 2. drivers/acpi/tables/tbutils.o -- it has requirement to access
>    misaligned memory before current is properly set up.
>
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
>
> Though this is not going to be in 4.10 I think it would be valuable to
> fix the issues it reports.
>
> And example output on x86:
> https://paste.debian.net/988890/

The traps.c and hypercall.c objections can't plausibly be fixed.  Each
example is from when we are hand-crafting x86 opcode, which does
inherently have unaligned data.

>
> diff --git a/xen/common/ubsan/ubsan.c b/xen/common/ubsan/ubsan.c
> new file mode 100644
> index 0000000000..aa359e712c
> --- /dev/null
> +++ b/xen/common/ubsan/ubsan.c
> @@ -0,0 +1,486 @@
> +/*
> + * UBSAN error reporting functions
> + *
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + * Author: Andrey Ryabinin <ryabinin.a.a@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Adapt to Xen by:
> + *  Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> + *  Wei Liu <wei.liu2@xxxxxxxxxx>
> + */
> +
> +#include <xen/bitops.h>
> +#include <xen/kernel.h>
> +#include <xen/lib.h>
> +#include <xen/types.h>
> +#include <xen/sched.h>
> +#include <xen/spinlock.h>
> +#include <xen/percpu.h>
> +
> +#define pr_err(...) printk(XENLOG_ERR __VA_ARGS__)
> +#define dump_stack dump_execution_state
> +#define __noreturn noreturn
> +#define CONFIG_ARCH_SUPPORTS_INT128
> +#define u64 long long unsigned int
> +#define s64 long long int

These last three should be in asm-$ARCH/config.h or similar.  I'm fairly
sure arm32 will struggle with INT128.

Also, I think the current (re)engineering should stay.  There is nothing
vcpu specific about the in_ubsan flag.  (Especially, given your
tbutils.o observation above.)

As for commits introducing the code, I had a vague plan to introduce the
unmodified code from upstream Linux, then my forward ported patch (as
that isn't in Linux, even today), and then a change adding the Kconfig
option, and these defines, to get it to compile.

Given that this is clearly a debugging option, and its basically ready,
I wouldn't necessarily hold off from putting it into 4.10.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel