[Xen-devel] [PATCH 0/2] xen/ubsan: Multiple fixes

[Xen-devel] [PATCH 0/2] xen/ubsan: Multiple fixes

[Andrew Cooper]

This series fixes building with GCC 8 and later, and fixes booting native with
GCC 5 and later.

Andrew Cooper (2):
  xen/ubsan: Don't perform alignment checking on supporting compilers
  xen/ubsan: Support for -fsanitise=builtin

 xen/Rules.mk             |  4 +++-
 xen/common/ubsan/ubsan.c | 21 +++++++++++++++++++++
 xen/common/ubsan/ubsan.h |  9 +++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

-- 
2.11.0


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

[Xen-devel] [PATCH 1/2] xen/ubsan: Don't perform alignment checking on supporting compilers

[Andrew Cooper]

GCC 5 introduced -fsanitize=alignment which is enabled by default by
CONFIG_UBSAN.  This trips a load of wont-fix cases in the ACPI tables and the
hypercall page and stubs writing logic.

It also causes the native Xen boot to crash before the console is set up, for
an as-yet unidentified reason (most likley a wont-fix case earlier on boot).

Disable alignment sanitisation on compilers which would try using it.

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: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

This isn't ideal, but we can't do better without a bit of an overhaul which I
don't have time for now.  Linux uses a whitelist of sanitisers but I'm not
entirely sure we want to go that route.  ARM currently isn't working well with
UBSAN, but AFACIT, all ARM platforms that we support also disable alignment
sanitisation in Linux.
---
 xen/Rules.mk | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/Rules.mk b/xen/Rules.mk
