[PATCH For 4.10 0/4] Add support for using the Undefined Behaviour Sanitiser

[PATCH For 4.10 0/4] Add support for using the Undefined Behaviour Sanitiser

[Andrew Cooper]

Andrew Cooper (4):
  xen/tmem: Drop unnecessary noinline attribute
  xen/ubsan: Import ubsan implementation from Linux 4.13
  xen/ubsan: Implement __ubsan_handle_nonnull_arg()
  xen/ubsan: Introduce and use CONFIG_UBSAN

 xen/Kconfig                |   6 +
 xen/Kconfig.debug          |   8 +
 xen/Rules.mk               |   4 +
 xen/arch/x86/Kconfig       |   1 +
 xen/common/Makefile        |   1 +
 xen/common/tmem.c          |   2 +-
 xen/common/ubsan/Makefile  |   1 +
 xen/common/ubsan/ubsan.c   | 484 +++++++++++++++++++++++++++++++++++++++++++++
 xen/common/ubsan/ubsan.h   |  84 ++++++++
 xen/include/xen/compiler.h |   1 +
 10 files changed, 591 insertions(+), 1 deletion(-)
 create mode 100644 xen/common/ubsan/Makefile
 create mode 100644 xen/common/ubsan/ubsan.c
 create mode 100644 xen/common/ubsan/ubsan.h

-- 
2.1.4


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

[PATCH 1/4] xen/tmem: Drop unnecessary noinline attribute

[Andrew Cooper]

tmem_mempool_page_get() is only referenced by address, so isn't eligable for
inlining in the first place.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Not related to the rest of the series, but I stumbled across it while
resolving another noinline issue.
---
 xen/common/tmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/tmem.c b/xen/common/tmem.c
index c955cf7..324f42a 100644
--- a/xen/common/tmem.c
+++ b/xen/common/tmem.c
@@ -200,7 +200,7 @@ static void tmem_free_page(struct tmem_pool *pool, struct page_info *pfp)
     atomic_dec_and_assert(global_page_count);
 }
 
