[1/2] xen/ubsan: Don't perform alignment checking on supporting compilers
diff mbox series

Message ID 20190624101723.23291-2-andrew.cooper3@citrix.com
State New
Headers show
Series
  • xen/ubsan: Multiple fixes
Related show

Commit Message

Andrew Cooper June 24, 2019, 10:17 a.m. UTC
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(-)

Comments

Julien Grall June 24, 2019, 10:33 a.m. UTC | #1
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,
Andrew Cooper June 24, 2019, 11:04 a.m. UTC | #2
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

Patch
diff mbox series

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)