index a151b3f625..61cd8ed5d9 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -138,7 +138,9 @@ $(filter-out %.init.o $(nocov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += $(
 endif
 
 ifeq ($(CONFIG_UBSAN),y)
-$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fsanitize=undefined
+UBSAN_FLAGS := -fsanitize=undefined
+$(call cc-option-add,UBSAN_FLAGS,CC,-fno-sanitize=alignment)
+$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += $(UBSAN_FLAGS)
 endif
 
 ifeq ($(CONFIG_LTO),y)
-- 
2.11.0


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

[Xen-devel] [PATCH 2/2] xen/ubsan: Support for -fsanitise=builtin

[Andrew Cooper]

This fixes the UBSAN build for GCC 8 and later.  The sanitiser checks for
passing 0 to the ctz()/clz() builtins.

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: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/common/ubsan/ubsan.c | 21 +++++++++++++++++++++
 xen/common/ubsan/ubsan.h |  9 +++++++++
 2 files changed, 30 insertions(+)

diff --git a/xen/common/ubsan/ubsan.c b/xen/common/ubsan/ubsan.c
index 50a4e14fac..e75f614fa7 100644
--- a/xen/common/ubsan/ubsan.c
+++ b/xen/common/ubsan/ubsan.c
@@ -518,3 +518,24 @@ void __ubsan_handle_pointer_overflow(struct pointer_overflow_data *data,
 
 	ubsan_epilogue(&flags);
 }
+
+void __ubsan_handle_invalid_builtin(struct invalid_builtin_data *data)
+{
+	unsigned long flags;
+	const char *fn;
+
+	if (suppress_report(&data->location))
+		return;
+
+	ubsan_prologue(&data->location, &flags);
+
+	switch (data->kind) {
+	case kind_ctz: fn = "ctz"; break;
+	case kind_clz: fn = "clz"; break;
+	default: fn = "<UNKNOWN>"; break;
+	}
+
+	pr_err("passing zero to %s(), which is not a valid argument\n", fn);
+
+	ubsan_epilogue(&flags);
+}
diff --git a/xen/common/ubsan/ubsan.h b/xen/common/ubsan/ubsan.h
index 2710cd423e..a3159040fe 100644
--- a/xen/common/ubsan/ubsan.h
+++ b/xen/common/ubsan/ubsan.h
@@ -84,6 +84,15 @@ struct pointer_overflow_data {
 	struct source_location location;
 };
 
+struct invalid_builtin_data {
+	struct source_location location;
+	unsigned char kind;
+};
+enum {
+	kind_ctz,
+	kind_clz,
+};
+
 #if defined(CONFIG_ARCH_SUPPORTS_INT128) && defined(__SIZEOF_INT128__)
 typedef __int128 s_max;
 typedef unsigned __int128 u_max;
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 1/2] xen/ubsan: Don't perform alignment checking on supporting compilers

[Julien Grall]

Hi Andrew,

On 6/24/19 11:17 AM, Andrew Cooper wrote:
> GCC 5 introduced -fsanitize=alignment which is enabled by default by
> CONFIG_UBSAN.  This trips a load of wont-fix cases in the ACPI tables and the
> hypercall page and stubs writing logic.
> 
> It also causes the native Xen boot to crash before the console is set up, for
> an as-yet unidentified reason (most likley a wont-fix case earlier on boot).
> 
> Disable alignment sanitisation on compilers which would try using it.
> 
> 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: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> 
> This isn't ideal, but we can't do better without a bit of an overhaul which I
> don't have time for now.  Linux uses a whitelist of sanitisers but I'm not
> entirely sure we want to go that route.  ARM currently isn't working well with
> UBSAN, but AFACIT, all ARM platforms that we support also disable alignment
> sanitisation in Linux.

Linux has an option to disable/enable aligment sanitisation. However, 
IIRC, Linux allows unaligned access for both Arm32 and Arm64.

For Xen:
   - On Arm32, alignment check is enabled, so any unaligned access will 
result to a crash.
   - On Arm64, alignment check is disabled, the only reason is because 
memcpy is using unaligned access (for performance reason). But we should 
still not rely on unaligned access as they are not atomic.

The only limitation for using UBSAN on Xen on Arm today is the size of 
the binary (we only support up to 2MB). So my preference here would be 
to make the new flag x86 only.

Ideally longer plan would be to make a per-file decision on the 
sanitization to use.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 1/2] xen/ubsan: Don't perform alignment checking on supporting compilers

[Andrew Cooper]

On 24/06/2019 11:33, Julien Grall wrote:
> Hi Andrew,
>
> On 6/24/19 11:17 AM, Andrew Cooper wrote:
>> GCC 5 introduced -fsanitize=alignment which is enabled by default by
>> CONFIG_UBSAN.  This trips a load of wont-fix cases in the ACPI tables
>> and the
>> hypercall page and stubs writing logic.
>>
>> It also causes the native Xen boot to crash before the console is set
>> up, for
>> an as-yet unidentified reason (most likley a wont-fix case earlier on
>> boot).
>>
>> Disable alignment sanitisation on compilers which would try using it.
>>
>> 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: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>>
>> This isn't ideal, but we can't do better without a bit of an overhaul
>> which I
>> don't have time for now.  Linux uses a whitelist of sanitisers but
>> I'm not
>> entirely sure we want to go that route.  ARM currently isn't working
>> well with
>> UBSAN, but AFACIT, all ARM platforms that we support also disable
>> alignment
>> sanitisation in Linux.
>
> Linux has an option to disable/enable aligment sanitisation. However,
> IIRC, Linux allows unaligned access for both Arm32 and Arm64.
>
> For Xen:
>   - On Arm32, alignment check is enabled, so any unaligned access will
> result to a crash.
>   - On Arm64, alignment check is disabled, the only reason is because
> memcpy is using unaligned access (for performance reason). But we
> should still not rely on unaligned access as they are not atomic.
>
> The only limitation for using UBSAN on Xen on Arm today is the size of
> the binary (we only support up to 2MB). So my preference here would be
> to make the new flag x86 only.

Ok - that shouldn't be too difficult to arrange.

>
> Ideally longer plan would be to make a per-file decision on the
> sanitization to use.

ARM64's memcpy is written in assembly so not subject to UBSAN.

For GCC 8 and later, there is a new __attribute__((no_sanitize("foo",
"bar"))) to selectively disable specific checkers on a per-function basis.

~Andrew

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

[Xen-devel] [PATCH v2 1/2] x86/ubsan: Don't perform alignment checking on supporting compilers

[Andrew Cooper]

GCC 5 introduced -fsanitize=alignment which is enabled by default by
CONFIG_UBSAN.  This trips a load of wont-fix cases in the ACPI tables and the
hypercall page and stubs writing logic.

It also causes the native Xen boot to crash before the console is set up, for
an as-yet unidentified reason (most likley a wont-fix case earlier on boot).

Disable alignment sanitisation on compilers which would try using it.

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: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

v2:
 * Avoid using -fno-sanitize=alignment for ARM
---
 xen/Rules.mk          | 5 ++++-
 xen/arch/x86/Rules.mk | 4 ++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/Rules.mk b/xen/Rules.mk
index a151b3f625..bf81735b8b 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -138,7 +138,10 @@ $(filter-out %.init.o $(nocov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += $(
 endif
 
 ifeq ($(CONFIG_UBSAN),y)
-$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fsanitize=undefined
+UBSAN_FLAGS += -fsanitize=undefined
+# Any -fno-sanitise= options need to come after any -fsanitise= options
+$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)):\
+CFLAGS += $(filter-out -fno-%,$(UBSAN_FLAGS)) $(filter -fno-%,$(UBSAN_FLAGS))
 endif
 
 ifeq ($(CONFIG_LTO),y)
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index babc0edbcd..00d861cf76 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -57,6 +57,10 @@ endif
 $(call cc-option-add,CFLAGS-stack-boundary,CC,-mpreferred-stack-boundary=3)
 CFLAGS += $(CFLAGS-stack-boundary)
 
+ifeq ($(CONFIG_UBSAN),y)
+$(call cc-option-add,UBSAN_FLAGS,CC,-fno-sanitize=alignment)
+endif
+
 # Set up the assembler include path properly for older toolchains.
 CFLAGS += -Wa,-I$(BASEDIR)/include
 
-- 
2.11.0


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

Re: [Xen-devel] [PATCH v2 1/2] x86/ubsan: Don't perform alignment checking on supporting compilers

[Jan Beulich]

>>> On 24.06.19 at 20:25, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -138,7 +138,10 @@ $(filter-out %.init.o $(nocov-y),$(obj-y) $(obj-bin-y) 
> $(extra-y)): CFLAGS += $(
>  endif
>  
>  ifeq ($(CONFIG_UBSAN),y)
> -$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): 
> CFLAGS += -fsanitize=undefined
> +UBSAN_FLAGS += -fsanitize=undefined

Here and in the x86 change below to append to UBSAN_FLAGS. I think we
have more such cases, but I also think we shouldn't extend the badness:
We should start with an empty variable, rather than whatever may have
been inherited from the environment.

Also could this become UBSAN_CFLAGS or CFLAGS_UBSAN? Or perhaps
UBSAN_CFLAGS-y / CFLAGS_UBSAN-y, making adding to it easier?

> +# Any -fno-sanitise= options need to come after any -fsanitise= options
> +$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)):\