-static noinline void *tmem_mempool_page_get(unsigned long size)
+static void *tmem_mempool_page_get(unsigned long size)
 {
     struct page_info *pi;
 
-- 
2.1.4


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

Re: [PATCH 1/4] xen/tmem: Drop unnecessary noinline attribute

[Konrad Rzeszutek Wilk]

On Tue, Oct 03, 2017 at 07:07:50PM +0100, Andrew Cooper wrote:
> tmem_mempool_page_get() is only referenced by address, so isn't eligable for
> inlining in the first place.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

> 
> Not related to the rest of the series, but I stumbled across it while
> resolving another noinline issue.
> ---
>  xen/common/tmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/tmem.c b/xen/common/tmem.c
> index c955cf7..324f42a 100644
> --- a/xen/common/tmem.c
> +++ b/xen/common/tmem.c
> @@ -200,7 +200,7 @@ static void tmem_free_page(struct tmem_pool *pool, struct page_info *pfp)
>      atomic_dec_and_assert(global_page_count);
>  }
>  
> -static noinline void *tmem_mempool_page_get(unsigned long size)
> +static void *tmem_mempool_page_get(unsigned long size)
>  {
>      struct page_info *pi;
>  
> -- 
> 2.1.4
> 

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

[PATCH 2/4] xen/ubsan: Import ubsan implementation from Linux 4.13

[Andrew Cooper]

A future change will adjust it to compile in Xen.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/common/ubsan/Makefile |   1 +
 xen/common/ubsan/ubsan.c  | 456 ++++++++++++++++++++++++++++++++++++++++++++++
 xen/common/ubsan/ubsan.h  |  84 +++++++++
 3 files changed, 541 insertions(+)
 create mode 100644 xen/common/ubsan/Makefile
 create mode 100644 xen/common/ubsan/ubsan.c
 create mode 100644 xen/common/ubsan/ubsan.h

diff --git a/xen/common/ubsan/Makefile b/xen/common/ubsan/Makefile
new file mode 100644
index 0000000..e6b85ea
--- /dev/null
+++ b/xen/common/ubsan/Makefile
@@ -0,0 +1 @@
+obj-y += ubsan.o
diff --git a/xen/common/ubsan/ubsan.c b/xen/common/ubsan/ubsan.c
new file mode 100644
index 0000000..fb0409d
--- /dev/null
+++ b/xen/common/ubsan/ubsan.c
@@ -0,0 +1,456 @@
+/*
+ * UBSAN error reporting functions
+ *
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ * Author: Andrey Ryabinin <ryabinin.a.a@gmail.com>
+ *
+ * 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.
+ *
+ */
+
+#include <linux/bitops.h>
+#include <linux/bug.h>
+#include <linux/ctype.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/sched.h>
+
+#include "ubsan.h"
+
+const char *type_check_kinds[] = {
+	"load of",
+	"store to",
+	"reference binding to",
+	"member access within",
+	"member call on",
+	"constructor call on",
+	"downcast of",
+	"downcast of"
+};
+
+#define REPORTED_BIT 31
+
+#if (BITS_PER_LONG == 64) && defined(__BIG_ENDIAN)
+#define COLUMN_MASK (~(1U << REPORTED_BIT))
+#define LINE_MASK   (~0U)
+#else
+#define COLUMN_MASK   (~0U)
+#define LINE_MASK (~(1U << REPORTED_BIT))
+#endif
+
+#define VALUE_LENGTH 40
+
+static bool was_reported(struct source_location *location)
+{
+	return test_and_set_bit(REPORTED_BIT, &location->reported);
+}
+
+static void print_source_location(const char *prefix,
+				struct source_location *loc)
+{
+	pr_err("%s %s:%d:%d\n", prefix, loc->file_name,
+		loc->line & LINE_MASK, loc->column & COLUMN_MASK);
+}
+
+static bool suppress_report(struct source_location *loc)
+{
+	return current->in_ubsan || was_reported(loc);
+}
+
+static bool type_is_int(struct type_descriptor *type)
+{
+	return type->type_kind == type_kind_int;
+}
+
+static bool type_is_signed(struct type_descriptor *type)
+{
+	WARN_ON(!type_is_int(type));
+	return  type->type_info & 1;
+}
+
+static unsigned type_bit_width(struct type_descriptor *type)
+{
+	return 1 << (type->type_info >> 1);
+}
+
+static bool is_inline_int(struct type_descriptor *type)
+{
+	unsigned inline_bits = sizeof(unsigned long)*8;
+	unsigned bits = type_bit_width(type);
+
+	WARN_ON(!type_is_int(type));
+
+	return bits <= inline_bits;
+}
+
+static s_max get_signed_val(struct type_descriptor *type, unsigned long val)
+{
+	if (is_inline_int(type)) {
+		unsigned extra_bits = sizeof(s_max)*8 - type_bit_width(type);
+		return ((s_max)val) << extra_bits >> extra_bits;
+	}
+
+	if (type_bit_width(type) == 64)
+		return *(s64 *)val;
+
+	return *(s_max *)val;
+}
+
+static bool val_is_negative(struct type_descriptor *type, unsigned long val)
+{
+	return type_is_signed(type) && get_signed_val(type, val) < 0;
+}
+
+static u_max get_unsigned_val(struct type_descriptor *type, unsigned long val)
+{
+	if (is_inline_int(type))
+		return val;
+
+	if (type_bit_width(type) == 64)
+		return *(u64 *)val;
+
+	return *(u_max *)val;
+}
+
+static void val_to_string(char *str, size_t size, struct type_descriptor *type,
+	unsigned long value)
+{
+	if (type_is_int(type)) {
+		if (type_bit_width(type) == 128) {
+#if defined(CONFIG_ARCH_SUPPORTS_INT128) && defined(__SIZEOF_INT128__)
+			u_max val = get_unsigned_val(type, value);
+
+			scnprintf(str, size, "0x%08x%08x%08x%08x",
+				(u32)(val >> 96),
+				(u32)(val >> 64),
+				(u32)(val >> 32),
+				(u32)(val));
+#else
+			WARN_ON(1);
+#endif
+		} else if (type_is_signed(type)) {
+			scnprintf(str, size, "%lld",
+				(s64)get_signed_val(type, value));
+		} else {
+			scnprintf(str, size, "%llu",
+				(u64)get_unsigned_val(type, value));
+		}
+	}
+}
+
+static bool location_is_valid(struct source_location *loc)
+{
+	return loc->file_name != NULL;
+}
+
+static DEFINE_SPINLOCK(report_lock);
+
+static void ubsan_prologue(struct source_location *location,
+			unsigned long *flags)
+{
+	current->in_ubsan++;
+	spin_lock_irqsave(&report_lock, *flags);
+
+	pr_err("========================================"
+		"========================================\n");
+	print_source_location("UBSAN: Undefined behaviour in", location);
+}
+
+static void ubsan_epilogue(unsigned long *flags)
+{
+	dump_stack();
+	pr_err("========================================"
+		"========================================\n");
+	spin_unlock_irqrestore(&report_lock, *flags);
+	current->in_ubsan--;
+}
+
+static void handle_overflow(struct overflow_data *data, unsigned long lhs,
+			unsigned long rhs, char op)
+{
+
+	struct type_descriptor *type = data->type;
+	unsigned long flags;
+	char lhs_val_str[VALUE_LENGTH];
+	char rhs_val_str[VALUE_LENGTH];
+
+	if (suppress_report(&data->location))
+		return;
+
+	ubsan_prologue(&data->location, &flags);
+
+	val_to_string(lhs_val_str, sizeof(lhs_val_str), type, lhs);
+	val_to_string(rhs_val_str, sizeof(rhs_val_str), type, rhs);
+	pr_err("%s integer overflow:\n",
+		type_is_signed(type) ? "signed" : "unsigned");
+	pr_err("%s %c %s cannot be represented in type %s\n",
+		lhs_val_str,
+		op,
+		rhs_val_str,
+		type->type_name);
+
+	ubsan_epilogue(&flags);
+}
+
+void __ubsan_handle_add_overflow(struct overflow_data *data,
+				unsigned long lhs,
+				unsigned long rhs)
+{
+
+	handle_overflow(data, lhs, rhs, '+');
+}
+EXPORT_SYMBOL(__ubsan_handle_add_overflow);
+
+void __ubsan_handle_sub_overflow(struct overflow_data *data,
+				unsigned long lhs,
+				unsigned long rhs)
+{
+	handle_overflow(data, lhs, rhs, '-');
+}
+EXPORT_SYMBOL(__ubsan_handle_sub_overflow);
+
+void __ubsan_handle_mul_overflow(struct overflow_data *data,
+				unsigned long lhs,
+				unsigned long rhs)
+{
+	handle_overflow(data, lhs, rhs, '*');
+}
+EXPORT_SYMBOL(__ubsan_handle_mul_overflow);
+
+void __ubsan_handle_negate_overflow(struct overflow_data *data,
+				unsigned long old_val)
+{
+	unsigned long flags;
+	char old_val_str[VALUE_LENGTH];
+
+	if (suppress_report(&data->location))
+		return;
+
+	ubsan_prologue(&data->location, &flags);
+
+	val_to_string(old_val_str, sizeof(old_val_str), data->type, old_val);
+
+	pr_err("negation of %s cannot be represented in type %s:\n",
+		old_val_str, data->type->type_name);
+
+	ubsan_epilogue(&flags);
+}
+EXPORT_SYMBOL(__ubsan_handle_negate_overflow);
+
+
+void __ubsan_handle_divrem_overflow(struct overflow_data *data,
+				unsigned long lhs,
+				unsigned long rhs)
+{
+	unsigned long flags;
+	char rhs_val_str[VALUE_LENGTH];
+
+	if (suppress_report(&data->location))
+		return;
+
+	ubsan_prologue(&data->location, &flags);
+
+	val_to_string(rhs_val_str, sizeof(rhs_val_str), data->type, rhs);
+
+	if (type_is_signed(data->type) && get_signed_val(data->type, rhs) == -1)
+		pr_err("division of %s by -1 cannot be represented in type %s\n",
+			rhs_val_str, data->type->type_name);
+	else
+		pr_err("division by zero\n");
+
+	ubsan_epilogue(&flags);
+}
+EXPORT_SYMBOL(__ubsan_handle_divrem_overflow);
+
+static void handle_null_ptr_deref(struct type_mismatch_data *data)
+{
+	unsigned long flags;
+
+	if (suppress_report(&data->location))
+		return;
+
+	ubsan_prologue(&data->location, &flags);
+
+	pr_err("%s null pointer of type %s\n",
+		type_check_kinds[data->type_check_kind],
+		data->type->type_name);
+
+	ubsan_epilogue(&flags);
+}
+
+static void handle_missaligned_access(struct type_mismatch_data *data,
+				unsigned long ptr)
+{
+	unsigned long flags;
+
+	if (suppress_report(&data->location))
+		return;
+
+	ubsan_prologue(&data->location, &flags);
+
+	pr_err("%s misaligned address %p for type %s\n",
+		type_check_kinds[data->type_check_kind],
+		(void *)ptr, data->type->type_name);
+	pr_err("which requires %ld byte alignment\n", data->alignment);
+
+	ubsan_epilogue(&flags);
+}
+
+static void handle_object_size_mismatch(struct type_mismatch_data *data,
+					unsigned long ptr)
+{
+	unsigned long flags;
+
+	if (suppress_report(&data->location))
+		return;
+
+	ubsan_prologue(&data->location, &flags);
+	pr_err("%s address %p with insufficient space\n",
+		type_check_kinds[data->type_check_kind],
+		(void *) ptr);
+	pr_err("for an object of type %s\n", data->type->type_name);
+	ubsan_epilogue(&flags);
+}
+
+void __ubsan_handle_type_mismatch(struct type_mismatch_data *data,
+				unsigned long ptr)
+{
+
+	if (!ptr)
+		handle_null_ptr_deref(data);
+	else if (data->alignment && !IS_ALIGNED(ptr, data->alignment))
+		handle_missaligned_access(data, ptr);
+	else
+		handle_object_size_mismatch(data, ptr);
+}
+EXPORT_SYMBOL(__ubsan_handle_type_mismatch);
+
+void __ubsan_handle_nonnull_return(struct nonnull_return_data *data)
+{
+	unsigned long flags;
+
+	if (suppress_report(&data->location))
+		return;
+
+	ubsan_prologue(&data->location, &flags);
+
+	pr_err("null pointer returned from function declared to never return null\n");
+
+	if (location_is_valid(&data->attr_location))
+		print_source_location("returns_nonnull attribute specified in",
+				&data->attr_location);
+
+	ubsan_epilogue(&flags);
+}
+EXPORT_SYMBOL(__ubsan_handle_nonnull_return);
+
+void __ubsan_handle_vla_bound_not_positive(struct vla_bound_data *data,
+					unsigned long bound)
+{
+	unsigned long flags;
+	char bound_str[VALUE_LENGTH];
+
+	if (suppress_report(&data->location))
+		return;
+
+	ubsan_prologue(&data->location, &flags);
+
+	val_to_string(bound_str, sizeof(bound_str), data->type, bound);
+	pr_err("variable length array bound value %s <= 0\n", bound_str);
+
+	ubsan_epilogue(&flags);
+}
+EXPORT_SYMBOL(__ubsan_handle_vla_bound_not_positive);
+
+void __ubsan_handle_out_of_bounds(struct out_of_bounds_data *data,
+				unsigned long index)
+{
+	unsigned long flags;
+	char index_str[VALUE_LENGTH];
+
+	if (suppress_report(&data->location))
+		return;
+
+	ubsan_prologue(&data->location, &flags);
+
+	val_to_string(index_str, sizeof(index_str), data->index_type, index);
+	pr_err("index %s is out of range for type %s\n", index_str,
+		data->array_type->type_name);
+	ubsan_epilogue(&flags);
+}
+EXPORT_SYMBOL(__ubsan_handle_out_of_bounds);
+
+void __ubsan_handle_shift_out_of_bounds(struct shift_out_of_bounds_data *data,
+					unsigned long lhs, unsigned long rhs)
+{
+	unsigned long flags;
+	struct type_descriptor *rhs_type = data->rhs_type;
+	struct type_descriptor *lhs_type = data->lhs_type;
+	char rhs_str[VALUE_LENGTH];
+	char lhs_str[VALUE_LENGTH];
+
+	if (suppress_report(&data->location))
+		return;
+
+	ubsan_prologue(&data->location, &flags);
+
+	val_to_string(rhs_str, sizeof(rhs_str), rhs_type, rhs);
+	val_to_string(lhs_str, sizeof(lhs_str), lhs_type, lhs);
+
+	if (val_is_negative(rhs_type, rhs))
+		pr_err("shift exponent %s is negative\n", rhs_str);
+
+	else if (get_unsigned_val(rhs_type, rhs) >=
+		type_bit_width(lhs_type))
+		pr_err("shift exponent %s is too large for %u-bit type %s\n",
+			rhs_str,
+			type_bit_width(lhs_type),
+			lhs_type->type_name);
+	else if (val_is_negative(lhs_type, lhs))
+		pr_err("left shift of negative value %s\n",
+			lhs_str);
+	else
+		pr_err("left shift of %s by %s places cannot be"
+			" represented in type %s\n",
+			lhs_str, rhs_str,
+			lhs_type->type_name);
+
+	ubsan_epilogue(&flags);
+}
+EXPORT_SYMBOL(__ubsan_handle_shift_out_of_bounds);
+
+
+void __noreturn
+__ubsan_handle_builtin_unreachable(struct unreachable_data *data)
+{
+	unsigned long flags;
+
+	ubsan_prologue(&data->location, &flags);
+	pr_err("calling __builtin_unreachable()\n");
+	ubsan_epilogue(&flags);
+	panic("can't return from __builtin_unreachable()");
+}
+EXPORT_SYMBOL(__ubsan_handle_builtin_unreachable);
+
+void __ubsan_handle_load_invalid_value(struct invalid_value_data *data,
+				unsigned long val)
+{
+	unsigned long flags;
+	char val_str[VALUE_LENGTH];
+
+	if (suppress_report(&data->location))
+		return;
+
+	ubsan_prologue(&data->location, &flags);
+
+	val_to_string(val_str, sizeof(val_str), data->type, val);
+
+	pr_err("load of value %s is not a valid value for type %s\n",
+		val_str, data->type->type_name);
+
+	ubsan_epilogue(&flags);
+}
+EXPORT_SYMBOL(__ubsan_handle_load_invalid_value);
diff --git a/xen/common/ubsan/ubsan.h b/xen/common/ubsan/ubsan.h
new file mode 100644
index 0000000..b2d18d4
--- /dev/null
+++ b/xen/common/ubsan/ubsan.h
@@ -0,0 +1,84 @@
+#ifndef _LIB_UBSAN_H
+#define _LIB_UBSAN_H
+
+enum {
+	type_kind_int = 0,
+	type_kind_float = 1,
+	type_unknown = 0xffff
+};
+
+struct type_descriptor {
+	u16 type_kind;
+	u16 type_info;
+	char type_name[1];
+};
+
+struct source_location {
+	const char *file_name;
+	union {
+		unsigned long reported;
+		struct {
+			u32 line;
+			u32 column;
+		};
+	};
+};
+
+struct overflow_data {
+	struct source_location location;
+	struct type_descriptor *type;
+};
+
+struct type_mismatch_data {
+	struct source_location location;
+	struct type_descriptor *type;
+	unsigned long alignment;
+	unsigned char type_check_kind;
+};
+
+struct nonnull_arg_data {
+	struct source_location location;
+	struct source_location attr_location;
+	int arg_index;
+};
+
+struct nonnull_return_data {
+	struct source_location location;
+	struct source_location attr_location;
+};
+
+struct vla_bound_data {
+	struct source_location location;
+	struct type_descriptor *type;
+};
+
+struct out_of_bounds_data {
+	struct source_location location;
+	struct type_descriptor *array_type;
+	struct type_descriptor *index_type;
+};
+
+struct shift_out_of_bounds_data {
+	struct source_location location;
+	struct type_descriptor *lhs_type;
+	struct type_descriptor *rhs_type;
+};
+
+struct unreachable_data {
+	struct source_location location;
+};
+
+struct invalid_value_data {
+	struct source_location location;
+	struct type_descriptor *type;
+};
+
+#if defined(CONFIG_ARCH_SUPPORTS_INT128) && defined(__SIZEOF_INT128__)
+typedef __int128 s_max;
+typedef unsigned __int128 u_max;
+#else
+typedef s64 s_max;
+typedef u64 u_max;
+#endif
+
+#endif
-- 
2.1.4


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

[PATCH 3/4] xen/ubsan: Implement __ubsan_handle_nonnull_arg()

[Andrew Cooper]

This hook appears to be missing from the Linux ubsan implemention.  This patch
is a forward port of https://lkml.org/lkml/2014/10/20/182

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/common/ubsan/ubsan.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/xen/common/ubsan/ubsan.c b/xen/common/ubsan/ubsan.c
index fb0409d..e44c8ce 100644
--- a/xen/common/ubsan/ubsan.c
+++ b/xen/common/ubsan/ubsan.c
@@ -328,6 +328,26 @@ void __ubsan_handle_type_mismatch(struct type_mismatch_data *data,
 }
 EXPORT_SYMBOL(__ubsan_handle_type_mismatch);
 
+void __ubsan_handle_nonnull_arg(struct nonnull_arg_data *data)
+{
+	unsigned long flags;
+
+	if (suppress_report(&data->location))
+		return;
+
+	ubsan_prologue(&data->location, &flags);
+
+	pr_err("null pointer passed as argument %d, declared with nonnull attribute\n",
+	       data->arg_index);
+
+	if (location_is_valid(&data->attr_location))
+		print_source_location("nonnull attribute declared in ",
+				      &data->attr_location);
+
+	ubsan_epilogue(&flags);
+}
+EXPORT_SYMBOL(__ubsan_handle_nonnull_arg);
+
 void __ubsan_handle_nonnull_return(struct nonnull_return_data *data)
 {
 	unsigned long flags;
-- 
2.1.4


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

Re: [PATCH 3/4] xen/ubsan: Implement __ubsan_handle_nonnull_arg()

[Jan Beulich]

>>> On 03.10.17 at 20:07, <andrew.cooper3@citrix.com> wrote:
> This hook appears to be missing from the Linux ubsan implemention.  This patch
> is a forward port of https://lkml.org/lkml/2014/10/20/182 
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Patch 2 as well as this one
Acked-by: Jan Beulich <jbeulich@suse.com>
albeit preferably with ...

> --- a/xen/common/ubsan/ubsan.c
> +++ b/xen/common/ubsan/ubsan.c
> @@ -328,6 +328,26 @@ void __ubsan_handle_type_mismatch(struct type_mismatch_data *data,
>  }
>  EXPORT_SYMBOL(__ubsan_handle_type_mismatch);
>  
> +void __ubsan_handle_nonnull_arg(struct nonnull_arg_data *data)
> +{
> +	unsigned long flags;
> +
> +	if (suppress_report(&data->location))
> +		return;
> +
> +	ubsan_prologue(&data->location, &flags);
> +
> +	pr_err("null pointer passed as argument %d, declared with nonnull attribute\n",
> +	       data->arg_index);
> +
> +	if (location_is_valid(&data->attr_location))
> +		print_source_location("nonnull attribute declared in ",
> +				      &data->attr_location);
> +
> +	ubsan_epilogue(&flags);
> +}
> +EXPORT_SYMBOL(__ubsan_handle_nonnull_arg);

... all the EXPORT_SYMBOL()s dropped - I was really hoping we
could get ris of what we've left instead of adding new ones.

Jan


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

[PATCH 4/4] xen/ubsan: Introduce and use CONFIG_UBSAN

[Andrew Cooper]

Tested with GCC 4.9 of Debian Jessie.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>

TODO at some future point: Fix the following known issues:

  Clang 3.9 - linker error in shadow/multi.c with fetch_type_names[].  With
  UBSAN enabled, it appears that dead code elimination doesn't remove the
  single reference to fetch_type_names[] which lives behind DEBUG_TRACE_DUMP.

  Clang 4.0 - ABI change with the hooks.
---
 xen/Kconfig                |  6 ++++++
 xen/Kconfig.debug          |  8 ++++++++
 xen/Rules.mk               |  4 ++++
 xen/arch/x86/Kconfig       |  1 +
 xen/common/Makefile        |  1 +
 xen/common/ubsan/ubsan.c   | 22 +++++++++++++++-------
 xen/include/xen/compiler.h |  1 +
 7 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/xen/Kconfig b/xen/Kconfig
index 65d491d..f57cefd 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -38,4 +38,10 @@ config LTO
 
 	  If unsure, say N.
 
+#
+# For architectures that know their GCC __int128 support is sound
+#
+config ARCH_SUPPORTS_INT128
+	bool
+
 source "Kconfig.debug"
diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
index 195d504..e63b533 100644
--- a/xen/Kconfig.debug
+++ b/xen/Kconfig.debug
@@ -121,6 +121,14 @@ config SCRUB_DEBUG
 	  Verify that pages that need to be scrubbed before being allocated to
 	  a guest are indeed scrubbed.
 
+config UBSAN
+	bool "Undefined behaviour sanitizer"
+	depends on X86
+	---help---
+	  Enable undefined behaviour sanitizer.
+
+	  If unsure, say N here.
+
 endif # DEBUG || EXPERT
 
 endmenu
diff --git a/xen/Rules.mk b/xen/Rules.mk
index cafc67b..2659f8a 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -119,6 +119,10 @@ ifeq ($(CONFIG_GCOV),y)
 $(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fprofile-arcs -ftest-coverage
 endif
 
+ifeq ($(CONFIG_UBSAN),y)
+$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fsanitize=undefined
+endif
+
 ifeq ($(CONFIG_LTO),y)
 CFLAGS += -flto
 LDFLAGS-$(clang) += -plugin LLVMgold.so
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 30c2769..f77e6fc 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -5,6 +5,7 @@ config X86
 	def_bool y
 	select ACPI
 	select ACPI_LEGACY_TABLES_LOOKUP
+	select ARCH_SUPPORTS_INT128
 	select COMPAT
 	select CORE_PARKING
 	select HAS_ALTERNATIVE
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 39e2614..66cc2c8 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -75,6 +75,7 @@ tmem-$(CONFIG_COMPAT) += compat/tmem_xen.o
 obj-$(CONFIG_TMEM) += $(tmem-y)
 
 subdir-$(CONFIG_GCOV) += gcov
+subdir-$(CONFIG_UBSAN) += ubsan
 
 subdir-y += libelf
 subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
diff --git a/xen/common/ubsan/ubsan.c b/xen/common/ubsan/ubsan.c
index e44c8ce..b601af9 100644
--- a/xen/common/ubsan/ubsan.c
+++ b/xen/common/ubsan/ubsan.c
@@ -10,13 +10,21 @@
  *
  */
 
-#include <linux/bitops.h>
-#include <linux/bug.h>
-#include <linux/ctype.h>
-#include <linux/init.h>
-#include <linux/kernel.h>
-#include <linux/types.h>
-#include <linux/sched.h>
+#include <xen/bitops.h>
+#include <xen/kernel.h>
+#include <xen/lib.h>
+#include <xen/types.h>
+#include <xen/spinlock.h>
+#include <xen/percpu.h>
+
+#define pr_err(...) printk(XENLOG_ERR __VA_ARGS__)
+struct xen_ubsan { int in_ubsan; };
+static DEFINE_PER_CPU(struct xen_ubsan[1], in_ubsan);
+#undef current
+#define current this_cpu(in_ubsan)
+#define dump_stack dump_execution_state
+#define u64 long long unsigned int
+#define s64 long long int
 
 #include "ubsan.h"
 
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index 533a8ea..e4d706f 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -15,6 +15,7 @@
 #define noinline      __attribute__((__noinline__))
 
 #define noreturn      __attribute__((__noreturn__))
+#define __noreturn    noreturn
 
 #define __packed      __attribute__((__packed__))
 
-- 
2.1.4


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

Re: [PATCH 4/4] xen/ubsan: Introduce and use CONFIG_UBSAN

[Wei Liu]

On Tue, Oct 03, 2017 at 07:07:53PM +0100, Andrew Cooper wrote:
> Tested with GCC 4.9 of Debian Jessie.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Series:

Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Tested-by: Wei Liu <wei.liu2@citrix.com> with GCC 6.3

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

Re: [PATCH 4/4] xen/ubsan: Introduce and use CONFIG_UBSAN

[Andrew Cooper]

On 03/10/17 19:07, Andrew Cooper wrote:
> TODO at some future point: Fix the following known issues:
>
>   Clang 3.9 - linker error in shadow/multi.c with fetch_type_names[].  With
>   UBSAN enabled, it appears that dead code elimination doesn't remove the
>   single reference to fetch_type_names[] which lives behind DEBUG_TRACE_DUMP.

FYI, the linking error is:

prelink.o: In function `_sh_propagate':
/local/xen.git/xen/arch/x86/mm/shadow/multi.c:731: undefined reference
to `fetch_type_names'

And this patch works around the error:

diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 28030ac..7a7ad3d 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -75,15 +75,11 @@ typedef enum {
     ft_demand_write = FETCH_TYPE_DEMAND | FETCH_TYPE_WRITE,
 } fetch_type_t;
 
-extern const char *const fetch_type_names[];
-
-#if defined(DEBUG_TRACE_DUMP) && CONFIG_PAGING_LEVELS ==
GUEST_PAGING_LEVELS
-const char *const fetch_type_names[] = {
+static const char *const fetch_type_names[] = {
     [ft_prefetch]     = "prefetch",
     [ft_demand_read]  = "demand read",
     [ft_demand_write] = "demand write",
 };
-#endif
 
 /**************************************************************************/
 /* Hash table mapping from guest pagetables to shadows


However, this goes against the intended purpose of c/s 89173c1051a0

~Andrew

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

Re: [PATCH 4/4] xen/ubsan: Introduce and use CONFIG_UBSAN

[Jan Beulich]

>>> On 03.10.17 at 20:07, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/Kconfig
> +++ b/xen/Kconfig
> @@ -38,4 +38,10 @@ config LTO
>  
>  	  If unsure, say N.
>  
> +#
> +# For architectures that know their GCC __int128 support is sound
> +#
> +config ARCH_SUPPORTS_INT128
> +	bool

Why GCC? What about Clang?

> --- a/xen/Kconfig.debug
> +++ b/xen/Kconfig.debug
> @@ -121,6 +121,14 @@ config SCRUB_DEBUG
>  	  Verify that pages that need to be scrubbed before being allocated to
>  	  a guest are indeed scrubbed.
>  
> +config UBSAN
> +	bool "Undefined behaviour sanitizer"
> +	depends on X86

I think we should switch away from this model of explicitly stating
architectures, and instead have HAVE_* symbols selected by each
architecture supporting it, and the main symbol then depending on
the HAVE_* one. Us having only two architectures right now
doesn't make this a big difference, but Linux has (partially?)
switched to that model for a reason, I think.

> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -119,6 +119,10 @@ ifeq ($(CONFIG_GCOV),y)
>  $(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += 
> -fprofile-arcs -ftest-coverage
>  endif
>  
> +ifeq ($(CONFIG_UBSAN),y)
> +$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fsanitize=undefined
> +endif

You have no users of noubsan-y, other than what Wei's RFC patch
had. Also neither you nor he explain why *.init.o are unilaterally
excluded.

> --- a/xen/common/ubsan/ubsan.c
> +++ b/xen/common/ubsan/ubsan.c
> @@ -10,13 +10,21 @@
>   *
>   */
>  
> -#include <linux/bitops.h>
> -#include <linux/bug.h>
> -#include <linux/ctype.h>
> -#include <linux/init.h>
> -#include <linux/kernel.h>
> -#include <linux/types.h>
> -#include <linux/sched.h>
> +#include <xen/bitops.h>
> +#include <xen/kernel.h>
> +#include <xen/lib.h>
> +#include <xen/types.h>
> +#include <xen/spinlock.h>
> +#include <xen/percpu.h>

I don't think all of these are needed - xen/types.h is certainly
being included implicitly by several others, for example, and
always will be.

> --- a/xen/include/xen/compiler.h
> +++ b/xen/include/xen/compiler.h
> @@ -15,6 +15,7 @@
>  #define noinline      __attribute__((__noinline__))
>  
>  #define noreturn      __attribute__((__noreturn__))
> +#define __noreturn    noreturn

Please let's avoid new name space violations if at all possibly, or
at least restrict them to individual source files where eliminating
them would be undesirable.

Jan


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

Re: [PATCH 4/4] xen/ubsan: Introduce and use CONFIG_UBSAN

[Andrew Cooper]

On 04/10/17 13:03, Jan Beulich wrote:
>>>> On 03.10.17 at 20:07, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/Kconfig
>> +++ b/xen/Kconfig
>> @@ -38,4 +38,10 @@ config LTO
>>  
>>  	  If unsure, say N.
>>  
>> +#
>> +# For architectures that know their GCC __int128 support is sound
>> +#
>> +config ARCH_SUPPORTS_INT128
>> +	bool
> Why GCC? What about Clang?

This came straight from Linux.  I can s/GCC/compiler/ if you like?

>
>> --- a/xen/Kconfig.debug
>> +++ b/xen/Kconfig.debug
>> @@ -121,6 +121,14 @@ config SCRUB_DEBUG
>>  	  Verify that pages that need to be scrubbed before being allocated to
>>  	  a guest are indeed scrubbed.
>>  
>> +config UBSAN
>> +	bool "Undefined behaviour sanitizer"
>> +	depends on X86
> I think we should switch away from this model of explicitly stating
> architectures, and instead have HAVE_* symbols selected by each
> architecture supporting it, and the main symbol then depending on
> the HAVE_* one. Us having only two architectures right now
> doesn't make this a big difference, but Linux has (partially?)
> switched to that model for a reason, I think.

I'm not fussed.  Which would you prefer?

>
>> --- a/xen/Rules.mk
>> +++ b/xen/Rules.mk
>> @@ -119,6 +119,10 @@ ifeq ($(CONFIG_GCOV),y)
>>  $(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += 
>> -fprofile-arcs -ftest-coverage
>>  endif
>>  
>> +ifeq ($(CONFIG_UBSAN),y)
>> +$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fsanitize=undefined
>> +endif
> You have no users of noubsan-y, other than what Wei's RFC patch
> had. Also neither you nor he explain why *.init.o are unilaterally
> excluded.

The answer is complicated.  If you want it to work with .init. files,
then the following change is required:

diff --git a/xen/Rules.mk b/xen/Rules.mk
index cafc67b..9ce5b56 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -189,7 +189,7 @@ $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)):
%.init.o: %.o Makefile
                .text|.text.*|.data|.data.*|.bss) \
                        test $$sz != 0 || continue; \
                        echo "Error: size of $<:$$name is 0x$$sz" >&2; \
-                       exit $$(expr $$idx + 1);; \
+                       # exit $$(expr $$idx + 1);; \
                esac; \
        done
        $(OBJCOPY) $(foreach s,$(SPECIAL_DATA_SECTIONS),--rename-section
.$(s)=.init.$(s)) $< $@

I was debating whether to keep or remove the noubsan, but I figured that
keeping it would be more flexible for developing with.

>
>> --- a/xen/common/ubsan/ubsan.c
>> +++ b/xen/common/ubsan/ubsan.c
>> @@ -10,13 +10,21 @@
>>   *
>>   */
>>  
>> -#include <linux/bitops.h>
>> -#include <linux/bug.h>
>> -#include <linux/ctype.h>
>> -#include <linux/init.h>
>> -#include <linux/kernel.h>
>> -#include <linux/types.h>
>> -#include <linux/sched.h>
>> +#include <xen/bitops.h>
>> +#include <xen/kernel.h>
>> +#include <xen/lib.h>
>> +#include <xen/types.h>
>> +#include <xen/spinlock.h>
>> +#include <xen/percpu.h>
> I don't think all of these are needed - xen/types.h is certainly
> being included implicitly by several others, for example, and
> always will be.

Hmm.  This list of headers is ~18 months old.  I will see if I can prune
it some more.

>
>> --- a/xen/include/xen/compiler.h
>> +++ b/xen/include/xen/compiler.h
>> @@ -15,6 +15,7 @@
>>  #define noinline      __attribute__((__noinline__))
>>  
>>  #define noreturn      __attribute__((__noreturn__))
>> +#define __noreturn    noreturn
> Please let's avoid new name space violations if at all possibly, or
> at least restrict them to individual source files where eliminating
> them would be undesirable.

This is entirely down to how much we want to diverge the Linux code. 
For ubsan, I've gone out of my way not to modify the Linux code at all.

I can see an argument for making this local to the file in question. 
However, that needs to be weighed up against other Linux source we
choose to take.

~Andrew

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

Re: [PATCH 4/4] xen/ubsan: Introduce and use CONFIG_UBSAN

[Jan Beulich]

>>> On 04.10.17 at 15:28, <andrew.cooper3@citrix.com> wrote:
> On 04/10/17 13:03, Jan Beulich wrote:
>>>>> On 03.10.17 at 20:07, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/Kconfig
>>> +++ b/xen/Kconfig
>>> @@ -38,4 +38,10 @@ config LTO
>>>  
>>>  	  If unsure, say N.
>>>  
>>> +#
>>> +# For architectures that know their GCC __int128 support is sound
>>> +#
>>> +config ARCH_SUPPORTS_INT128
>>> +	bool
>> Why GCC? What about Clang?
> 
> This came straight from Linux.  I can s/GCC/compiler/ if you like?

Yes please (provided it's usable with Clang).

>>> --- a/xen/Kconfig.debug
>>> +++ b/xen/Kconfig.debug
>>> @@ -121,6 +121,14 @@ config SCRUB_DEBUG
>>>  	  Verify that pages that need to be scrubbed before being allocated to
>>>  	  a guest are indeed scrubbed.
>>>  
>>> +config UBSAN
>>> +	bool "Undefined behaviour sanitizer"
>>> +	depends on X86
>> I think we should switch away from this model of explicitly stating
>> architectures, and instead have HAVE_* symbols selected by each
>> architecture supporting it, and the main symbol then depending on
>> the HAVE_* one. Us having only two architectures right now
>> doesn't make this a big difference, but Linux has (partially?)
>> switched to that model for a reason, I think.
> 
> I'm not fussed.  Which would you prefer?

I'd prefer the combination HAVE_UBSAN plus UBSAN, as outlined.

>>> --- a/xen/Rules.mk
>>> +++ b/xen/Rules.mk
>>> @@ -119,6 +119,10 @@ ifeq ($(CONFIG_GCOV),y)
>>>  $(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += 
>>> -fprofile-arcs -ftest-coverage
>>>  endif
>>>  
>>> +ifeq ($(CONFIG_UBSAN),y)
>>> +$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fsanitize=undefined
>>> +endif
>> You have no users of noubsan-y, other than what Wei's RFC patch
>> had.

What about this part?

>> Also neither you nor he explain why *.init.o are unilaterally
>> excluded.
> 
> The answer is complicated.  If you want it to work with .init. files,
> then the following change is required:
> 
> diff --git a/xen/Rules.mk b/xen/Rules.mk
> index cafc67b..9ce5b56 100644
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -189,7 +189,7 @@ $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)):
> %.init.o: %.o Makefile
>                 .text|.text.*|.data|.data.*|.bss) \
>                         test $$sz != 0 || continue; \
>                         echo "Error: size of $<:$$name is 0x$$sz" >&2; \
> -                       exit $$(expr $$idx + 1);; \
> +                       # exit $$(expr $$idx + 1);; \
>                 esac; \
>         done
>         $(OBJCOPY) $(foreach s,$(SPECIAL_DATA_SECTIONS),--rename-section
> .$(s)=.init.$(s)) $< $@
> 
> I was debating whether to keep or remove the noubsan, but I figured that
> keeping it would be more flexible for developing with.

Could you mention this in the commit message then, please?

>>> --- a/xen/include/xen/compiler.h
>>> +++ b/xen/include/xen/compiler.h
>>> @@ -15,6 +15,7 @@
>>>  #define noinline      __attribute__((__noinline__))
>>>  
>>>  #define noreturn      __attribute__((__noreturn__))
>>> +#define __noreturn    noreturn
>> Please let's avoid new name space violations if at all possibly, or
>> at least restrict them to individual source files where eliminating
>> them would be undesirable.
> 
> This is entirely down to how much we want to diverge the Linux code. 
> For ubsan, I've gone out of my way not to modify the Linux code at all.

Except for the #include-s.

> I can see an argument for making this local to the file in question. 
> However, that needs to be weighed up against other Linux source we
> choose to take.

As there's no reasonable chance for us to ever be able to take
a Linux file completely unmodified, if putting such #define-s into
individual files is too restrictive for your taste, how about making
those files identify themselves (by, say, #define LINUX_SOURCE)
and enabling such compatibility things only for those?

Jan


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

Re: [PATCH 4/4] xen/ubsan: Introduce and use CONFIG_UBSAN

[Konrad Rzeszutek Wilk]

> +config UBSAN
> +	bool "Undefined behaviour sanitizer"
> +	depends on X86
> +	---help---
> +	  Enable undefined behaviour sanitizer.
> +
> +	  If unsure, say N here.

Could you perhaps expand it a bit? How does it sanitize it? With soap :-)?
And what 'undefinded behaviour's are we talking about? opcodes? 

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

Re: [PATCH 4/4] xen/ubsan: Introduce and use CONFIG_UBSAN

[Wei Liu]

On Thu, Oct 05, 2017 at 08:49:36AM -0400, Konrad Rzeszutek Wilk wrote:
> > +config UBSAN
> > +	bool "Undefined behaviour sanitizer"
> > +	depends on X86
> > +	---help---
> > +	  Enable undefined behaviour sanitizer.
> > +
> > +	  If unsure, say N here.
> 
> Could you perhaps expand it a bit? How does it sanitize it? With soap :-)?
> And what 'undefinded behaviour's are we talking about? opcodes? 

It sanitizes undefined behaviour in C.

It does so by using compiler black magic: the code in Xen is
instrumented; appropriate calls to hooks are inserted; hooks are called
once UB is detected during runtime.

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