Hi
This is yet another sanitizer for linux kernel.
UBSan uses copile-time instumentation to catch undefined behavior (UB).
Compiler inserts code that perform certain kinds of
checks before operations that could cause UB.
If check fails (i.e. UB detected) __ubsan_handle_* function called.
to print error message.
Patch is also available via git:
git://github.com/aryabinin/linux --branch ubsan/v1
GCC supports this since 4.9, however upcoming GCC 5.0 has
more checkers implemented.
Different kinds of checkers could be enabled via boot parameter:
ubsan_handle=OEAINVBSLF.
If ubsan_handle not present in cmdline default options are used: ELNVBSLF
O - different kinds of overflows
E - negation overflow, division overflow, division by zero.
A - misaligned memory access.
I - load from/store to an object with insufficient space.
N - null argument declared with nonnull attribute,
returned null from function which never returns null, null ptr dereference.
V - variable size array with non-positive length
B - out-of-bounds memory accesses.
S - shifting out-of-bounds.
L - load of invalid value (value out of range for the enum type, loading other then 0/1 to bool type)
F - call to function through pointer with incorrect function type
(AFAIK this is not implemented in gcc yet, probably works with clang,
though I didn't check it).
Andrey Ryabinin (1):
UBSan: run-time undefined behavior sanity checker
Makefile | 12 +-
arch/x86/Kconfig | 1 +
arch/x86/boot/Makefile | 1 +
arch/x86/boot/compressed/Makefile | 1 +
arch/x86/realmode/rm/Makefile | 1 +
arch/x86/vdso/Makefile | 2 +
drivers/firmware/efi/libstub/Makefile | 1 +
include/linux/sched.h | 4 +
kernel/printk/Makefile | 1 +
lib/Kconfig.debug | 23 ++
lib/Makefile | 3 +
lib/ubsan.c | 559 ++++++++++++++++++++++++++++++++++
lib/ubsan.h | 84 +++++
scripts/Makefile.lib | 6 +
14 files changed, 698 insertions(+), 1 deletion(-)
create mode 100644 lib/ubsan.c
create mode 100644 lib/ubsan.h
--
2.1.2
UBSan uses compile-time instrumentation to catch undefined behavior (UB).
Compiler inserts code that perform certain kinds of
checks before operations that could cause UB.
If check fails (i.e. UB detected) __ubsan_handle_* function called.
to print error message.
So the most of the work is done by compiler.
This patch just implements ubsan handlers printing errors.
GCC supports this since 4.9, however upcoming GCC 5.0 has
more checkers implemented.
Different kinds of checks could be enabled via boot parameter:
ubsan_handle=OEAINVBSLF.
If ubsan_handle not present in cmdline default options are used: ELNVBSLF
O - different kinds of overflows
E - negation overflow, division overflow, division by zero.
A - misaligned memory access.
I - load from/store to an object with insufficient space.
N - null argument declared with nonnull attribute,
returned null from function which never returns null, null ptr dereference.
V - variable size array with non-positive length
B - out-of-bounds accesses.
S - shifting out-of-bounds.
L - load of invalid value (value out of range for the enum type, loading other then 0/1 to bool type)
F - call to function through pointer with incorrect function type
(AFAIK this is not implemented in gcc yet, probably works with clang, though
I didn't check ubsan with clang at all).
Instrumentation in kernel/printk/printk.c is disabled because struct printk_log is not properly aligned,
therefore we are recursively taking logbuf_lock while trying to print error in __ubsan_handle*().
Signed-off-by: Andrey Ryabinin <[email protected]>
---
Makefile | 12 +-
arch/x86/Kconfig | 1 +
arch/x86/boot/Makefile | 1 +
arch/x86/boot/compressed/Makefile | 1 +
arch/x86/realmode/rm/Makefile | 1 +
arch/x86/vdso/Makefile | 2 +
drivers/firmware/efi/libstub/Makefile | 1 +
include/linux/sched.h | 4 +
kernel/printk/Makefile | 1 +
lib/Kconfig.debug | 23 ++
lib/Makefile | 3 +
lib/ubsan.c | 559 ++++++++++++++++++++++++++++++++++
lib/ubsan.h | 84 +++++
scripts/Makefile.lib | 6 +
14 files changed, 698 insertions(+), 1 deletion(-)
create mode 100644 lib/ubsan.c
create mode 100644 lib/ubsan.h
diff --git a/Makefile b/Makefile
index 05d67af..d3e23f9 100644
--- a/Makefile
+++ b/Makefile
@@ -377,6 +377,9 @@ LDFLAGS_MODULE =
CFLAGS_KERNEL =
AFLAGS_KERNEL =
CFLAGS_GCOV = -fprofile-arcs -ftest-coverage
+CFLAGS_UBSAN = $(call cc-option, -fsanitize=undefined) \
+ $(call cc-option, -fno-sanitize=unreachable) \
+ $(call cc-option, -fno-sanitize=float-cast-overflow)
# Use USERINCLUDE when you must reference the UAPI directories only.
@@ -421,7 +424,7 @@ export MAKE AWK GENKSYMS INSTALLKERNEL PERL PYTHON UTS_MACHINE
export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS LDFLAGS
-export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_GCOV
+export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_GCOV CFLAGS_UBSAN
export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE
export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_LDFLAGS_MODULE
export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL
@@ -670,6 +673,13 @@ endif
endif
KBUILD_CFLAGS += $(stackp-flag)
+ifdef CONFIG_UBSAN
+ ifeq ($(strip $(CFLAGS_UBSAN)),)
+ $(warning Cannot use CONFIG_UBSAN: \
+ -fsanitize=undefined not supported by compiler)
+ endif
+endif
+
ifeq ($(COMPILER),clang)
KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,)
KBUILD_CPPFLAGS += $(call cc-option,-Wno-unknown-warning-option,)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index f2327e8..b318fe8 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -55,6 +55,7 @@ config X86
select HAVE_FUNCTION_TRACER
select HAVE_FUNCTION_GRAPH_TRACER
select HAVE_FUNCTION_GRAPH_FP_TEST
+ select HAVE_ARCH_UBSAN_SANTIZE_ALL
select HAVE_SYSCALL_TRACEPOINTS
select SYSCTL_EXCEPTION_TRACE
select HAVE_KVM
diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
index 5b016e2..95bf522 100644
--- a/arch/x86/boot/Makefile
+++ b/arch/x86/boot/Makefile
@@ -57,6 +57,7 @@ endif
KBUILD_CFLAGS := $(USERINCLUDE) $(REALMODE_CFLAGS) -D_SETUP
KBUILD_AFLAGS := $(KBUILD_CFLAGS) -D__ASSEMBLY__
GCOV_PROFILE := n
+UBSAN_SANITIZE := n
$(obj)/bzImage: asflags-y := $(SVGA_MODE)
diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index 704f58a..16940f8 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -19,6 +19,7 @@ KBUILD_CFLAGS += $(call cc-option,-fno-stack-protector)
KBUILD_AFLAGS := $(KBUILD_CFLAGS) -D__ASSEMBLY__
GCOV_PROFILE := n
+UBSAN_SANITIZE :=n
LDFLAGS := -m elf_$(UTS_MACHINE)
LDFLAGS_vmlinux := -T
diff --git a/arch/x86/realmode/rm/Makefile b/arch/x86/realmode/rm/Makefile
index 7c0d7be..5c48444 100644
--- a/arch/x86/realmode/rm/Makefile
+++ b/arch/x86/realmode/rm/Makefile
@@ -69,3 +69,4 @@ KBUILD_CFLAGS := $(LINUXINCLUDE) $(REALMODE_CFLAGS) -D_SETUP -D_WAKEUP \
-I$(srctree)/arch/x86/boot
KBUILD_AFLAGS := $(KBUILD_CFLAGS) -D__ASSEMBLY__
GCOV_PROFILE := n
+UBSAN_SANITIZE := n
diff --git a/arch/x86/vdso/Makefile b/arch/x86/vdso/Makefile
index 5a4affe..3054473 100644
--- a/arch/x86/vdso/Makefile
+++ b/arch/x86/vdso/Makefile
@@ -4,6 +4,8 @@
KBUILD_CFLAGS += $(DISABLE_LTO)
+UBSAN_SANITIZE := n
+
VDSO64-$(CONFIG_X86_64) := y
VDSOX32-$(CONFIG_X86_X32_ABI) := y
VDSO32-$(CONFIG_X86_32) := y
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index b14bc2b..7b07624 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -19,6 +19,7 @@ KBUILD_CFLAGS := $(cflags-y) \
$(call cc-option,-fno-stack-protector)
GCOV_PROFILE := n
+UBSAN_SANITIZE := n
lib-y := efi-stub-helper.o
lib-$(CONFIG_EFI_ARMSTUB) += arm-stub.o fdt.o
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 5e344bb..cfba635 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1661,6 +1661,10 @@ struct task_struct {
unsigned int sequential_io;
unsigned int sequential_io_avg;
#endif
+
+#ifdef CONFIG_UBSAN
+ unsigned int in_ubsan;
+#endif
};
/* Future-safe accessor for struct task_struct's cpus_allowed. */
diff --git a/kernel/printk/Makefile b/kernel/printk/Makefile
index 85405bd..36461b5 100644
--- a/kernel/printk/Makefile
+++ b/kernel/printk/Makefile
@@ -1,2 +1,3 @@
obj-y = printk.o
obj-$(CONFIG_A11Y_BRAILLE_CONSOLE) += braille.o
+UBSAN_SANITIZE := n
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 4e35a5d..7dc9b89 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -646,6 +646,29 @@ config DEBUG_SHIRQ
Drivers ought to be able to handle interrupts coming in at those
points; some don't and need to be caught.
+config HAVE_ARCH_UBSAN_SANTIZE_ALL
+ bool
+
+config UBSAN
+ bool "Undefined behaviour sanity checker"
+ help
+ This option enables undefined behaviour sanity checker
+ Compile-time instrumentataion used to detect various undefined
+ behaviours in runtime. Different kinds of checks could be enabled
+ via boot parameter ubsan_handle (see: Documentation/ubsan.txt).
+ (TODO: write docs).
+
+config UBSAN_SANITIZE_ALL
+ bool "Enable instrumentation for the entire kernel"
+ depends on UBSAN
+ depends on HAVE_ARCH_UBSAN_SANTIZE_ALL
+ default y
+ help
+ This option acitivates instrumentation for the entire kernel.
+ If you don't enable this option, you have to explicitly specify
+ UBSAN_SANITIZE := y for the files/directories you want to check for UB.
+
+
menu "Debug Lockups and Hangs"
config LOCKUP_DETECTOR
diff --git a/lib/Makefile b/lib/Makefile
index 7512dc9..d8c05c68 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -196,3 +196,6 @@ quiet_cmd_build_OID_registry = GEN $@
clean-files += oid_registry_data.c
obj-$(CONFIG_UCS2_STRING) += ucs2_string.o
+obj-$(CONFIG_UBSAN) += ubsan.o
+
+UBSAN_SANITIZE_ubsan.o := n
diff --git a/lib/ubsan.c b/lib/ubsan.c
new file mode 100644
index 0000000..7788f47
--- /dev/null
+++ b/lib/ubsan.c
@@ -0,0 +1,559 @@
+#include <linux/bug.h>
+#include <linux/ctype.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"
+};
+
+enum {
+ SUM_OVERFLOW,
+ SUB_OVERFLOW,
+ MUL_OVERFLOW,
+ NEG_OVERFLOW,
+ DIVREM_OVERFLOW,
+ ALIGNMENT,
+ OBJECT_SIZE,
+ NONNULL_ARG,
+ NONNULL_RET,
+ NULL_PTR,
+ VLA_BOUND,
+ OUT_OF_BOUNDS,
+ SHIFT_OUT_OF_BOUNDS,
+ INVALID_LOAD,
+ FUNC_TYPE_MISMATCH,
+ HANDLERS_END,
+};
+
+/* By default enable everything except signed overflows and
+ * misaligned accesses
+ */
+static unsigned long ubsan_handle = GENMASK(HANDLERS_END, 0) &
+ ~(BIT_MASK(SUM_OVERFLOW) | BIT_MASK(SUB_OVERFLOW) |
+ BIT_MASK(NEG_OVERFLOW) | BIT_MASK(ALIGNMENT));
+
+static void enable_handler(unsigned int handler)
+{
+ set_bit(handler, &ubsan_handle);
+}
+
+static bool handler_enabled(unsigned int handler)
+{
+ return test_bit(handler, &ubsan_handle);
+}
+
+#define REPORTED_BIT 31
+#define COLUMN_MASK (~(1U << REPORTED_BIT))
+
+static bool is_disabled(struct source_location *location)
+{
+ return test_and_set_bit(REPORTED_BIT,
+ (unsigned long *)&location->column);
+}
+
+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, loc->column & COLUMN_MASK);
+}
+
+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)
+{
+ return type_is_int(type) && (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);
+
+ 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)
+{
+ u_max val = get_unsigned_val(type, value);
+
+ if (type_is_int(type)) {
+ if (type_bit_width(type) == 128)
+ scnprintf(str, size, "0x%08x%08x%08x%08x",
+ (u32)(val >> 96),
+ (u32)(val >> 64),
+ (u32)(val >> 32),
+ (u32)(val));
+ 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 void ubsan_prologue(struct source_location *location)
+{
+ current->in_ubsan++;
+ pr_err("========================================"
+ "========================================\n");
+ print_source_location("UBSan: Undefined behaviour in", location);
+}
+
+static void ubsan_epilogue(void)
+{
+ dump_stack();
+ pr_err("========================================"
+ "========================================\n");
+ 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;
+ char lhs_val_str[60];
+ char rhs_val_str[60];
+
+ if (current->in_ubsan)
+ return;
+
+ if (is_disabled(&data->location))
+ return;
+
+ ubsan_prologue(&data->location);
+
+ 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();
+}
+
+void __ubsan_handle_add_overflow(struct overflow_data *data,
+ unsigned long lhs,
+ unsigned long rhs)
+{
+
+ if (handler_enabled(SUM_OVERFLOW))
+ 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)
+{
+ if (handler_enabled(SUB_OVERFLOW))
+ 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)
+{
+ if (handler_enabled(MUL_OVERFLOW))
+ handle_overflow(data, lhs, rhs, '*');
+}
+EXPORT_SYMBOL(__ubsan_handle_mul_overflow);
+
+void __ubsan_handle_negate_overflow(struct overflow_data *data,
+ unsigned long old_val)
+{
+
+ char old_val_str[60];
+
+ if (!handler_enabled(NEG_OVERFLOW))
+ return;
+
+ if (current->in_ubsan)
+ return;
+
+ if (is_disabled(&data->location))
+ return;
+
+ ubsan_prologue(&data->location);
+
+ 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();
+}
+EXPORT_SYMBOL(__ubsan_handle_negate_overflow);
+
+
+void __ubsan_handle_divrem_overflow(struct overflow_data *data,
+ unsigned long lhs,
+ unsigned long rhs)
+{
+ char rhs_val_str[60];
+
+ if (!handler_enabled(DIVREM_OVERFLOW))
+ return;
+
+ if (current->in_ubsan)
+ return;
+
+ if (is_disabled(&data->location))
+ return;
+
+ ubsan_prologue(&data->location);
+
+ 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();
+}
+EXPORT_SYMBOL(__ubsan_handle_divrem_overflow);
+
+void __ubsan_handle_type_mismatch(struct type_mismatch_data *data,
+ unsigned long ptr)
+{
+
+ if (current->in_ubsan)
+ return;
+
+ if (is_disabled(&data->location))
+ return;
+
+ if (!ptr) {
+ if (!handler_enabled(NULL_PTR))
+ return;
+
+ ubsan_prologue(&data->location);
+
+ pr_err("%s null pointer of type %s\n",
+ type_check_kinds[data->type_check_kind],
+ data->type->type_name);
+ } else if (data->alignment && (ptr & (data->alignment - 1))) {
+ if (!handler_enabled(ALIGNMENT))
+ return;
+
+ ubsan_prologue(&data->location);
+ pr_err("%s misaligned address %p for type %s "
+ "which requires %ld byte alignment\n",
+ type_check_kinds[data->type_check_kind],
+ (void *)ptr, data->type->type_name, data->alignment);
+ } else {
+ if (!handler_enabled(OBJECT_SIZE))
+ return;
+
+ ubsan_prologue(&data->location);
+ pr_err("%s address %pk 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();
+}
+EXPORT_SYMBOL(__ubsan_handle_type_mismatch);
+
+void __ubsan_handle_nonnull_arg(struct nonnull_arg_data *data)
+{
+
+ if (!handler_enabled(NONNULL_ARG))
+ return;
+
+ if (current->in_ubsan)
+ return;
+
+ if (is_disabled(&data->location))
+ return;
+
+ ubsan_prologue(&data->location);
+
+ 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();
+}
+EXPORT_SYMBOL(__ubsan_handle_nonnull_arg);
+
+void __ubsan_handle_nonnull_return(struct nonnull_return_data *data)
+{
+ if (!handler_enabled(NONNULL_RET))
+ return;
+
+ if (current->in_ubsan)
+ return;
+
+ if (is_disabled(&data->location))
+ return;
+
+ ubsan_prologue(&data->location);
+
+ 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();
+}
+EXPORT_SYMBOL(__ubsan_handle_nonnull_return);
+
+void __ubsan_handle_vla_bound_not_positive(struct vla_bound_data *data,
+ unsigned long bound)
+{
+ char bound_str[60];
+
+ if (!handler_enabled(VLA_BOUND))
+ return;
+
+ if (current->in_ubsan)
+ return;
+
+ if (is_disabled(&data->location))
+ return;
+
+ ubsan_prologue(&data->location);
+
+ 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();
+}
+EXPORT_SYMBOL(__ubsan_handle_vla_bound_not_positive);
+
+void __ubsan_handle_out_of_bounds(struct out_of_bounds_data *data,
+ unsigned long index)
+{
+ char index_str[60];
+
+ if (!handler_enabled(OUT_OF_BOUNDS))
+ return;
+
+ if (current->in_ubsan)
+ return;
+
+ if (is_disabled(&data->location))
+ return;
+
+ ubsan_prologue(&data->location);
+
+ val_to_string(index_str, sizeof(index_str), data->index_type, index);
+ pr_err("index %s out of bounds for type %s\n", index_str,
+ data->array_type->type_name);
+
+ ubsan_epilogue();
+}
+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)
+{
+ struct type_descriptor *rhs_type = data->rhs_type;
+ struct type_descriptor *lhs_type = data->lhs_type;
+ char rhs_str[60];
+ char lhs_str[60];
+
+ if (!handler_enabled(SHIFT_OUT_OF_BOUNDS))
+ return;
+
+ if (current->in_ubsan)
+ return;
+
+ if (is_disabled(&data->location))
+ return;
+
+ ubsan_prologue(&data->location);
+
+ 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 to 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();
+}
+EXPORT_SYMBOL(__ubsan_handle_shift_out_of_bounds);
+
+void __ubsan_handle_load_invalid_value(struct invalid_value_data *data,
+ unsigned long val)
+{
+ char val_str[60];
+
+ if (!handler_enabled(INVALID_LOAD))
+ return;
+
+ if (current->in_ubsan)
+ return;
+
+ if (is_disabled(&data->location))
+ return;
+
+ ubsan_prologue(&data->location);
+
+ 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();
+}
+EXPORT_SYMBOL(__ubsan_handle_load_invalid_value);
+
+
+void __ubsan_handle_function_type_mismatch(struct func_type_mismatch_data *data,
+ unsigned long function)
+{
+
+ if (!handler_enabled(FUNC_TYPE_MISMATCH))
+ return;
+
+ if (current->in_ubsan)
+ return;
+
+ if (is_disabled(&data->location))
+ return;
+
+ ubsan_prologue(&data->location);
+
+ pr_err("call to function %pF through pointer to incorrect function type %s\n",
+ (void *)function, data->type->type_name);
+
+ ubsan_epilogue();
+}
+EXPORT_SYMBOL(__ubsan_handle_function_type_mismatch);
+
+static int __init setup_ubsan_handlers(char *str)
+{
+ ubsan_handle = 0;
+
+ for (; *str; str++) {
+ switch (tolower(*str)) {
+ case 'o':
+ enable_handler(SUM_OVERFLOW);
+ enable_handler(SUB_OVERFLOW);
+ enable_handler(MUL_OVERFLOW);
+ break;
+ case 'e':
+ enable_handler(NEG_OVERFLOW);
+ enable_handler(DIVREM_OVERFLOW);
+ break;
+ case 'a':
+ enable_handler(ALIGNMENT);
+ break;
+ case 'i':
+ enable_handler(OBJECT_SIZE);
+ break;
+ case 'n':
+ enable_handler(NONNULL_ARG);
+ enable_handler(NONNULL_RET);
+ enable_handler(NULL_PTR);
+ break;
+ case 'v':
+ enable_handler(VLA_BOUND);
+ break;
+ case 'b':
+ enable_handler(OUT_OF_BOUNDS);
+ break;
+ case 's':
+ enable_handler(SHIFT_OUT_OF_BOUNDS);
+ break;
+ case 'l':
+ enable_handler(INVALID_LOAD);
+ break;
+ case 'f':
+ enable_handler(FUNC_TYPE_MISMATCH);
+ break;
+ default:
+ pr_err("skipping unknown option '%c'\n", *str);
+ break;
+ }
+ }
+
+ return 0;
+}
+
+early_param("ubsan_handle", setup_ubsan_handlers);
diff --git a/lib/ubsan.h b/lib/ubsan.h
new file mode 100644
index 0000000..e2d8634
--- /dev/null
+++ b/lib/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;
+ 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;
+};
+
+struct func_type_mismatch_data {
+ struct source_location location;
+ struct type_descriptor *type;
+};
+
+#ifdef CONFIG_ARCH_SUPPORTS_INT128
+typedef __int128 s_max;
+typedef unsigned __int128 u_max;
+#else
+typedef s64 s_max;
+typedef u64 u_max;
+#endif
+
+#endif
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 54be19a..3f631b9 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -119,6 +119,12 @@ _c_flags += $(if $(patsubst n%,, \
$(CFLAGS_GCOV))
endif
+ifeq ($(CONFIG_UBSAN),y)
+_c_flags += $(if $(patsubst n%,, \
+ $(UBSAN_SANITIZE_$(basetarget).o)$(UBSAN_SANITIZE)$(CONFIG_UBSAN_SANITIZE_ALL)), \
+ $(CFLAGS_UBSAN))
+endif
+
# If building the kernel in a separate objtree expand all occurrences
# of -Idir to -I$(srctree)/dir except for absolute paths (starting with '/').
--
2.1.2
Hi, Theodore.
I've got this while booting kernel with ubsan:
[ 0.000000] ================================================================================
[ 0.000000] UBSan: Undefined behaviour in ../include/linux/bitops.h:107:33
[ 0.000000] shift exponent 32 is to large for 32-bit type 'unsigned int'
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 3.18.0-rc1+ #64
[ 0.000000] 0000000000000020 0000000000000000 0000000000000000 ffffffff83003c68
[ 0.000000] ffffffff82add58a 000000000000009f 0000000000000020 ffffffff83003c78
[ 0.000000] ffffffff819a4519 ffffffff83003d28 ffffffff819a4a05 ffffffff82df889d
[ 0.000000] Call Trace:
[ 0.000000] dump_stack (/home/andrew/linux/ubsan_x86//lib/dump_stackc:52)
[ 0.000000] ubsan_epilogue (/home/andrew/linux/ubsan_x86//lib/ubsanc:159)
[ 0.000000] __ubsan_handle_shift_out_of_bounds (/home/andrew/linux/ubsan_x86//lib/ubsanc:458)
[ 0.000000] ? dmi_string_nosave (/home/andrew/linux/ubsan_x86//drivers/firmware/dmi_scanc:51)
[ 0.000000] ? dmi_string (/home/andrew/linux/ubsan_x86//drivers/firmware/dmi_scanc:72)
[ 0.000000] ? dmi_save_ident (/home/andrew/linux/ubsan_x86//drivers/firmware/dmi_scanc:159)
[ 0.000000] _mix_pool_bytes (/home/andrew/linux/ubsan_x86//include/linux/bitopsh:107 /home/andrew/linux/ubsan_x86//drivers/char/randomc:509)
[ 0.000000] ? dmi_decode (/home/andrew/linux/ubsan_x86//drivers/firmware/dmi_scanc:376)
[ 0.000000] ? dmi_walk_early (/home/andrew/linux/ubsan_x86//drivers/firmware/dmi_scanc:127)
[ 0.000000] add_device_randomness (/home/andrew/linux/ubsan_x86//drivers/char/randomc:747)
[ 0.000000] ? dmi_save_one_device (/home/andrew/linux/ubsan_x86//drivers/firmware/dmi_scanc:375)
[ 0.000000] ? dmi_save_one_device (/home/andrew/linux/ubsan_x86//drivers/firmware/dmi_scanc:375)
[ 0.000000] dmi_walk_early (/home/andrew/linux/ubsan_x86//drivers/firmware/dmi_scanc:127)
[ 0.000000] dmi_present (/home/andrew/linux/ubsan_x86//drivers/firmware/dmi_scanc:497)
[ 0.000000] dmi_scan_machine (/home/andrew/linux/ubsan_x86//drivers/firmware/dmi_scanc:555)
[ 0.000000] setup_arch (/home/andrew/linux/ubsan_x86//arch/x86/kernel/setupc:1022)
[ 0.000000] ? printk (/home/andrew/linux/ubsan_x86//kernel/printk/printkc:1849)
[ 0.000000] ? early_idt_handlers (/home/andrew/linux/ubsan_x86//arch/x86/kernel/head_64S:344)
[ 0.000000] start_kernel (/home/andrew/linux/ubsan_x86//include/linux/bitmaph:162 /home/andrew/linux/ubsan_x86//include/linux/cpumaskh:333
/home/andrew/linux/ubsan_x86//include/linux/mm_typesh:464 /home/andrew/linux/ubsan_x86//init/mainc:532)
[ 0.000000] ? early_idt_handlers (/home/andrew/linux/ubsan_x86//arch/x86/kernel/head_64S:344)
[ 0.000000] x86_64_start_reservations (/home/andrew/linux/ubsan_x86//arch/x86/kernel/head64c:194)
[ 0.000000] x86_64_start_kernel (/home/andrew/linux/ubsan_x86//arch/x86/kernel/head64c:183)
[ 0.000000] ================================================================================
On kernel with UBSan enabled I've got following:
UBSan: Undefined behaviour in ../kernel/time/clockevents.c:75:34
shift exponent 32 is to large for 32-bit type 'unsigned int'
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.17.0-rc7+ #39
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
0000000000000000 0000000000000000 0000000000000001 ffffffff83003db0
ffffffff82a30940 0000000000000020 ffffffff83003dc0 ffffffff819502e9
ffffffff83003e40 ffffffff81950735 ffff88013f003233 0000000000000000
Call Trace:
dump_stack (/home/andrew/linux/lib/dump_stack.c:52)
ubsan_epilogue (/home/andrew/linux/lib/ubsan.c:122)
__ubsan_handle_shift_out_of_bounds (/home/andrew/linux/lib/ubsan.c:390)
? hpet_enable (/home/andrew/linux/arch/x86/kernel/hpet.c:862)
cev_delta2ns (/home/andrew/linux/kernel/time/clockevents.c:75 (discriminator 1))
clockevents_config.part.2 (/home/andrew/linux/kernel/time/clockevents.c:421)
? __clocksource_select (/home/andrew/linux/kernel/time/clocksource.c:607 /home/andrew/linux/kernel/time/clocksource.c:631)
clockevents_config_and_register (/home/andrew/linux/kernel/time/clockevents.c:440)
hpet_enable (/home/andrew/linux/arch/x86/kernel/hpet.c:305 /home/andrew/linux/arch/x86/kernel/hpet.c:891)
hpet_time_init (/home/andrew/linux/arch/x86/kernel/time.c:79)
x86_late_time_init (/home/andrew/linux/arch/x86/kernel/time.c:87)
start_kernel (/home/andrew/linux/init/main.c:637)
? early_idt_handlers (/home/andrew/linux/arch/x86/kernel/head_64.S:344)
x86_64_start_reservations (/home/andrew/linux/arch/x86/kernel/head64.c:194)
x86_64_start_kernel (/home/andrew/linux/arch/x86/kernel/head64.c:183)
I guess it should be 1ULL here instead of 1U:
(!ismax || evt->mult <= (1U << evt->shift)))
Hi
I've got the following spew on mounting ext4 rootfs on kernel with UBSan:
================================================================================
UBSan: Undefined behaviour in ../fs/ext4/mballoc.c:2589:15
shift exponent -1 is negative
CPU: 2 PID: 1 Comm: swapper/0 Not tainted 3.18.0-rc1+ #65
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
0000000000000010 0000000000000000 0000000000000001 ffff88013ab0f998
ffffffff82ade70a 0000000000000082 ffffffffffffffff ffff88013ab0f9a8
ffffffff819a5339 ffff88013ab0fa58 ffffffff819a5825 ffff8800bb0c7840
Call Trace:
dump_stack (/home/andrew/linux/ubsan_x86//lib/dump_stackc:52)
ubsan_epilogue (/home/andrew/linux/ubsan_x86//lib/ubsanc:159)
__ubsan_handle_shift_out_of_bounds (/home/andrew/linux/ubsan_x86//lib/ubsanc:458)
? e1000_phy_get_info (/home/andrew/linux/ubsan_x86//drivers/net/ethernet/intel/e1000/e1000_hwc:3385 /home/andrew/linux/ubsan_x86//drivers/net/ethernet/intel/e1000/e1000_hwc:3455)
? put_online_cpus (/home/andrew/linux/ubsan_x86//kernel/cpuc:126)
? kmem_cache_create (/home/andrew/linux/ubsan_x86//mm/slab_commonc:431)
ext4_mb_init (/home/andrew/linux/ubsan_x86//fs/ext4/mballocc:2589 (discriminator 1))
? ext4_setup_system_zone (/home/andrew/linux/ubsan_x86//fs/ext4/block_validityc:150)
ext4_fill_super (/home/andrew/linux/ubsan_x86//fs/ext4/superc:4101)
? register_shrinker (/home/andrew/linux/ubsan_x86//mm/vmscanc:207)
mount_bdev (/home/andrew/linux/ubsan_x86//fs/superc:1004)
? ext4_calculate_overhead (/home/andrew/linux/ubsan_x86//fs/ext4/superc:3374)
ext4_mount (/home/andrew/linux/ubsan_x86//fs/ext4/superc:5404)
mount_fs (/home/andrew/linux/ubsan_x86//fs/superc:1106)
vfs_kern_mount (/home/andrew/linux/ubsan_x86//fs/namespacec:908)
do_mount (/home/andrew/linux/ubsan_x86//fs/namespacec:2292 /home/andrew/linux/ubsan_x86//fs/namespacec:2607)
SyS_mount (/home/andrew/linux/ubsan_x86//fs/namespacec:2799 /home/andrew/linux/ubsan_x86//fs/namespacec:2774)
mount_block_root (/home/andrew/linux/ubsan_x86//init/do_mountsc:364 /home/andrew/linux/ubsan_x86//init/do_mountsc:393)
? done_path_create (/home/andrew/linux/ubsan_x86//fs/nameic:3366)
mount_root (/home/andrew/linux/ubsan_x86//init/do_mountsc:534)
prepare_namespace (/home/andrew/linux/ubsan_x86//init/do_mountsc:592)
kernel_init_freeable (/home/andrew/linux/ubsan_x86//init/mainc:903 /home/andrew/linux/ubsan_x86//init/mainc:1032)
? rest_init (/home/andrew/linux/ubsan_x86//init/mainc:931)
kernel_init (/home/andrew/linux/ubsan_x86//init/mainc:936)
ret_from_fork (/home/andrew/linux/ubsan_x86//arch/x86/kernel/entry_64S:348)
? rest_init (/home/andrew/linux/ubsan_x86//init/mainc:931)
================================================================================
And similar in another place:
================================================================================
UBSan: Undefined behaviour in ../fs/ext4/mballoc.c:1263:11
shift exponent -1 is negative
CPU: 2 PID: 1426 Comm: mktemp Not tainted 3.18.0-rc1+ #65
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
0000000000000010 0000000000000000 0000000000000001 ffff8800babe7778
ffffffff82ade70a 00000000000000bc ffffffffffffffff ffff8800babe7788
ffffffff819a5339 ffff8800babe7838 ffffffff819a5825 ffff8800bb11ae68
Call Trace:
dump_stack (/home/andrew/linux/ubsan_x86//lib/dump_stackc:52)
ubsan_epilogue (/home/andrew/linux/ubsan_x86//lib/ubsanc:159)
__ubsan_handle_shift_out_of_bounds (/home/andrew/linux/ubsan_x86//lib/ubsanc:458)
? ext4_mb_init_cache (/home/andrew/linux/ubsan_x86//include/linux/buffer_headh:286 /home/andrew/linux/ubsan_x86//fs/ext4/mballocc:961)
mb_find_order_for_block (/home/andrew/linux/ubsan_x86//fs/ext4/mballocc:1263 (discriminator 1))
mb_find_extent (/home/andrew/linux/ubsan_x86//fs/ext4/mballocc:1512)
ext4_mb_complex_scan_group (/home/andrew/linux/ubsan_x86//fs/ext4/mballocc:1947)
? pagecache_get_page (/home/andrew/linux/ubsan_x86//mm/filemapc:1102)
? ext4_mark_iloc_dirty (/home/andrew/linux/ubsan_x86//fs/ext4/inodec:4301 /home/andrew/linux/ubsan_x86//fs/ext4/inodec:4732)
ext4_mb_regular_allocator (/home/andrew/linux/ubsan_x86//fs/ext4/ext4h:1346 /home/andrew/linux/ubsan_x86//fs/ext4/ext4h:2516 /home/andrew/linux/ubsan_x86//fs/ext4/ext4h:2551
/home/andrew/linux/ubsan_x86//fs/ext4/mballocc:2185)
? ext4_get_group_no_and_offset (/home/andrew/linux/ubsan_x86//fs/ext4/ballocc:61)
ext4_mb_new_blocks (/home/andrew/linux/ubsan_x86//fs/ext4/mballocc:4475)
? ext4_inode_to_goal_block (/home/andrew/linux/ubsan_x86//fs/ext4/ballocc:870)
ext4_ext_map_blocks (/home/andrew/linux/ubsan_x86//fs/ext4/extentsc:4455)
ext4_map_blocks (/home/andrew/linux/ubsan_x86//fs/ext4/inodec:611)
? __ext4_new_inode (/home/andrew/linux/ubsan_x86//fs/ext4/iallocc:1061)
ext4_getblk (/home/andrew/linux/ubsan_x86//fs/ext4/inodec:751)
ext4_bread (/home/andrew/linux/ubsan_x86//fs/ext4/inodec:805)
ext4_append (/home/andrew/linux/ubsan_x86//fs/ext4/nameic:66 (discriminator 3))
ext4_mkdir (/home/andrew/linux/ubsan_x86//fs/ext4/nameic:2404 /home/andrew/linux/ubsan_x86//fs/ext4/nameic:2452)
? security_inode_permission (/home/andrew/linux/ubsan_x86//security/securityc:573)
vfs_mkdir (/home/andrew/linux/ubsan_x86//fs/nameic:3494)
SyS_mkdir (/home/andrew/linux/ubsan_x86//fs/nameic:3517 /home/andrew/linux/ubsan_x86//fs/nameic:3500 /home/andrew/linux/ubsan_x86//fs/nameic:3527 /home/andrew/linux/ubsan_x86//fs/nameic:3525)
system_call_fastpath (/home/andrew/linux/ubsan_x86//arch/x86/kernel/entry_64S:423)
================================================================================
And one more negative shift, this time in jbd2/revoke.c in hash() function:
================================================================================
UBSan: Undefined behaviour in ../fs/jbd2/revoke.c:142:9
shift exponent -4 is negative
CPU: 3 PID: 1314 Comm: runscript.sh Not tainted 3.18.0-rc1+ #65
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
0000000000000010 0000000000000000 0000000000000000 ffff8800baba3868
ffffffff82ade70a 000000000000004a fffffffffffffffc ffff8800baba3878
ffffffff819a5339 ffff8800baba3928 ffffffff819a5825 ffff8800baba38b8
Call Trace:
dump_stack (/home/andrew/linux/ubsan_x86//lib/dump_stackc:52)
ubsan_epilogue (/home/andrew/linux/ubsan_x86//lib/ubsanc:159)
__ubsan_handle_shift_out_of_bounds (/home/andrew/linux/ubsan_x86//lib/ubsanc:458)
find_revoke_record (/home/andrew/linux/ubsan_x86//fs/jbd2/revokec:142 /home/andrew/linux/ubsan_x86//fs/jbd2/revokec:180)
jbd2_journal_cancel_revoke (/home/andrew/linux/ubsan_x86//fs/jbd2/revokec:449)
do_get_write_access (/home/andrew/linux/ubsan_x86//fs/jbd2/transactionc:992)
jbd2_journal_get_write_access (/home/andrew/linux/ubsan_x86//fs/jbd2/transactionc:1022)
__ext4_journal_get_write_access (/home/andrew/linux/ubsan_x86//fs/ext4/ext4_jbd2c:159)
ext4_file_open (/home/andrew/linux/ubsan_x86//fs/ext4/filec:238)
do_dentry_open (/home/andrew/linux/ubsan_x86//fs/openc:722)
? __inode_permission (/home/andrew/linux/ubsan_x86//fs/nameic:418)
? ext4_check_all_de (/home/andrew/linux/ubsan_x86//fs/ext4/filec:209)
finish_open (/home/andrew/linux/ubsan_x86//fs/openc:784)
? may_open (/home/andrew/linux/ubsan_x86//fs/nameic:2572)
do_last (/home/andrew/linux/ubsan_x86//fs/nameic:3069)
? link_path_walk (/home/andrew/linux/ubsan_x86//fs/nameic:1495 /home/andrew/linux/ubsan_x86//fs/nameic:1757)
? inode_has_perm (/home/andrew/linux/ubsan_x86//security/selinux/hooksc:1620)
path_openat (/home/andrew/linux/ubsan_x86//fs/nameic:699 /home/andrew/linux/ubsan_x86//fs/nameic:3229)
do_filp_open (/home/andrew/linux/ubsan_x86//fs/nameic:3260)
? prepare_creds (/home/andrew/linux/ubsan_x86//kernel/credc:269)
do_open_exec (/home/andrew/linux/ubsan_x86//fs/execc:762)
do_execve_common.isra.21 (/home/andrew/linux/ubsan_x86//fs/execc:1476)
? getname_flags (/home/andrew/linux/ubsan_x86//fs/nameic:160)
SyS_execve (/home/andrew/linux/ubsan_x86//fs/execc:1604)
stub_execve (/home/andrew/linux/ubsan_x86//arch/x86/kernel/entry_64S:649)
================================================================================
On Mon, Oct 20, 2014 at 03:03:22PM +0400, Andrey Ryabinin wrote:
> Hi, Theodore.
>
> I've got this while booting kernel with ubsan:
>
> [ 0.000000] ================================================================================
> [ 0.000000] UBSan: Undefined behaviour in ../include/linux/bitops.h:107:33
> [ 0.000000] shift exponent 32 is to large for 32-bit type 'unsigned int'
...
> [ 0.000000] _mix_pool_bytes (/home/andrew/linux/ubsan_x86//include/linux/bitopsh:107 /home/andrew/linux/ubsan_x86//drivers/char/randomc:509)
So this doesn't make any sense to me. This is triggering here:
w = rol32(*bytes++, input_rotate);
.... but input_rotate should never be >= 32, since it is set this way:
input_rotate = (input_rotate + (i ? 7 : 14)) & 31;
Just to be sure I've tried adding a:
WARN_ON(input_rotate >= 32);
before the rol32 line, and it's not triggering for me after booting
under kvm using an i386 kernel.
Is this something you can reliably reproduce? Can you try putting a
WARN_ON before the rol32() on a kernel w/o usbsan, just to make sure
this isn't some kind of false positive? And then can you tell me
something more about the .config you are using to build your test kernel?
Thanks,
- Ted
On 10/20/2014 04:49 PM, Theodore Ts'o wrote:
> On Mon, Oct 20, 2014 at 03:03:22PM +0400, Andrey Ryabinin wrote:
>> Hi, Theodore.
>>
>> I've got this while booting kernel with ubsan:
>>
>> [ 0.000000] ================================================================================
>> [ 0.000000] UBSan: Undefined behaviour in ../include/linux/bitops.h:107:33
>> [ 0.000000] shift exponent 32 is to large for 32-bit type 'unsigned int'
> ...
>> [ 0.000000] _mix_pool_bytes (/home/andrew/linux/ubsan_x86//include/linux/bitopsh:107 /home/andrew/linux/ubsan_x86//drivers/char/randomc:509)
>
> So this doesn't make any sense to me. This is triggering here:
>
> w = rol32(*bytes++, input_rotate);
>
> .... but input_rotate should never be >= 32, since it is set this way:
>
It's triggering when input_rotate == 0, so UBSan complains about right shift in rol32()
static inline __u32 rol32(__u32 word, unsigned int shift)
{
return (word << shift) | (word >> (32 - shift));
}
> input_rotate = (input_rotate + (i ? 7 : 14)) & 31;
>
> Just to be sure I've tried adding a:
>
> WARN_ON(input_rotate >= 32);
>
> before the rol32 line, and it's not triggering for me after booting
> under kvm using an i386 kernel.
>
> Is this something you can reliably reproduce? Can you try putting a
> WARN_ON before the rol32() on a kernel w/o usbsan, just to make sure
> this isn't some kind of false positive? And then can you tell me
> something more about the .config you are using to build your test kernel?
>
> Thanks,
>
> - Ted
>
On Mon, Oct 20, 2014 at 05:58:25PM +0400, Andrey Ryabinin wrote:
> It's triggering when input_rotate == 0, so UBSan complains about right shift in rol32()
>
> static inline __u32 rol32(__u32 word, unsigned int shift)
> {
> return (word << shift) | (word >> (32 - shift));
> }
Ah, thanks; I don't know why I didn't see that.
So the only real question is whether we fix it in the call to rol32,
or in the rol32 function itself (i.e):
static inline __u32 rol32(__u32 word, unsigned int shift)
{
return shift ? ((word << shift) | (word >> (32 - shift))) : word;
}
Dos that make sense to everyone?
- Ted
On 10/20/2014 03:58 PM, Andrey Ryabinin wrote:
> On 10/20/2014 04:49 PM, Theodore Ts'o wrote:
>> On Mon, Oct 20, 2014 at 03:03:22PM +0400, Andrey Ryabinin wrote:
>>> Hi, Theodore.
>>>
>>> I've got this while booting kernel with ubsan:
>>>
>>> [ 0.000000] ================================================================================
>>> [ 0.000000] UBSan: Undefined behaviour in ../include/linux/bitops.h:107:33
>>> [ 0.000000] shift exponent 32 is to large for 32-bit type 'unsigned int'
>> ...
>>> [ 0.000000] _mix_pool_bytes (/home/andrew/linux/ubsan_x86//include/linux/bitopsh:107 /home/andrew/linux/ubsan_x86//drivers/char/randomc:509)
>>
>> So this doesn't make any sense to me. This is triggering here:
>>
>> w = rol32(*bytes++, input_rotate);
>>
>> .... but input_rotate should never be >= 32, since it is set this way:
>>
>
> It's triggering when input_rotate == 0, so UBSan complains about right shift in rol32()
>
> static inline __u32 rol32(__u32 word, unsigned int shift)
> {
> return (word << shift) | (word >> (32 - shift));
> }
So that would be the case when the entropy store's input_rotate calls
_mix_pool_bytes() for the very first time ... I don't think it's an
issue though.
On 10/20/2014 10:09 AM, Daniel Borkmann wrote:
> On 10/20/2014 03:58 PM, Andrey Ryabinin wrote:
>> On 10/20/2014 04:49 PM, Theodore Ts'o wrote:
>>> On Mon, Oct 20, 2014 at 03:03:22PM +0400, Andrey Ryabinin wrote:
>>>> Hi, Theodore.
>>>>
>>>> I've got this while booting kernel with ubsan:
>>>>
>>>> [ 0.000000] ================================================================================
>>>> [ 0.000000] UBSan: Undefined behaviour in ../include/linux/bitops.h:107:33
>>>> [ 0.000000] shift exponent 32 is to large for 32-bit type 'unsigned int'
>>> ...
>>>> [ 0.000000] _mix_pool_bytes (/home/andrew/linux/ubsan_x86//include/linux/bitopsh:107 /home/andrew/linux/ubsan_x86//drivers/char/randomc:509)
>>>
>>> So this doesn't make any sense to me. This is triggering here:
>>>
>>> w = rol32(*bytes++, input_rotate);
>>>
>>> .... but input_rotate should never be >= 32, since it is set this way:
>>>
>>
>> It's triggering when input_rotate == 0, so UBSan complains about right shift in rol32()
>>
>> static inline __u32 rol32(__u32 word, unsigned int shift)
>> {
>> return (word << shift) | (word >> (32 - shift));
>> }
>
> So that would be the case when the entropy store's input_rotate calls
> _mix_pool_bytes() for the very first time ... I don't think it's an
> issue though.
It's an issue because it's _undefined_. For all you know gcc could return
whatever it wants as the result (42?) - no one promises you a "0" there.
Thanks,
Sasha
On Mon, Oct 20, 2014 at 04:09:30PM +0200, Daniel Borkmann wrote:
> >
> >It's triggering when input_rotate == 0, so UBSan complains about right shift in rol32()
> >
> >static inline __u32 rol32(__u32 word, unsigned int shift)
> >{
> > return (word << shift) | (word >> (32 - shift));
> >}
>
> So that would be the case when the entropy store's input_rotate calls
> _mix_pool_bytes() for the very first time ... I don't think it's an
> issue though.
I'm sure it's not an issue, but it's still true that
return (word << 0) | (word >> 32);
is technically not undefined, and while it would be unfortunate (and
highly unlikely) if gcc were to say, start nethack, it's technically
allowed by the C spec. :-)
- Ted
On 10/20/2014 06:16 PM, Theodore Ts'o wrote:
> On Mon, Oct 20, 2014 at 04:09:30PM +0200, Daniel Borkmann wrote:
>>>
>>> It's triggering when input_rotate == 0, so UBSan complains about right shift in rol32()
>>>
>>> static inline __u32 rol32(__u32 word, unsigned int shift)
>>> {
>>> return (word << shift) | (word >> (32 - shift));
>>> }
>>
>> So that would be the case when the entropy store's input_rotate calls
>> _mix_pool_bytes() for the very first time ... I don't think it's an
>> issue though.
>
> I'm sure it's not an issue, but it's still true that
>
> return (word << 0) | (word >> 32);
>
> is technically not undefined, and while it would be unfortunate (and
> highly unlikely) if gcc were to say, start nethack, it's technically
> allowed by the C spec. :-)
>
There are plenty of such technically not defined but really
harmless things in kernel.
I've reported only few UBs which seemed to me like a real bugs.
So if it's not an issue we could ignore it, OTOH it would be nice
to fix it to shut up ubsan.
> - Ted
>
On 10/20/2014 06:54 AM, Andrey Ryabinin wrote:
>
> UBSan uses compile-time instrumentation to catch undefined behavior (UB).
> Compiler inserts code that perform certain kinds of
> checks before operations that could cause UB.
> If check fails (i.e. UB detected) __ubsan_handle_* function called.
> to print error message.
>
> So the most of the work is done by compiler.
> This patch just implements ubsan handlers printing errors.
>
> GCC supports this since 4.9, however upcoming GCC 5.0 has
> more checkers implemented.
>
> Different kinds of checks could be enabled via boot parameter:
> ubsan_handle=OEAINVBSLF.
> If ubsan_handle not present in cmdline default options are used: ELNVBSLF
>
> O - different kinds of overflows
> E - negation overflow, division overflow, division by zero.
> A - misaligned memory access.
> I - load from/store to an object with insufficient space.
> N - null argument declared with nonnull attribute,
> returned null from function which never returns null, null ptr dereference.
> V - variable size array with non-positive length
> B - out-of-bounds accesses.
> S - shifting out-of-bounds.
> L - load of invalid value (value out of range for the enum type, loading other then 0/1 to bool type)
> F - call to function through pointer with incorrect function type
> (AFAIK this is not implemented in gcc yet, probably works with clang, though
> I didn't check ubsan with clang at all).
>
> Instrumentation in kernel/printk/printk.c is disabled because struct printk_log is not properly aligned,
> therefore we are recursively taking logbuf_lock while trying to print error in __ubsan_handle*().
>
> Signed-off-by: Andrey Ryabinin <[email protected]>
> ---
> Makefile | 12 +-
> arch/x86/Kconfig | 1 +
> arch/x86/boot/Makefile | 1 +
> arch/x86/boot/compressed/Makefile | 1 +
> arch/x86/realmode/rm/Makefile | 1 +
> arch/x86/vdso/Makefile | 2 +
> drivers/firmware/efi/libstub/Makefile | 1 +
> include/linux/sched.h | 4 +
> kernel/printk/Makefile | 1 +
> lib/Kconfig.debug | 23 ++
> lib/Makefile | 3 +
> lib/ubsan.c | 559 ++++++++++++++++++++++++++++++++++
> lib/ubsan.h | 84 +++++
> scripts/Makefile.lib | 6 +
> 14 files changed, 698 insertions(+), 1 deletion(-)
> create mode 100644 lib/ubsan.c
> create mode 100644 lib/ubsan.h
>
> diff --git a/Makefile b/Makefile
> index 05d67af..d3e23f9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -377,6 +377,9 @@ LDFLAGS_MODULE =
> CFLAGS_KERNEL =
> AFLAGS_KERNEL =
> CFLAGS_GCOV = -fprofile-arcs -ftest-coverage
> +CFLAGS_UBSAN = $(call cc-option, -fsanitize=undefined) \
> + $(call cc-option, -fno-sanitize=unreachable) \
> + $(call cc-option, -fno-sanitize=float-cast-overflow)
What's the reason behind those two -fno-sanitize?
[snip]
> +config HAVE_ARCH_UBSAN_SANTIZE_ALL
> + bool
> +
> +config UBSAN
> + bool "Undefined behaviour sanity checker"
> + help
> + This option enables undefined behaviour sanity checker
> + Compile-time instrumentataion used to detect various undefined
instrumentation
> + behaviours in runtime. Different kinds of checks could be enabled
> + via boot parameter ubsan_handle (see: Documentation/ubsan.txt).
> + (TODO: write docs).
> +
> +config UBSAN_SANITIZE_ALL
> + bool "Enable instrumentation for the entire kernel"
> + depends on UBSAN
> + depends on HAVE_ARCH_UBSAN_SANTIZE_ALL
> + default y
> + help
> + This option acitivates instrumentation for the entire kernel.
activates
> + If you don't enable this option, you have to explicitly specify
> + UBSAN_SANITIZE := y for the files/directories you want to check for UB.
> +
> +
[snip
> +/* By default enable everything except signed overflows and
> + * misaligned accesses
> + */
Why those two are disabled? Maybe we should be fixing them rather
than ignoring?
> +static unsigned long ubsan_handle = GENMASK(HANDLERS_END, 0) &
> + ~(BIT_MASK(SUM_OVERFLOW) | BIT_MASK(SUB_OVERFLOW) |
> + BIT_MASK(NEG_OVERFLOW) | BIT_MASK(ALIGNMENT));
> +
> +static void enable_handler(unsigned int handler)
> +{
> + set_bit(handler, &ubsan_handle);
> +}
> +
> +static bool handler_enabled(unsigned int handler)
> +{
> + return test_bit(handler, &ubsan_handle);
> +}
> +
> +#define REPORTED_BIT 31
> +#define COLUMN_MASK (~(1U << REPORTED_BIT))
> +
> +static bool is_disabled(struct source_location *location)
> +{
> + return test_and_set_bit(REPORTED_BIT,
> + (unsigned long *)&location->column);
> +}
> +
> +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, loc->column & COLUMN_MASK);
> +}
> +
> +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)
> +{
> + return type_is_int(type) && (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);
> +
Shouldn't we check type_is_int() here as well?
> + return bits <= inline_bits;
> +}
[snip]
> +void __ubsan_handle_negate_overflow(struct overflow_data *data,
> + unsigned long old_val)
> +{
> +
> + char old_val_str[60];
> +
> + if (!handler_enabled(NEG_OVERFLOW))
> + return;
> +
> + if (current->in_ubsan)
> + return;
> +
> + if (is_disabled(&data->location))
> + return;
This pattern of 3 if()s is repeating itself couple of times, maybe
it deserves a function of it's own?
> + ubsan_prologue(&data->location);
> +
> + 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();
> +}
> +EXPORT_SYMBOL(__ubsan_handle_negate_overflow);
Thanks,
Sasha
On 10/20/2014 11:35 PM, Sasha Levin wrote:
> On 10/20/2014 06:54 AM, Andrey Ryabinin wrote:
>>
>> UBSan uses compile-time instrumentation to catch undefined behavior (UB).
>> Compiler inserts code that perform certain kinds of
>> checks before operations that could cause UB.
>> If check fails (i.e. UB detected) __ubsan_handle_* function called.
>> to print error message.
>>
>> So the most of the work is done by compiler.
>> This patch just implements ubsan handlers printing errors.
>>
>> GCC supports this since 4.9, however upcoming GCC 5.0 has
>> more checkers implemented.
>>
>> Different kinds of checks could be enabled via boot parameter:
>> ubsan_handle=OEAINVBSLF.
>> If ubsan_handle not present in cmdline default options are used: ELNVBSLF
>>
>> O - different kinds of overflows
>> E - negation overflow, division overflow, division by zero.
>> A - misaligned memory access.
>> I - load from/store to an object with insufficient space.
>> N - null argument declared with nonnull attribute,
>> returned null from function which never returns null, null ptr dereference.
>> V - variable size array with non-positive length
>> B - out-of-bounds accesses.
>> S - shifting out-of-bounds.
>> L - load of invalid value (value out of range for the enum type, loading other then 0/1 to bool type)
>> F - call to function through pointer with incorrect function type
>> (AFAIK this is not implemented in gcc yet, probably works with clang, though
>> I didn't check ubsan with clang at all).
>>
>> Instrumentation in kernel/printk/printk.c is disabled because struct printk_log is not properly aligned,
>> therefore we are recursively taking logbuf_lock while trying to print error in __ubsan_handle*().
>>
>> Signed-off-by: Andrey Ryabinin <[email protected]>
>> ---
>> Makefile | 12 +-
>> arch/x86/Kconfig | 1 +
>> arch/x86/boot/Makefile | 1 +
>> arch/x86/boot/compressed/Makefile | 1 +
>> arch/x86/realmode/rm/Makefile | 1 +
>> arch/x86/vdso/Makefile | 2 +
>> drivers/firmware/efi/libstub/Makefile | 1 +
>> include/linux/sched.h | 4 +
>> kernel/printk/Makefile | 1 +
>> lib/Kconfig.debug | 23 ++
>> lib/Makefile | 3 +
>> lib/ubsan.c | 559 ++++++++++++++++++++++++++++++++++
>> lib/ubsan.h | 84 +++++
>> scripts/Makefile.lib | 6 +
>> 14 files changed, 698 insertions(+), 1 deletion(-)
>> create mode 100644 lib/ubsan.c
>> create mode 100644 lib/ubsan.h
>>
>> diff --git a/Makefile b/Makefile
>> index 05d67af..d3e23f9 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -377,6 +377,9 @@ LDFLAGS_MODULE =
>> CFLAGS_KERNEL =
>> AFLAGS_KERNEL =
>> CFLAGS_GCOV = -fprofile-arcs -ftest-coverage
>> +CFLAGS_UBSAN = $(call cc-option, -fsanitize=undefined) \
>> + $(call cc-option, -fno-sanitize=unreachable) \
>> + $(call cc-option, -fno-sanitize=float-cast-overflow)
>
> What's the reason behind those two -fno-sanitize?
Both not implemented.
float-cast-overflow is for floating point arithmetic which we don't have in kernel.
This could be removed safely without needing to implement __ubsan_handle_float_cast_overflow().
fsanitize=unreachable is for catching calls to __builtin_unreachable().
>> +config HAVE_ARCH_UBSAN_SANTIZE_ALL
>> + bool
>> +
>> +config UBSAN
>> + bool "Undefined behaviour sanity checker"
>> + help
>> + This option enables undefined behaviour sanity checker
>> + Compile-time instrumentataion used to detect various undefined
> instrumentation
>> + behaviours in runtime. Different kinds of checks could be enabled
>> + via boot parameter ubsan_handle (see: Documentation/ubsan.txt).
>> + (TODO: write docs).
>> +
>> +config UBSAN_SANITIZE_ALL
>> + bool "Enable instrumentation for the entire kernel"
>> + depends on UBSAN
>> + depends on HAVE_ARCH_UBSAN_SANTIZE_ALL
>> + default y
>> + help
>> + This option acitivates instrumentation for the entire kernel.
> activates
>> + If you don't enable this option, you have to explicitly specify
>> + UBSAN_SANITIZE := y for the files/directories you want to check for UB.
>> +
>> +
> [snip
>
>> +/* By default enable everything except signed overflows and
>> + * misaligned accesses
>> + */
>
> Why those two are disabled? Maybe we should be fixing them rather
> than ignoring?
>
Signed overflows are disabled because they are allowed in linux kernel. Using -fno-strict-alliasing
disables compiler's optimization based on assumption that signed overflow never happens. Though this
option doesn't make signed overflows defined, it was proven by years that it just works.
There is -fwrapv option which make signed overflows defined, but it's not used because it bogus on
some versions of compilers.
Unaligned accesses disabled because they are allowed on some arches (see HAVE_EFFICIENT_UNALIGNED_ACCESS).
Another reason is that there are to many reports. Not because there are lot of bugs, but because
there are many reports for one bug.
For example take a look at struct irqaction. It declared with ____cacheline_internodealigned_in_smp.
And look at the request_threaded_irq():
action = kzalloc(sizeof(struct irqaction), GFP_KERNEL);
if (!action)
return -ENOMEM;
action->handler = handler;
action->thread_fn = thread_fn;
action->flags = irqflags;
action->name = devname;
action->dev_id = dev_id;
This struct allocated via kmalloc which is wrong because in general kmalloc guaranties only sizeof(void *) alignment.
UBSan print report only once per source location, but there are many places where 'action' from above referenced.
Just after allocation where we fill struct's fields there will be 5 reports.
>> +static unsigned long ubsan_handle = GENMASK(HANDLERS_END, 0) &
>> + ~(BIT_MASK(SUM_OVERFLOW) | BIT_MASK(SUB_OVERFLOW) |
>> + BIT_MASK(NEG_OVERFLOW) | BIT_MASK(ALIGNMENT));
Oops, s/NEG_OVERFLOW/MULL_OVERFLOW
>> +
>> +static void enable_handler(unsigned int handler)
>> +{
>> + set_bit(handler, &ubsan_handle);
>> +}
>> +
>> +static bool handler_enabled(unsigned int handler)
>> +{
>> + return test_bit(handler, &ubsan_handle);
>> +}
>> +
>> +#define REPORTED_BIT 31
>> +#define COLUMN_MASK (~(1U << REPORTED_BIT))
>> +
>> +static bool is_disabled(struct source_location *location)
>> +{
>> + return test_and_set_bit(REPORTED_BIT,
>> + (unsigned long *)&location->column);
>> +}
>> +
>> +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, loc->column & COLUMN_MASK);
>> +}
>> +
>> +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)
>> +{
>> + return type_is_int(type) && (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);
>> +
>
> Shouldn't we check type_is_int() here as well?
>
It won't hurt.
This actually shouldn't be called for any other type than integer types,
so it deserves WARN_ON(!type_is_int());
>> + return bits <= inline_bits;
>> +}
>
> [snip]
>
>> +void __ubsan_handle_negate_overflow(struct overflow_data *data,
>> + unsigned long old_val)
>> +{
>> +
>> + char old_val_str[60];
>> +
>> + if (!handler_enabled(NEG_OVERFLOW))
>> + return;
>> +
>> + if (current->in_ubsan)
>> + return;
>> +
>> + if (is_disabled(&data->location))
>> + return;
>
> This pattern of 3 if()s is repeating itself couple of times, maybe
> it deserves a function of it's own?
>
Definitely.
>> + ubsan_prologue(&data->location);
>> +
>> + 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();
>> +}
>> +EXPORT_SYMBOL(__ubsan_handle_negate_overflow);
>
>
> Thanks,
> Sasha
>
>
On Mon, Oct 20, 2014 at 02:54:59PM +0400, Andrey Ryabinin wrote:
>
> UBSan uses compile-time instrumentation to catch undefined behavior (UB).
> Compiler inserts code that perform certain kinds of
> checks before operations that could cause UB.
> If check fails (i.e. UB detected) __ubsan_handle_* function called.
> to print error message.
>
> So the most of the work is done by compiler.
> This patch just implements ubsan handlers printing errors.
>
> GCC supports this since 4.9, however upcoming GCC 5.0 has
> more checkers implemented.
It might be useful if you've got a link to the relevant GCC
documentation of this new shiny stuf.
On 10/21/2014 01:47 PM, Peter Zijlstra wrote:
> On Mon, Oct 20, 2014 at 02:54:59PM +0400, Andrey Ryabinin wrote:
>>
>> UBSan uses compile-time instrumentation to catch undefined behavior (UB).
>> Compiler inserts code that perform certain kinds of
>> checks before operations that could cause UB.
>> If check fails (i.e. UB detected) __ubsan_handle_* function called.
>> to print error message.
>>
>> So the most of the work is done by compiler.
>> This patch just implements ubsan handlers printing errors.
>>
>> GCC supports this since 4.9, however upcoming GCC 5.0 has
>> more checkers implemented.
>
> It might be useful if you've got a link to the relevant GCC
> documentation of this new shiny stuf.
>
Documentation is very brief (look for fsanitize=undefined): https://gcc.gnu.org/onlinedocs/gcc/Debugging-Options.html
GCC 4.9.1 doc: https://gcc.gnu.org/onlinedocs/gcc-4.9.1/gcc/Debugging-Options.html
And there is an article which might be interesting: http://developerblog.redhat.com/2014/10/16/gcc-undefined-behavior-sanitizer-ubsan/
On 10/20/14 03:54, Andrey Ryabinin wrote:
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 4e35a5d..7dc9b89 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -646,6 +646,29 @@ config DEBUG_SHIRQ
> Drivers ought to be able to handle interrupts coming in at those
> points; some don't and need to be caught.
>
> +config HAVE_ARCH_UBSAN_SANTIZE_ALL
> + bool
> +
> +config UBSAN
> + bool "Undefined behaviour sanity checker"
> + help
> + This option enables undefined behaviour sanity checker
checker.
> + Compile-time instrumentataion used to detect various undefined
instrumentation is used
> + behaviours in runtime. Different kinds of checks could be enabled
prefer: may be enabled
> + via boot parameter ubsan_handle (see: Documentation/ubsan.txt).
> + (TODO: write docs).
> +
> +config UBSAN_SANITIZE_ALL
> + bool "Enable instrumentation for the entire kernel"
> + depends on UBSAN
> + depends on HAVE_ARCH_UBSAN_SANTIZE_ALL
> + default y
> + help
> + This option acitivates instrumentation for the entire kernel.
activates
> + If you don't enable this option, you have to explicitly specify
> + UBSAN_SANITIZE := y for the files/directories you want to check for UB.
> +
> +
> menu "Debug Lockups and Hangs"
>
> config LOCKUP_DETECTOR
--
~Randy
On Mon, Oct 20 2014, Andrey Ryabinin <[email protected]> wrote:
> UBSan uses compile-time instrumentation to catch undefined behavior (UB).
> Compiler inserts code that perform certain kinds of
> checks before operations that could cause UB.
> If check fails (i.e. UB detected) __ubsan_handle_* function called.
> to print error message.
>
> So the most of the work is done by compiler.
> This patch just implements ubsan handlers printing errors.
>
> GCC supports this since 4.9, however upcoming GCC 5.0 has
> more checkers implemented.
[...]
> +
> +#define REPORTED_BIT 31
> +#define COLUMN_MASK (~(1U << REPORTED_BIT))
> +
> +static bool is_disabled(struct source_location *location)
> +{
> + return test_and_set_bit(REPORTED_BIT,
> + (unsigned long *)&location->column);
> +}
[...]
> +struct source_location {
> + const char *file_name;
> + u32 line;
> + u32 column;
> +};
AFAICT, this introduces UB and/or memory corruption on big-endian
systems with BITS_PER_LONG==64. (Also, on both LE and BE 64 bit systems,
there's the issue of the alignment of location->column, which is likely
to be 4-but-not-8 byte aligned).
Is the layout of struct source_location dictated by gcc?
Rasmus
On 10/22/2014 01:58 PM, Rasmus Villemoes wrote:
> On Mon, Oct 20 2014, Andrey Ryabinin <[email protected]> wrote:
>
>> UBSan uses compile-time instrumentation to catch undefined behavior (UB).
>> Compiler inserts code that perform certain kinds of
>> checks before operations that could cause UB.
>> If check fails (i.e. UB detected) __ubsan_handle_* function called.
>> to print error message.
>>
>> So the most of the work is done by compiler.
>> This patch just implements ubsan handlers printing errors.
>>
>> GCC supports this since 4.9, however upcoming GCC 5.0 has
>> more checkers implemented.
>
> [...]
>
>> +
>> +#define REPORTED_BIT 31
>> +#define COLUMN_MASK (~(1U << REPORTED_BIT))
>> +
>> +static bool is_disabled(struct source_location *location)
>> +{
>> + return test_and_set_bit(REPORTED_BIT,
>> + (unsigned long *)&location->column);
>> +}
>
> [...]
>
>> +struct source_location {
>> + const char *file_name;
>> + u32 line;
>> + u32 column;
>> +};
>
>
> AFAICT, this introduces UB and/or memory corruption on big-endian
> systems with BITS_PER_LONG==64. (Also, on both LE and BE 64 bit systems,
> there's the issue of the alignment of location->column, which is likely
> to be 4-but-not-8 byte aligned).
>
You are right. This should fix it:
diff --git a/lib/ubsan.c b/lib/ubsan.c
index 7788f47..cfdf017 100644
--- a/lib/ubsan.c
+++ b/lib/ubsan.c
@@ -53,18 +53,24 @@ static bool handler_enabled(unsigned int handler)
}
#define REPORTED_BIT 31
-#define COLUMN_MASK (~(1U << REPORTED_BIT))
+
+#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
static bool is_disabled(struct source_location *location)
{
- return test_and_set_bit(REPORTED_BIT,
- (unsigned long *)&location->column);
+ 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, loc->column & COLUMN_MASK);
+ loc->line & LINE_MASK, loc->column & COLUMN_MASK);
}
static bool type_is_int(struct type_descriptor *type)
diff --git a/lib/ubsan.h b/lib/ubsan.h
index e2d8634..8965591 100644
--- a/lib/ubsan.h
+++ b/lib/ubsan.h
@@ -15,8 +15,13 @@ struct type_descriptor {
struct source_location {
const char *file_name;
- u32 line;
- u32 column;
+ union {
+ unsigned long reported;
+ struct {
+ u32 line;
+ u32 column;
+ };
+ };
};
struct overflow_data {
> Is the layout of struct source_location dictated by gcc?
>
Yes.
> Rasmus
>
> Unaligned accesses disabled because they are allowed on some arches (see
HAVE_EFFICIENT_UNALIGNED_ACCESS).
> Another reason is that there are to many reports. Not because there are
> lot of bugs, but because
> there are many reports for one bug.
A side note - unaligned accesses would prevent KASan from doing it's job
well because instrumentation code relies on address alignment when
performing the check.
-Y
--
View this message in context: http://linux-kernel.2935.n7.nabble.com/RFC-UBSan-run-time-undefined-behavior-sanity-checker-tp965203p968477.html
Sent from the Linux Kernel mailing list archive at Nabble.com.
On Mon, Oct 20, 2014 at 10:16:35AM -0400, Theodore Ts'o wrote:
> On Mon, Oct 20, 2014 at 04:09:30PM +0200, Daniel Borkmann wrote:
> > >
> > >It's triggering when input_rotate == 0, so UBSan complains about right shift in rol32()
> > >
> > >static inline __u32 rol32(__u32 word, unsigned int shift)
> > >{
> > > return (word << shift) | (word >> (32 - shift));
> > >}
> >
> > So that would be the case when the entropy store's input_rotate calls
> > _mix_pool_bytes() for the very first time ... I don't think it's an
> > issue though.
>
> I'm sure it's not an issue, but it's still true that
>
> return (word << 0) | (word >> 32);
>
> is technically not undefined, and while it would be unfortunate (and
> highly unlikely) if gcc were to say, start nethack, it's technically
> allowed by the C spec. :-)
In fact, n >> 32 == n.
#include <stdio.h>
int main(int argc, char **argv)
{
int i = atoi(argv[1]);
int shift = atoi(argv[2]);
printf("%x\n", i >> shift);
return 0;
}
$ ./shift 5 32
5
On x86 at least the shift ops simply mask out the upper bits and
therefore the 32 == 0.
So you end up OR-ing the same value twice, which is harmless.
So no misbehaviour on the rol32() function.
I think I've ran into this before, in that case I did get fail because I
did indeed expect the 0 and things didn't work out.
On 10/24/2014 02:01 PM, Peter Zijlstra wrote:
> On Mon, Oct 20, 2014 at 10:16:35AM -0400, Theodore Ts'o wrote:
>> On Mon, Oct 20, 2014 at 04:09:30PM +0200, Daniel Borkmann wrote:
>>>>
>>>> It's triggering when input_rotate == 0, so UBSan complains about right shift in rol32()
>>>>
>>>> static inline __u32 rol32(__u32 word, unsigned int shift)
>>>> {
>>>> return (word << shift) | (word >> (32 - shift));
>>>> }
>>>
>>> So that would be the case when the entropy store's input_rotate calls
>>> _mix_pool_bytes() for the very first time ... I don't think it's an
>>> issue though.
>>
>> I'm sure it's not an issue, but it's still true that
>>
>> return (word << 0) | (word >> 32);
>>
>> is technically not undefined, and while it would be unfortunate (and
>> highly unlikely) if gcc were to say, start nethack, it's technically
>> allowed by the C spec. :-)
>
> In fact, n >> 32 == n.
>
> #include <stdio.h>
>
> int main(int argc, char **argv)
> {
> int i = atoi(argv[1]);
> int shift = atoi(argv[2]);
> printf("%x\n", i >> shift);
> return 0;
> }
>
> $ ./shift 5 32
> 5
>
> On x86 at least the shift ops simply mask out the upper bits and
> therefore the 32 == 0.
>
> So you end up OR-ing the same value twice, which is harmless.
>
> So no misbehaviour on the rol32() function.
>
E.g. on arm (i >> 32) == 0, so rol32() will also work as expected.
But what about other architectures?
On Mon, Oct 20, 2014 at 03:07:50PM +0400, Andrey Ryabinin wrote:
>
> On kernel with UBSan enabled I've got following:
>
> UBSan: Undefined behaviour in ../kernel/time/clockevents.c:75:34
> shift exponent 32 is to large for 32-bit type 'unsigned int'
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.17.0-rc7+ #39
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
> 0000000000000000 0000000000000000 0000000000000001 ffffffff83003db0
> ffffffff82a30940 0000000000000020 ffffffff83003dc0 ffffffff819502e9
> ffffffff83003e40 ffffffff81950735 ffff88013f003233 0000000000000000
> Call Trace:
> dump_stack (/home/andrew/linux/lib/dump_stack.c:52)
> ubsan_epilogue (/home/andrew/linux/lib/ubsan.c:122)
> __ubsan_handle_shift_out_of_bounds (/home/andrew/linux/lib/ubsan.c:390)
> ? hpet_enable (/home/andrew/linux/arch/x86/kernel/hpet.c:862)
> cev_delta2ns (/home/andrew/linux/kernel/time/clockevents.c:75 (discriminator 1))
> clockevents_config.part.2 (/home/andrew/linux/kernel/time/clockevents.c:421)
> ? __clocksource_select (/home/andrew/linux/kernel/time/clocksource.c:607 /home/andrew/linux/kernel/time/clocksource.c:631)
> clockevents_config_and_register (/home/andrew/linux/kernel/time/clockevents.c:440)
> hpet_enable (/home/andrew/linux/arch/x86/kernel/hpet.c:305 /home/andrew/linux/arch/x86/kernel/hpet.c:891)
> hpet_time_init (/home/andrew/linux/arch/x86/kernel/time.c:79)
> x86_late_time_init (/home/andrew/linux/arch/x86/kernel/time.c:87)
> start_kernel (/home/andrew/linux/init/main.c:637)
> ? early_idt_handlers (/home/andrew/linux/arch/x86/kernel/head_64.S:344)
> x86_64_start_reservations (/home/andrew/linux/arch/x86/kernel/head64.c:194)
> x86_64_start_kernel (/home/andrew/linux/arch/x86/kernel/head64.c:183)
>
> I guess it should be 1ULL here instead of 1U:
> (!ismax || evt->mult <= (1U << evt->shift)))
Probably so indeed, clocks_calc_mult_shift() has max return value of 32,
which will indeed trigger that overflow.
On Tue, Oct 21, 2014 at 02:09:14PM +0400, Andrey Ryabinin wrote:
> On 10/21/2014 01:47 PM, Peter Zijlstra wrote:
> > On Mon, Oct 20, 2014 at 02:54:59PM +0400, Andrey Ryabinin wrote:
> >>
> >> UBSan uses compile-time instrumentation to catch undefined behavior (UB).
> >> Compiler inserts code that perform certain kinds of
> >> checks before operations that could cause UB.
> >> If check fails (i.e. UB detected) __ubsan_handle_* function called.
> >> to print error message.
> >>
> >> So the most of the work is done by compiler.
> >> This patch just implements ubsan handlers printing errors.
> >>
> >> GCC supports this since 4.9, however upcoming GCC 5.0 has
> >> more checkers implemented.
> >
> > It might be useful if you've got a link to the relevant GCC
> > documentation of this new shiny stuf.
> >
>
> Documentation is very brief (look for fsanitize=undefined): https://gcc.gnu.org/onlinedocs/gcc/Debugging-Options.html
> GCC 4.9.1 doc: https://gcc.gnu.org/onlinedocs/gcc-4.9.1/gcc/Debugging-Options.html
>
> And there is an article which might be interesting: http://developerblog.redhat.com/2014/10/16/gcc-undefined-behavior-sanitizer-ubsan/
Thanks, please consider including these in the changelog of future
postings of this patch set.
2014-10-24 12:31 GMT+04:00 y.gribov <[email protected]>:
>> Unaligned accesses disabled because they are allowed on some arches (see
> HAVE_EFFICIENT_UNALIGNED_ACCESS).
>> Another reason is that there are to many reports. Not because there are
>> lot of bugs, but because
>> there are many reports for one bug.
>
> A side note - unaligned accesses would prevent KASan from doing it's job
> well because instrumentation code relies on address alignment when
> performing the check.
>
I guess it only matters for inline instrumentation, right?
Because in outline case I've taken care about unaligned accesses.
We could do following trick in Kconfig:
select HAVE_EFFICIENT_UNALIGNED_ACCESS if !KASAN
This will prevent a lot of unaligned accesses, but surely not all of them
--
Best regards,
Andrey Ryabinin
On 10/24/2014 06:01 AM, Peter Zijlstra wrote:
> On Mon, Oct 20, 2014 at 10:16:35AM -0400, Theodore Ts'o wrote:
>> > On Mon, Oct 20, 2014 at 04:09:30PM +0200, Daniel Borkmann wrote:
>>>> > > >
>>>> > > >It's triggering when input_rotate == 0, so UBSan complains about right shift in rol32()
>>>> > > >
>>>> > > >static inline __u32 rol32(__u32 word, unsigned int shift)
>>>> > > >{
>>>> > > > return (word << shift) | (word >> (32 - shift));
>>>> > > >}
>>> > >
>>> > > So that would be the case when the entropy store's input_rotate calls
>>> > > _mix_pool_bytes() for the very first time ... I don't think it's an
>>> > > issue though.
>> >
>> > I'm sure it's not an issue, but it's still true that
>> >
>> > return (word << 0) | (word >> 32);
>> >
>> > is technically not undefined, and while it would be unfortunate (and
>> > highly unlikely) if gcc were to say, start nethack, it's technically
>> > allowed by the C spec. :-)
> In fact, n >> 32 == n.
>
> #include <stdio.h>
>
> int main(int argc, char **argv)
> {
> int i = atoi(argv[1]);
> int shift = atoi(argv[2]);
> printf("%x\n", i >> shift);
> return 0;
> }
>
> $ ./shift 5 32
> 5
>
> On x86 at least the shift ops simply mask out the upper bits and
> therefore the 32 == 0.
>
> So you end up OR-ing the same value twice, which is harmless.
>
> So no misbehaviour on the rol32() function.
>
> I think I've ran into this before, in that case I did get fail because I
> did indeed expect the 0 and things didn't work out.
i >> 32 may happen to be "i", but is there anything that prevents the compiler
from returning, let's say, 42?
Thanks,
Sasha
On Fri, Oct 24, 2014 at 09:23:35AM -0400, Sasha Levin wrote:
>
> i >> 32 may happen to be "i", but is there anything that prevents the compiler
> from returning, let's say, 42?
Not really, although gcc seems to opt for the 'sane' option and emit the
instruction and let the arch figure out how to deal with it. Hence the
'fun' difference between x86 and ARM.
On 10/24/2014 09:42 AM, Peter Zijlstra wrote:
> On Fri, Oct 24, 2014 at 09:23:35AM -0400, Sasha Levin wrote:
>>
>> i >> 32 may happen to be "i", but is there anything that prevents the compiler
>> from returning, let's say, 42?
>
> Not really, although gcc seems to opt for the 'sane' option and emit the
> instruction and let the arch figure out how to deal with it. Hence the
> 'fun' difference between x86 and ARM.
It's interesting how many different views on undefined behaviour there are between
kernel folks.
Everything between Ted Ts'o saying that GCC can launch nethack on oversized shifts,
to DaveM saying he will file a GCC bug if the behaviour isn't sane w.r.t to memcpy().
Thanks,
Sasha
On Fri, Oct 24, 2014 at 7:04 PM, Sasha Levin <[email protected]> wrote:
> On 10/24/2014 09:42 AM, Peter Zijlstra wrote:
>> On Fri, Oct 24, 2014 at 09:23:35AM -0400, Sasha Levin wrote:
>>>
>>> i >> 32 may happen to be "i", but is there anything that prevents the compiler
>>> from returning, let's say, 42?
>>
>> Not really, although gcc seems to opt for the 'sane' option and emit the
>> instruction and let the arch figure out how to deal with it. Hence the
>> 'fun' difference between x86 and ARM.
>
> It's interesting how many different views on undefined behaviour there are between
> kernel folks.
>
> Everything between Ted Ts'o saying that GCC can launch nethack on oversized shifts,
> to DaveM saying he will file a GCC bug if the behaviour isn't sane w.r.t to memcpy().
One of the benefits of fixing such issues (or not letting them into
code in the first place) is just saving numerous hours of top-notch
engineers spent on disputes like this.
On Fri, 24 Oct 2014 19:10:49 +0400
Dmitry Vyukov <[email protected]> wrote:
> On Fri, Oct 24, 2014 at 7:04 PM, Sasha Levin <[email protected]> wrote:
> > On 10/24/2014 09:42 AM, Peter Zijlstra wrote:
> >> On Fri, Oct 24, 2014 at 09:23:35AM -0400, Sasha Levin wrote:
> >>>
> >>> i >> 32 may happen to be "i", but is there anything that prevents the compiler
> >>> from returning, let's say, 42?
> >>
> >> Not really, although gcc seems to opt for the 'sane' option and emit the
> >> instruction and let the arch figure out how to deal with it. Hence the
> >> 'fun' difference between x86 and ARM.
> >
> > It's interesting how many different views on undefined behaviour there are between
> > kernel folks.
> >
> > Everything between Ted Ts'o saying that GCC can launch nethack on oversized shifts,
> > to DaveM saying he will file a GCC bug if the behaviour isn't sane w.r.t to memcpy().
>
> One of the benefits of fixing such issues (or not letting them into
> code in the first place) is just saving numerous hours of top-notch
> engineers spent on disputes like this.
Also it means when someone quietly changes the default behaviour next
year in the compiler they won't spend months trying to work out why it
broke.
gcc has one behaviour but people also try and build the kernel with icc
and with llvm. In addition in some cases you risk the compiler simply
generating an undefined in hardware operation and the hardware behaviour
changing. If x >> 32 is undefined then generating "load Y with the
shift, shift X left by Y" is fine. What happens in future silicon - who
knows.
Most of the kernel is already very careful about the >> 32 problem.
Alan
On Oct 24, 2014, at 9:10 AM, Dmitry Vyukov <[email protected]> wrote:
> On Fri, Oct 24, 2014 at 7:04 PM, Sasha Levin <[email protected]> wrote:
>> On 10/24/2014 09:42 AM, Peter Zijlstra wrote:
>>> On Fri, Oct 24, 2014 at 09:23:35AM -0400, Sasha Levin wrote:
>>>>
>>>> i >> 32 may happen to be "i", but is there anything that prevents the compiler from returning, let's say, 42?
>>>
>>> Not really, although gcc seems to opt for the 'sane' option and emit
>>> the instruction and let the arch figure out how to deal with it.
>>> Hence the 'fun' difference between x86 and ARM.
>>
>> It's interesting how many different views on undefined behaviour there are between kernel folks.
>>
>> Everything between Ted Ts'o saying that GCC can launch nethack on oversized shifts, to DaveM saying he will file a GCC bug if the
>> behaviour isn't sane w.r.t to memcpy().
>
> One of the benefits of fixing such issues (or not letting them into
> code in the first place) is just saving numerous hours of top-notch
> engineers spent on disputes like this.
By the principle of least surprise, I would expect "__u32 >> N", where
N >= 32 to return zero instead of random garbage. For N < 32 it will
return progressively smaller numbers, until it has shifted away all of
the set bits, at which turn it will return 0. For it suddenly to jump
up once N = 32 is used, is counter-intuitive.
Cheers, Andreas
On 10/24/2014 02:05 PM, One Thousand Gnomes wrote:
> On Fri, 24 Oct 2014 19:10:49 +0400
> Dmitry Vyukov <[email protected]> wrote:
>
>> On Fri, Oct 24, 2014 at 7:04 PM, Sasha Levin <[email protected]> wrote:
>>> On 10/24/2014 09:42 AM, Peter Zijlstra wrote:
>>>> On Fri, Oct 24, 2014 at 09:23:35AM -0400, Sasha Levin wrote:
>>>>>
>>>>> i >> 32 may happen to be "i", but is there anything that prevents the compiler
>>>>> from returning, let's say, 42?
>>>>
>>>> Not really, although gcc seems to opt for the 'sane' option and emit the
>>>> instruction and let the arch figure out how to deal with it. Hence the
>>>> 'fun' difference between x86 and ARM.
>>>
>>> It's interesting how many different views on undefined behaviour there are between
>>> kernel folks.
>>>
>>> Everything between Ted Ts'o saying that GCC can launch nethack on oversized shifts,
>>> to DaveM saying he will file a GCC bug if the behaviour isn't sane w.r.t to memcpy().
>>
>> One of the benefits of fixing such issues (or not letting them into
>> code in the first place) is just saving numerous hours of top-notch
>> engineers spent on disputes like this.
>
> Also it means when someone quietly changes the default behaviour next
> year in the compiler they won't spend months trying to work out why it
> broke.
>
> gcc has one behaviour but people also try and build the kernel with icc
> and with llvm. In addition in some cases you risk the compiler simply
> generating an undefined in hardware operation and the hardware behaviour
> changing. If x >> 32 is undefined then generating "load Y with the
> shift, shift X left by Y" is fine. What happens in future silicon - who
> knows.
>
> Most of the kernel is already very careful about the >> 32 problem.
>
The real question is if we can rely on the gcc-ism:
(x >> (S-y)) | (x << y)
... where S is the number of bits to indicate a rotate.
This is technically a gcc extension to the C language.
-hpa
On 10/24/2014 03:09 PM, Andreas Dilger wrote:
> On Oct 24, 2014, at 9:10 AM, Dmitry Vyukov <[email protected]> wrote:
>> On Fri, Oct 24, 2014 at 7:04 PM, Sasha Levin <[email protected]> wrote:
>>> On 10/24/2014 09:42 AM, Peter Zijlstra wrote:
>>>> On Fri, Oct 24, 2014 at 09:23:35AM -0400, Sasha Levin wrote:
>>>>>
>>>>> i >> 32 may happen to be "i", but is there anything that prevents the compiler from returning, let's say, 42?
>>>>
>>>> Not really, although gcc seems to opt for the 'sane' option and emit
>>>> the instruction and let the arch figure out how to deal with it.
>>>> Hence the 'fun' difference between x86 and ARM.
>>>
>>> It's interesting how many different views on undefined behaviour there are between kernel folks.
>>>
>>> Everything between Ted Ts'o saying that GCC can launch nethack on oversized shifts, to DaveM saying he will file a GCC bug if the
>>> behaviour isn't sane w.r.t to memcpy().
>>
>> One of the benefits of fixing such issues (or not letting them into
>> code in the first place) is just saving numerous hours of top-notch
>> engineers spent on disputes like this.
>
> By the principle of least surprise, I would expect "__u32 >> N", where
> N >= 32 to return zero instead of random garbage. For N < 32 it will
> return progressively smaller numbers, until it has shifted away all of
> the set bits, at which turn it will return 0. For it suddenly to jump
> up once N = 32 is used, is counter-intuitive.
>
That's why it is undefined.
-hpa
On 10/24/2014 06:22 PM, H. Peter Anvin wrote:
>> By the principle of least surprise, I would expect "__u32 >> N", where
>> > N >= 32 to return zero instead of random garbage. For N < 32 it will
>> > return progressively smaller numbers, until it has shifted away all of
>> > the set bits, at which turn it will return 0. For it suddenly to jump
>> > up once N = 32 is used, is counter-intuitive.
>> >
> That's why it is undefined.
Now I'm curious about things like "memcpy(ptr, NULL, 0)". According to the
standard they're undefined, and since we're using gcc's implementation for
memcpy() we are doing "undefined memcpy" in quite a few places in the kernel.
Is it an issue, or would you expect memcpy() to not deref the "from" ptr
since length is 0?
Thanks,
Sasha
On Fri, 24 Oct 2014 20:50:46 -0400
Sasha Levin <[email protected]> wrote:
> On 10/24/2014 06:22 PM, H. Peter Anvin wrote:
> >> By the principle of least surprise, I would expect "__u32 >> N", where
> >> > N >= 32 to return zero instead of random garbage. For N < 32 it will
> >> > return progressively smaller numbers, until it has shifted away all of
> >> > the set bits, at which turn it will return 0. For it suddenly to jump
> >> > up once N = 32 is used, is counter-intuitive.
> >> >
> > That's why it is undefined.
>
> Now I'm curious about things like "memcpy(ptr, NULL, 0)". According to the
> standard they're undefined, and since we're using gcc's implementation for
> memcpy() we are doing "undefined memcpy" in quite a few places in the kernel.
>
> Is it an issue, or would you expect memcpy() to not deref the "from" ptr
> since length is 0?
No. Furthermore gcc 4.9 actually has optimiser magic around this. See the
"Porting to gcc 4.9" notes.
--------
GCC might now optimize away the null pointer check in code like:
int copy (int* dest, int* src, size_t nbytes) {
memmove (dest, src, nbytes);
if (src != NULL)
return *src;
return 0;
}
The pointers passed to memmove (and similar functions in <string.h>) must
be non-null even when nbytes==0, so GCC can use that information to
remove the check after the memmove call. Calling copy(p, NULL, 0) can
therefore deference a null pointer and crash.
-------------
Which is unfortunate because an operating system has a lot of legitimate
reasons to copy data to address 0 (on many processors its the exception
vectors for example)
Alan
2014-10-25 23:30 GMT+03:00 One Thousand Gnomes <[email protected]>:
> On Fri, 24 Oct 2014 20:50:46 -0400
> Sasha Levin <[email protected]> wrote:
>
>> On 10/24/2014 06:22 PM, H. Peter Anvin wrote:
>> >> By the principle of least surprise, I would expect "__u32 >> N", where
>> >> > N >= 32 to return zero instead of random garbage. For N < 32 it will
>> >> > return progressively smaller numbers, until it has shifted away all of
>> >> > the set bits, at which turn it will return 0. For it suddenly to jump
>> >> > up once N = 32 is used, is counter-intuitive.
>> >> >
>> > That's why it is undefined.
>>
>> Now I'm curious about things like "memcpy(ptr, NULL, 0)". According to the
>> standard they're undefined, and since we're using gcc's implementation for
>> memcpy() we are doing "undefined memcpy" in quite a few places in the kernel.
>>
>> Is it an issue, or would you expect memcpy() to not deref the "from" ptr
>> since length is 0?
>
> No. Furthermore gcc 4.9 actually has optimiser magic around this. See the
> "Porting to gcc 4.9" notes.
>
> --------
>
> GCC might now optimize away the null pointer check in code like:
>
>
> int copy (int* dest, int* src, size_t nbytes) {
> memmove (dest, src, nbytes);
> if (src != NULL)
> return *src;
> return 0;
> }
>
> The pointers passed to memmove (and similar functions in <string.h>) must
> be non-null even when nbytes==0, so GCC can use that information to
> remove the check after the memmove call. Calling copy(p, NULL, 0) can
> therefore deference a null pointer and crash.
>
> -------------
>
> Which is unfortunate because an operating system has a lot of legitimate
> reasons to copy data to address 0 (on many processors its the exception
> vectors for example)
That is why kernel builds with -fno-delete-null-pointer-checks.
>
> Alan
--
Best regards,
Andrey Ryabinin