Could you add a blank before the backslash, for readability?

> --- a/xen/arch/x86/Rules.mk
> +++ b/xen/arch/x86/Rules.mk
> @@ -57,6 +57,10 @@ endif
>  $(call cc-option-add,CFLAGS-stack-boundary,CC,-mpreferred-stack-boundary=3)
>  CFLAGS += $(CFLAGS-stack-boundary)
>  
> +ifeq ($(CONFIG_UBSAN),y)
> +$(call cc-option-add,UBSAN_FLAGS,CC,-fno-sanitize=alignment)
> +endif

Perhaps worth adding a short comment as to the "why"? And perhaps
no need for the ifeq()?

Jan



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

Re: [Xen-devel] [PATCH 2/2] xen/ubsan: Support for -fsanitise=builtin

[Jan Beulich]

>>> On 24.06.19 at 12:17, <andrew.cooper3@citrix.com> wrote:
> This fixes the UBSAN build for GCC 8 and later.  The sanitiser checks for
> passing 0 to the ctz()/clz() builtins.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Fundamentally
Acked-by: Jan Beulich <jbeulich@suse.com>

However,

> --- a/xen/common/ubsan/ubsan.c
> +++ b/xen/common/ubsan/ubsan.c
> @@ -518,3 +518,24 @@ void __ubsan_handle_pointer_overflow(struct pointer_overflow_data *data,
>  
>  	ubsan_epilogue(&flags);
>  }
> +
> +void __ubsan_handle_invalid_builtin(struct invalid_builtin_data *data)
> +{
> +	unsigned long flags;
> +	const char *fn;
> +
> +	if (suppress_report(&data->location))
> +		return;
> +
> +	ubsan_prologue(&data->location, &flags);
> +
> +	switch (data->kind) {
> +	case kind_ctz: fn = "ctz"; break;
> +	case kind_clz: fn = "clz"; break;
> +	default: fn = "<UNKNOWN>"; break;
> +	}
> +
> +	pr_err("passing zero to %s(), which is not a valid argument\n", fn);

... logging the unknown enumerator value might turn out helpful
down the road.

Jan



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

[Xen-devel] [PATCH v3 1/2] x86/ubsan: Don't perform alignment checking on supporting compilers

[Andrew Cooper]

GCC 5 introduced -fsanitize=alignment which is enabled by default by
CONFIG_UBSAN.  This trips a load of wont-fix cases in the ACPI tables and the
hypercall page and stubs writing logic.

It also causes the native Xen boot to crash before the console is set up, for
an as-yet unidentified reason (most likley a wont-fix case earlier on boot).

Disable alignment sanitisation on compilers which would try using it.

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: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

v2:
 * Avoid using -fno-sanitize=alignment for ARM
v3:
 * Use CFLAGS_UBSAN, initialised to empty first.
 * Leave a comment explaining why we use -fno-sanitize=alignment

The reason why x86 has a ifeq ($(CONFIG_UBSAN),y) is to avoid yet another
cc-option-add evaluated for every translation uint in the default case.  This
will all go away when we update to a recent version of Kconfig which moves all
the toolchain checks into that phase.
---
 xen/Rules.mk          | 8 +++++++-
 xen/arch/x86/Rules.mk | 8 ++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/xen/Rules.mk b/xen/Rules.mk
index a151b3f625..3090ea7828 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -41,6 +41,9 @@ ALL_OBJS-y               += $(BASEDIR)/xsm/built_in.o
 ALL_OBJS-y               += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o
 ALL_OBJS-$(CONFIG_CRYPTO)   += $(BASEDIR)/crypto/built_in.o
 
+# Initialise some variables
+CFLAGS_UBSAN :=
+
 ifeq ($(CONFIG_DEBUG),y)
 CFLAGS += -O1
 else
@@ -138,7 +141,10 @@ $(filter-out %.init.o $(nocov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += $(
 endif
 
 ifeq ($(CONFIG_UBSAN),y)
-$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fsanitize=undefined
+CFLAGS_UBSAN += -fsanitize=undefined
+# Any -fno-sanitize= options need to come after any -fsanitize= options
+$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): \
+CFLAGS += $(filter-out -fno-%,$(CFLAGS_UBSAN)) $(filter -fno-%,$(CFLAGS_UBSAN))
 endif
 
 ifeq ($(CONFIG_LTO),y)
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index babc0edbcd..52e93afb48 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -57,6 +57,14 @@ endif
 $(call cc-option-add,CFLAGS-stack-boundary,CC,-mpreferred-stack-boundary=3)
 CFLAGS += $(CFLAGS-stack-boundary)
 
+ifeq ($(CONFIG_UBSAN),y)
+# Don't enable alignment sanitisation.  x86 has efficient unaligned accesses,
+# and various things (ACPI tables, hypercall pages, stubs, etc) are wont-fix.
+# It also causes an as-yet-unidentified crash on native boot before the
+# console starts.
+$(call cc-option-add,CFLAGS_UBSAN,CC,-fno-sanitize=alignment)
+endif
+
 # Set up the assembler include path properly for older toolchains.
 CFLAGS += -Wa,-I$(BASEDIR)/include
 
-- 
2.11.0


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