[U-Boot] [PATCH v2 00/13] Import Undefined Behavior Sanitizer

[U-Boot] [PATCH v2 00/13] Import Undefined Behavior Sanitizer

[Eugeniu Rosca]

While certain classes of bugs (e.g. locking related) are totally
irrelevant for U-Boot, undefined behavior is something U-Boot may
experience all over the place and this certainly can lead to hidden
and difficult to debug issues.

As of v4.18, Linux kernel contains roughly 119 UBSAN fixes [1], so it
has been quite a productive and useful tool to play with during
development.

Thanks to UBSAN, this series fixes 11 (+1 in [2]) UB issues, revealed
by simply cold-booting (sometimes by running an existing command on)
sandbox, qemu-system-{arm,ppc,i386} and arm64 R-Car Gen3 targets.

  | Undefined Behavior class              | Noticed | Fixed | Ref
--|---------------------------------------|---------|-------|-----
A | Signed shift overflow                 | 8       | 8     |
B | Zero-sized array declaration          | 2       | 2     |
C | Read-past-end-of-array                | 1       | 1     |
D | Shift by negative value               | 1       | 0     | [3]
E | Load of address 'X' with insufficient |         |       |
  |      space for an object of type 'Y'  | ~20-30  | 0     | [4]

A certain class of UBs (see E above) is reported regularly at runtime
on all architectures and looks to be related to the implementation of
U-Boot linker-generated arrays [4]. I believe some feedback from the
authors/maintainers of those is required to assess if this is a UBSAN
false positive or a real bug.

The "signed shift overflow" (see A above) UB is very common in U-Boot.
Grepping the code for '(1 << 31)' (which is a consistent source of this
type of UB) gives 528 occurrences:
$ git grep -E '1[ ]*<<[ ]*31' | wc -l
  528

This series collects the low-hanging fruit, as well as leaves others to
experiment with UBSAN themselves.

Best regards,
Eugeniu.

[1] git log --oneline --no-merges --grep UBSAN v4.18 | wc -l
    119
[2] https://patchwork.ozlabs.org/patch/957323/

[3] Example of "shift by negative value" UB
 ==================================================================
 UBSAN: Undefined behaviour in drivers/pci/fsl_pci_init.c:139:17
 shift exponent -1 is negative
 ==================================================================

[4] Either a false-positive or a bug in "include/linker_lists.h":
 =================================================================
 UBSAN: Undefined behaviour in drivers/core/lists.c:28:26
 load of address 000000000075f180 with insufficient space
 for an object of type 'char *'
 =================================================================

Eugeniu Rosca (13):
  UBSAN: run-time undefined behavior sanity checker
  mmc: Fix signed shift overflow
  armv8: mmu: Fix signed shift overflow
  pinctrl: renesas: Fix signed shift overflow
  net: phy: Fix signed shift overflow
  net: ravb: Fix signed shift overflow
  x86: Fix signed shift overflow in MSR_IA32_APICBASE_BASE
  disk: part_dos: Fix signed shift overflow
  common.h: Fix signed shift overflow in cpumask_next()
  mmc: Fix read-past-end-of-array
  hashtable: Fix zero-sized array
  input: Fix zero-sized array
  configs: sandbox*: Enable UBSAN

 Makefile                           |   3 +-
 arch/Kconfig                       |   4 +
 arch/arm/Kconfig                   |   1 +
 arch/arm/include/asm/armv8/mmu.h   |  42 +--
 arch/x86/include/asm/msr-index.h   |   2 +-
 configs/sandbox64_defconfig        |   1 +
 configs/sandbox_defconfig          |   1 +
 configs/sandbox_flattree_defconfig |   1 +
 configs/sandbox_noblk_defconfig    |   1 +
 configs/sandbox_spl_defconfig      |   1 +
 disk/part_dos.c                    |   9 +-
 drivers/input/input.c              |   4 +-
 drivers/mmc/mmc.c                  |   4 +-
 drivers/net/phy/phy.c              |   4 +-
 drivers/net/ravb.c                 |  16 +-
 drivers/pinctrl/renesas/sh_pfc.h   |  14 +-
 examples/standalone/Makefile       |   2 +
 include/common.h                   |   2 +-
 include/linux/compat.h             |   3 +
 include/search.h                   |   2 +-
 lib/Kconfig                        |   1 +
 lib/Kconfig.ubsan                  |  29 ++
 lib/Makefile                       |   2 +
 lib/hashtable.c                    |   4 +-
 lib/linux_compat.c                 |   3 +
 lib/ubsan.c                        | 461 +++++++++++++++++++++++++++++
 lib/ubsan.h                        |  94 ++++++
 scripts/Makefile.lib               |   6 +
 scripts/Makefile.ubsan             |  20 ++
 29 files changed, 684 insertions(+), 53 deletions(-)
 create mode 100644 lib/Kconfig.ubsan
 create mode 100644 lib/ubsan.c
 create mode 100644 lib/ubsan.h
 create mode 100644 scripts/Makefile.ubsan

-- 
2.18.0

[U-Boot] [PATCH v2 01/13] UBSAN: run-time undefined behavior sanity checker

[Eugeniu Rosca]

Import Undefined Behavior SANitizer from Linux Kernel v4.18, as
implemented by Andrey Ryabinin <aryabinin@virtuozzo.com>.

Roughly, the UBSAN development history in Linux kernel looks like:

v4.18     3ca17b1f3628 ("lib/ubsan: remove null-pointer checks")
v4.17-rc1 317506009216 ("lib/test_ubsan.c: make test_ubsan_misaligned_access() static")
v4.17-rc1 854686f4edf4 ("lib: add testing module for UBSAN")
v4.16-rc1 bac7a1fff792 ("lib/ubsan: remove returns-nonnull-attribute checks")
v4.16-rc1 42440c1f9911 ("lib/ubsan: add type mismatch handler for new GCC/Clang")
v4.16-rc1 b8fe1120b4ba ("lib/ubsan.c: s/missaligned/misaligned/")
v4.10-rc1 0462554707d6 ("Kconfig: lib/Kconfig.ubsan fix reference to ubsan documentation")
 v4.9-rc5 a76bcf557ef4 ("Kbuild: enable -Wmaybe-uninitialized warning for "make W=1"")
 v4.9-rc1 725c4d22bbc4 ("ubsan: allow to disable the null sanitizer")
 v4.9-rc1 1ead009cd622 ("docs: sphinxify ubsan.txt and move it to dev-tools")
 v4.8-rc1 901d805c33fc ("UBSAN: fix typo in format string")
 v4.8-rc1 6e8d666e9253 ("Disable "maybe-uninitialized" warning globally")
 v4.6-rc1 dde5cf39d4d2 ("ubsan: fix tree-wide -Wmaybe-uninitialized false positives")
 v4.5-rc4 7707535ab95e ("ubsan: cosmetic fix to Kconfig text")
 v4.5-rc1 bf76f73c5f65 ("powerpc: enable UBSAN support")
 v4.5-rc1 c6d308534aef ("UBSAN: run-time undefined behavior sanity checker")

What's not interesting for U-Boot is:
 - 317506009216 ("lib/test_ubsan.c: make test_ubsan_misaligned_access() static")
 - 854686f4edf4 ("lib: add testing module for UBSAN")
   since they add a module-only test functionality.
 - any Documentation commits.

Since dump_stack() evaluates to NOOP in U-Boot, the UBSAN report
retains only the header from the original kernel report.

As example, below is a UB found in U-Boot thanks to UBSAN:

 ====================================================================
 UBSAN: Undefined behaviour in drivers/net/phy/phy.c:728:19
 left shift of 1 by 31 places cannot be represented in type 'int'
 ====================================================================

For comparison, below is a full-fledged kernel UBSAN report, based on
v4.17-rc4 Linux commit 0dfc0c792d69 ("iommu/vt-d: fix shift-out-of-
bounds in bug checking"):

 ================================================================================
UBSAN: Undefined behaviour in drivers/iommu/dmar.c:1348:3
shift exponent 64 is too large for 32-bit type 'int'
CPU: 2 PID: 0 Comm: swapper/2 Tainted: G     U            4.17.0-rc1+ #89
Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.2.8 01/26/2016
Call Trace:
 <IRQ>
 dump_stack+0x90/0xfb
 ubsan_epilogue+0x9/0x40
 __ubsan_handle_shift_out_of_bounds+0x10e/0x170
 ? qi_flush_dev_iotlb+0x124/0x180
-----[snip]-----
 apic_timer_interrupt+0xf/0x20
 </IRQ>
RIP: 0010:poll_idle+0x60/0xe7
RSP: 0018:ffffb1b201943e30 EFLAGS: 00000246 ORIG_RAX: ffffffffffffff13
RAX: 0000000080200000 RBX: 000000000000008e RCX: 000000000000001f
RDX: 0000000000000000 RSI: 000000002819aa06 RDI: 0000000000000000
RBP: ffff9e93c6b33280 R08: 00000010f717d567 R09: 000000000010d205
R10: ffffb1b201943df8 R11: 0000000000000001 R12: 00000000e01b169d
R13: 0000000000000000 R14: ffffffffb12aa400 R15: 0000000000000000
 cpuidle_enter_state+0xb4/0x470
 do_idle+0x222/0x310
 cpu_startup_entry+0x78/0x90
 start_secondary+0x205/0x2e0
 secondary_startup_64+0xa5/0xb0
 ================================================================================

To enable UBSAN, two prerequisites must be met from Kconfig perspective:
* ARCH has to select CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL
* defconfig has to enable CONFIG_UBSAN

Enable CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL on below architectures
(specify compiler, defconfig and phys/virt target used for testing):
* SANDBOX:
  * gcc (Ubuntu 8.1.0-5ubuntu1~16.04)
  * sandbox*_defconfig
  * x86_64 Ubuntu machine
* x86:
  * gcc (Ubuntu 8.1.0-5ubuntu1~16.04)
  * qemu-x86_defconfig
  * qemu-system-i386
* ARM:
  * arm-linux-gnueabihf-gcc (Linaro GCC 7.2-2017.11)
  * vexpress_ca9x4_defconfig
  * qemu-system-arm
* ARM64:
  * aarch64-linux-gnu-gcc (Linaro GCC 7.2-2017.11)
  * r8a7795_salvator-x_defconfig
  * R-Car H3 ES2.0 Salvator-X board
* PPC:
  * powerpc-linux-gnu-gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9)
  * qemu-ppce500_defconfig
  * qemu-system-ppc

Any defconfig update (UBSAN=y) is expected to come separately.
No functional change is intended, assuming CONFIG_UBSAN=n.

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---

Changes in v2:
 - [Tom Rini] Relocate the ELF/BIN size measurements to the commit
   updating the sandbox defconfig, to avoid confusion.
 - [York Sun] Fix SPL build with UBSAN=y.
 - Enable CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL on x86, ARM and PPC, in
   addition to SANDBOX and ARM64 (all build and runtime tested with
   at least one defconfig as described in the commit description).
---
 Makefile                     |   3 +-
 arch/Kconfig                 |   4 ++
 arch/arm/Kconfig             |   1 +
 examples/standalone/Makefile |   2 +
 include/linux/compat.h       |   3 +
 lib/Kconfig                  |   1 +
 lib/Kconfig.ubsan            |  29 +++++++++
 lib/Makefile                 |   2 +
 lib/linux_compat.c           |   3 +
 lib/ubsan.c                  | 461 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/ubsan.h                  |  94 +++++++++++++++++++++++++++++
 scripts/Makefile.lib         |   6 ++
 scripts/Makefile.ubsan       |  20 +++++++
 13 files changed, 628 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index b5bf8abc1f88..d1eeaa9dbcd2 100644
--- a/Makefile
+++ b/Makefile
@@ -391,7 +391,7 @@ export MAKE LEX YACC AWK PERL PYTHON PYTHON2 PYTHON3
 export HOSTCXX HOSTCXXFLAGS CHECK CHECKFLAGS DTC DTC_FLAGS
 
 export KBUILD_CPPFLAGS NOSTDINC_FLAGS UBOOTINCLUDE OBJCOPYFLAGS LDFLAGS
-export KBUILD_CFLAGS KBUILD_AFLAGS
+export KBUILD_CFLAGS KBUILD_AFLAGS CFLAGS_UBSAN
 
 # When compiling out-of-tree modules, put MODVERDIR in the module
 # tree rather than in the kernel tree. The kernel tree might
@@ -648,6 +648,7 @@ endif
 KBUILD_CFLAGS   += $(call cc-option,-Werror=date-time)
 
 include scripts/Makefile.extrawarn
+include scripts/Makefile.ubsan
 
 # Add user supplied CPPFLAGS, AFLAGS and CFLAGS as the last assignments
 KBUILD_CPPFLAGS += $(KCPPFLAGS)
diff --git a/arch/Kconfig b/arch/Kconfig
index bf1b4a9afac6..ebb8ee5979e0 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -19,6 +19,7 @@ config ARC
 
 config ARM
 	bool "ARM architecture"
+	select ARCH_HAS_UBSAN_SANITIZE_ALL
 	select CREATE_ARCH_SYMLINK
 	select HAVE_PRIVATE_LIBGCC if !ARM64
 	select SUPPORT_OF_CONTROL
@@ -54,6 +55,7 @@ config NIOS2
 
 config PPC
 	bool "PowerPC architecture"
+	select ARCH_HAS_UBSAN_SANITIZE_ALL
 	select HAVE_PRIVATE_LIBGCC
 	select SUPPORT_OF_CONTROL
 	select SYS_BOOT_GET_CMDLINE
@@ -65,6 +67,7 @@ config RISCV
 
 config SANDBOX
 	bool "Sandbox"
+	select ARCH_HAS_UBSAN_SANITIZE_ALL
 	select BOARD_LATE_INIT
 	select DM
 	select DM_GPIO
@@ -98,6 +101,7 @@ config SH
 
 config X86
 	bool "x86 architecture"
+	select ARCH_HAS_UBSAN_SANITIZE_ALL
 	select CREATE_ARCH_SYMLINK
 	select DM
 	select DM_PCI
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 9f5eaf8591b6..0905ebce7004 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -6,6 +6,7 @@ config SYS_ARCH
 
 config ARM64
 	bool
+	select ARCH_HAS_UBSAN_SANITIZE_ALL
 	select PHYS_64BIT
 	select SYS_CACHE_SHIFT_6
 
diff --git a/examples/standalone/Makefile b/examples/standalone/Makefile
index 09364d84a0ad..66716d37465a 100644
--- a/examples/standalone/Makefile
+++ b/examples/standalone/Makefile
@@ -3,6 +3,8 @@
 # (C) Copyright 2000-2006
 # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
 
+UBSAN_SANITIZE := n
+
 extra-y        := hello_world
 extra-$(CONFIG_SMC91111)           += smc91111_eeprom
 extra-$(CONFIG_SMC911X)            += smc911x_eeprom
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 6e3feb64d2d5..e851b2982d76 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -11,6 +11,9 @@ typedef struct unused unused_t;
 
 struct p_current{
        int pid;
+#ifdef CONFIG_UBSAN
+	unsigned int in_ubsan;
+#endif
 };
 
 extern struct p_current *current;
diff --git a/lib/Kconfig b/lib/Kconfig
index 622f3c26c331..251903af9e6b 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -375,6 +375,7 @@ endmenu
 
 source lib/efi/Kconfig
 source lib/efi_loader/Kconfig
+source lib/Kconfig.ubsan
 source lib/optee/Kconfig
 
 endmenu
diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
new file mode 100644
index 000000000000..dbbfe974221d
--- /dev/null
+++ b/lib/Kconfig.ubsan
@@ -0,0 +1,29 @@
+config ARCH_HAS_UBSAN_SANITIZE_ALL
+	bool
+
+config UBSAN
+	bool "Undefined behaviour sanity checker"
+	help
+	  This option enables undefined behaviour sanity checker
+	  Compile-time instrumentation is used to detect various undefined
+	  behaviours in runtime.
+
+config UBSAN_SANITIZE_ALL
+	bool "Enable instrumentation for the entire kernel"
+	depends on UBSAN
+	depends on ARCH_HAS_UBSAN_SANITIZE_ALL
+	default y
+	help
+	  This option activates 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.
+	  Enabling this option will get kernel image size increased
+	  significantly.
+
+config UBSAN_ALIGNMENT
+	bool "Enable checking of pointers alignment"
+	depends on UBSAN
+	help
+	  This option enables detection of unaligned memory accesses.
+	  Enabling this option on architectures that support unaligned
+	  accesses may produce a lot of false positives.
diff --git a/lib/Makefile b/lib/Makefile
index 5f583aed37d9..0ae2c121cacf 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -51,6 +51,8 @@ endif
 obj-$(CONFIG_RSA) += rsa/
 obj-$(CONFIG_SHA1) += sha1.o
 obj-$(CONFIG_SHA256) += sha256.o
+obj-$(CONFIG_UBSAN) += ubsan.o
+UBSAN_SANITIZE_ubsan.o := n
 
 obj-$(CONFIG_$(SPL_)ZLIB) += zlib/
 obj-$(CONFIG_$(SPL_)GZIP) += gunzip.o
diff --git a/lib/linux_compat.c b/lib/linux_compat.c
index a936a7eac214..4dd89b50e924 100644
--- a/lib/linux_compat.c
+++ b/lib/linux_compat.c
@@ -4,6 +4,9 @@
 
 struct p_current cur = {
 	.pid = 1,
+#ifdef CONFIG_UBSAN
+	.in_ubsan = 0,
+#endif
 };
 __maybe_unused struct p_current *current = &cur;
 
diff --git a/lib/ubsan.c b/lib/ubsan.c
new file mode 100644
index 000000000000..642ef9f02c00
--- /dev/null
+++ b/lib/ubsan.c
@@ -0,0 +1,461 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * UBSAN error reporting functions
+ *
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ * Author: Andrey Ryabinin <ryabinin.a.a@gmail.com>
+ */
+
+#include <linux/bitops.h>
+#include <linux/bug.h>
+#include <linux/ctype.h>
+#include <linux/kernel.h>
+#include <linux/types.h>
+
+#ifdef __UBOOT__
+#include <linux/compat.h>
+#include <common.h>
+#endif
+
+#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));
+		}
+	}
+}
+
+#ifndef __UBOOT__
+static DEFINE_SPINLOCK(report_lock);
+#endif
+
+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_common *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_misaligned_access(struct type_mismatch_data_common *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_common *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);
+}
+
+static void ubsan_type_mismatch_common(struct type_mismatch_data_common *data,
+				unsigned long ptr)
+{
+
+	if (!ptr)
+		handle_null_ptr_deref(data);
+	else if (data->alignment && !IS_ALIGNED(ptr, data->alignment))
+		handle_misaligned_access(data, ptr);
+	else
+		handle_object_size_mismatch(data, ptr);
+}
+
+void __ubsan_handle_type_mismatch(struct type_mismatch_data *data,
+				unsigned long ptr)
+{
+	struct type_mismatch_data_common common_data = {
+		.location = &data->location,
+		.type = data->type,
+		.alignment = data->alignment,
+		.type_check_kind = data->type_check_kind
+	};
+
+	ubsan_type_mismatch_common(&common_data, ptr);
+}
+EXPORT_SYMBOL(__ubsan_handle_type_mismatch);
+
+void __ubsan_handle_type_mismatch_v1(struct type_mismatch_data_v1 *data,
+				unsigned long ptr)
+{
+
+	struct type_mismatch_data_common common_data = {
+		.location = &data->location,
+		.type = data->type,
+		.alignment = 1UL << data->log_alignment,
+		.type_check_kind = data->type_check_kind
+	};
+
+	ubsan_type_mismatch_common(&common_data, ptr);
+}
+EXPORT_SYMBOL(__ubsan_handle_type_mismatch_v1);
+
+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/lib/ubsan.h b/lib/ubsan.h
new file mode 100644
index 000000000000..01efea6bae11
--- /dev/null
+++ b/lib/ubsan.h
@@ -0,0 +1,94 @@
+// SPDX-License-Identifier: GPL-2.0
+#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 type_mismatch_data_v1 {
+	struct source_location location;
+	struct type_descriptor *type;
+	unsigned char log_alignment;
+	unsigned char type_check_kind;
+};
+
+struct type_mismatch_data_common {
+	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 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
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index f8c3fff1d151..ccf683265fcb 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -131,6 +131,12 @@ _c_flags += $(if $(patsubst n%,, \
 		$(CFLAGS_KASAN))
 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 '/').
 
diff --git a/scripts/Makefile.ubsan b/scripts/Makefile.ubsan
new file mode 100644
index 000000000000..38b2b4818e8e
--- /dev/null
+++ b/scripts/Makefile.ubsan
@@ -0,0 +1,20 @@
+# SPDX-License-Identifier: GPL-2.0
+ifdef CONFIG_UBSAN
+      CFLAGS_UBSAN += $(call cc-option, -fsanitize=shift)
+      CFLAGS_UBSAN += $(call cc-option, -fsanitize=integer-divide-by-zero)
+      CFLAGS_UBSAN += $(call cc-option, -fsanitize=unreachable)
+      CFLAGS_UBSAN += $(call cc-option, -fsanitize=vla-bound)
+      CFLAGS_UBSAN += $(call cc-option, -fsanitize=signed-integer-overflow)
+      CFLAGS_UBSAN += $(call cc-option, -fsanitize=bounds)
+      CFLAGS_UBSAN += $(call cc-option, -fsanitize=object-size)
+      CFLAGS_UBSAN += $(call cc-option, -fsanitize=bool)
+      CFLAGS_UBSAN += $(call cc-option, -fsanitize=enum)
+
+ifdef CONFIG_UBSAN_ALIGNMENT
+      CFLAGS_UBSAN += $(call cc-option, -fsanitize=alignment)
+endif
+
+      # -fsanitize=* options makes GCC less smart than usual and
+      # increase number of 'maybe-uninitialized false-positives
+      CFLAGS_UBSAN += $(call cc-option, -Wno-maybe-uninitialized)
+endif
-- 
2.18.0

[U-Boot] [PATCH v2 02/13] mmc: Fix signed shift overflow

[Eugeniu Rosca]

Below is reproduced both with sandbox and R-Car Gen3 arm64 U-Boot:

 ===================================================================
 UBSAN: Undefined behaviour in drivers/mmc/mmc.c:1147:21
 left shift of 1 by 31 places cannot be represented in type 'int'
 ===================================================================

Fixes: 272cc70b211e ("Add MMC Framework")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---

Changes in v2:
 - Shorten the summary line
---
 drivers/mmc/mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index ad429f49c992..447519f46f15 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -1136,7 +1136,7 @@ int mmc_getcd(struct mmc *mmc)
 #endif
 
 #if !CONFIG_IS_ENABLED(MMC_TINY)
-static int sd_switch(struct mmc *mmc, int mode, int group, u8 value, u8 *resp)
+static int sd_switch(struct mmc *mmc, uint mode, int group, u8 value, u8 *resp)
 {
 	struct mmc_cmd cmd;
 	struct mmc_data data;
-- 
2.18.0

[U-Boot] [PATCH v2 03/13] armv8: mmu: Fix signed shift overflow

[Eugeniu Rosca]

Fix the following UBSAN warnings:

------8<-----
CPU: Renesas Electronics R8A7795 rev 2.0
Model: Renesas Salvator-X board based on r8a7795 ES2.0+
 ====================================================================
 UBSAN: Undefined behaviour in arch/arm/cpu/armv8/cache_v8.c:72:9
 left shift of 1 by 31 places cannot be represented in type 'int'
 ====================================================================
 ====================================================================
 UBSAN: Undefined behaviour in arch/arm/cpu/armv8/cache_v8.c:74:9
 left shift of 1 by 31 places cannot be represented in type 'int'
 ====================================================================
------8<-----

Use (1UL << i) instead of BIT() macro for consistency.

Fixes: ad3d6e88a1a4 ("armv8/mmu: Set bits marked RES1 in TCR")
Fixes: 9bb367a590fe ("arm64: Disable TTBR1 maps in EL1")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---

Changes in v2:
 - Shorten the summary line
 - Use (1UL << i) instead of BIT() macro for consistency
---
 arch/arm/include/asm/armv8/mmu.h | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/arch/arm/include/asm/armv8/mmu.h b/arch/arm/include/asm/armv8/mmu.h
index 62d00d15c26d..632d3a442df8 100644
--- a/arch/arm/include/asm/armv8/mmu.h
+++ b/arch/arm/include/asm/armv8/mmu.h
@@ -78,27 +78,27 @@
  * TCR flags.
  */
 #define TCR_T0SZ(x)		((64 - (x)) << 0)
-#define TCR_IRGN_NC		(0 << 8)
-#define TCR_IRGN_WBWA		(1 << 8)
-#define TCR_IRGN_WT		(2 << 8)
-#define TCR_IRGN_WBNWA		(3 << 8)
-#define TCR_IRGN_MASK		(3 << 8)
-#define TCR_ORGN_NC		(0 << 10)
-#define TCR_ORGN_WBWA		(1 << 10)
-#define TCR_ORGN_WT		(2 << 10)
-#define TCR_ORGN_WBNWA		(3 << 10)
-#define TCR_ORGN_MASK		(3 << 10)
-#define TCR_SHARED_NON		(0 << 12)
-#define TCR_SHARED_OUTER	(2 << 12)
-#define TCR_SHARED_INNER	(3 << 12)
-#define TCR_TG0_4K		(0 << 14)
-#define TCR_TG0_64K		(1 << 14)
-#define TCR_TG0_16K		(2 << 14)
-#define TCR_EPD1_DISABLE	(1 << 23)
-
-#define TCR_EL1_RSVD		(1 << 31)
-#define TCR_EL2_RSVD		(1 << 31 | 1 << 23)
-#define TCR_EL3_RSVD		(1 << 31 | 1 << 23)
+#define TCR_IRGN_NC		(0UL << 8)
+#define TCR_IRGN_WBWA		(1UL << 8)
+#define TCR_IRGN_WT		(2UL << 8)
+#define TCR_IRGN_WBNWA		(3UL << 8)
+#define TCR_IRGN_MASK		(3UL << 8)
+#define TCR_ORGN_NC		(0UL << 10)
+#define TCR_ORGN_WBWA		(1UL << 10)
+#define TCR_ORGN_WT		(2UL << 10)
+#define TCR_ORGN_WBNWA		(3UL << 10)
+#define TCR_ORGN_MASK		(3UL << 10)
+#define TCR_SHARED_NON		(0UL << 12)
+#define TCR_SHARED_OUTER	(2UL << 12)
+#define TCR_SHARED_INNER	(3UL << 12)
+#define TCR_TG0_4K		(0UL << 14)
+#define TCR_TG0_64K		(1UL << 14)
+#define TCR_TG0_16K		(2UL << 14)
+#define TCR_EPD1_DISABLE	(1UL << 23)
+
+#define TCR_EL1_RSVD		(1UL << 31)
+#define TCR_EL2_RSVD		(1UL << 31 | 1UL << 23)
+#define TCR_EL3_RSVD		(1UL << 31 | 1UL << 23)
 
 #ifndef __ASSEMBLY__
 static inline void set_ttbr_tcr_mair(int el, u64 table, u64 tcr, u64 attr)
-- 
2.18.0

[U-Boot] [PATCH v2 04/13] pinctrl: renesas: Fix signed shift overflow

[Eugeniu Rosca]

Booting R-Car H3-Salvator-X (CONFIG_UBSAN=y) consistently results in:

 =====================================================================
 UBSAN: Undefined behaviour in drivers/pinctrl/renesas/pfc.c:402:40
 left shift of 1 by 31 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/pinctrl/renesas/pfc.c:410:39
 left shift of 1 by 31 places cannot be represented in type 'int'
 =====================================================================

While fixing these warnings, convert *all* SH_PFC_PIN_CFG_* definitions
to use the recommended BIT() macro.

Fixes: 910df4d07e37 ("pinctrl: rmobile: Add Renesas RCar pincontrol driver")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Acked-by: Marek Vasut <marek.vasut@gmail.com>
---

Changes in v2:
 - Shorten the summary line
 - Add "Acked-by: Marek Vasut <marek.vasut@gmail.com>"
---
 drivers/pinctrl/renesas/sh_pfc.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/renesas/sh_pfc.h b/drivers/pinctrl/renesas/sh_pfc.h
index b98c2f185d26..b58e52bbfbb9 100644
--- a/drivers/pinctrl/renesas/sh_pfc.h
+++ b/drivers/pinctrl/renesas/sh_pfc.h
@@ -21,13 +21,13 @@ enum {
 	PINMUX_TYPE_INPUT,
 };
 
-#define SH_PFC_PIN_CFG_INPUT		(1 << 0)
-#define SH_PFC_PIN_CFG_OUTPUT		(1 << 1)
-#define SH_PFC_PIN_CFG_PULL_UP		(1 << 2)
-#define SH_PFC_PIN_CFG_PULL_DOWN	(1 << 3)
-#define SH_PFC_PIN_CFG_IO_VOLTAGE	(1 << 4)
-#define SH_PFC_PIN_CFG_DRIVE_STRENGTH	(1 << 5)
-#define SH_PFC_PIN_CFG_NO_GPIO		(1 << 31)
+#define SH_PFC_PIN_CFG_INPUT		BIT(0)
+#define SH_PFC_PIN_CFG_OUTPUT		BIT(1)
+#define SH_PFC_PIN_CFG_PULL_UP		BIT(2)
+#define SH_PFC_PIN_CFG_PULL_DOWN	BIT(3)
+#define SH_PFC_PIN_CFG_IO_VOLTAGE	BIT(4)
+#define SH_PFC_PIN_CFG_DRIVE_STRENGTH	BIT(5)
+#define SH_PFC_PIN_CFG_NO_GPIO		BIT(31)
 
 struct sh_pfc_pin {
 	u16 pin;
-- 
2.18.0

[U-Boot] [PATCH v2 05/13] net: phy: Fix signed shift overflow

[Eugeniu Rosca]

Booting R-Car Gen3 arm64 U-Boot with CONFIG_UBSAN=y results in:

 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/phy/phy.c:728:19
 left shift of 1 by 31 places cannot be represented in type 'int'
 =====================================================================

Fix it by appending the UL suffix to the numeric literal. While at it,
convert the type of "addr" variable from signed to unsigned, to protect
against shifting the numeric literal by a negative value (which would
lead to yet another undefined behavior).

Fixes: 1adb406b0141 ("phy: add phy_find_by_mask/phy_connect_dev")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---

Changes in v2:
 - Shorten the summary line. Rephrase/rewrap the description.
---
 drivers/net/phy/phy.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e837eb7688cc..0a8df72a495f 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -719,13 +719,13 @@ static struct phy_device *search_for_existing_phy(struct mii_dev *bus,
 {
 	/* If we have one, return the existing device, with new interface */
 	while (phy_mask) {
-		int addr = ffs(phy_mask) - 1;
+		unsigned int addr = ffs(phy_mask) - 1;
 
 		if (bus->phymap[addr]) {
 			bus->phymap[addr]->interface = interface;
 			return bus->phymap[addr];
 		}
-		phy_mask &= ~(1 << addr);
+		phy_mask &= ~(1UL << addr);
 	}
 	return NULL;
 }
-- 
2.18.0

[U-Boot] [PATCH v2 06/13] net: ravb: Fix signed shift overflow

[Eugeniu Rosca]

Running "tftp" on R-Car H3 Salvator-X with CONFIG_UBSAN=y results in:

=> tftp
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:237:28
 left shift of 10 by 28 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:258:44
 left shift of 9 by 28 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:263:46
 left shift of 9 by 28 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:283:31
 left shift of 9 by 28 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:288:49
 left shift of 9 by 28 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:293:46
 left shift of 9 by 28 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:345:2
 left shift of 222 by 24 places cannot be represented in type 'int'
 =====================================================================

Pinging the host results in:

=> ping 192.168.2.11
 Using ethernet at e6800000 device
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:161:21
 left shift of 15 by 28 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:182:25
 left shift of 15 by 28 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:182:47
 left shift of 12 by 28 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:205:20
 left shift of 12 by 28 places cannot be represented in type 'int'
 =====================================================================
 host 192.168.2.11 is alive

There are two issues behind:
 - calculating RAVB_DESC_DT_* bitfields
 - assembling MAC address from its char components

Fix both.

Fixes: 8ae51b6f324e ("net: ravb: Add Renesas Ethernet RAVB driver")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Acked-by: Marek Vasut <marek.vasut@gmail.com>
---

Changes in v2:
 - Shorten the summary line
 - Add "Acked-by: Marek Vasut <marek.vasut@gmail.com>"
---
 drivers/net/ravb.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ravb.c b/drivers/net/ravb.c
index 749562db960e..5baff198889b 100644
--- a/drivers/net/ravb.c
+++ b/drivers/net/ravb.c
@@ -73,12 +73,12 @@
 #define RAVB_RX_QUEUE_OFFSET		4
 
 #define RAVB_DESC_DT(n)			((n) << 28)
-#define RAVB_DESC_DT_FSINGLE		RAVB_DESC_DT(0x7)
-#define RAVB_DESC_DT_LINKFIX		RAVB_DESC_DT(0x9)
-#define RAVB_DESC_DT_EOS		RAVB_DESC_DT(0xa)
-#define RAVB_DESC_DT_FEMPTY		RAVB_DESC_DT(0xc)
-#define RAVB_DESC_DT_EEMPTY		RAVB_DESC_DT(0x3)
-#define RAVB_DESC_DT_MASK		RAVB_DESC_DT(0xf)
+#define RAVB_DESC_DT_FSINGLE		RAVB_DESC_DT(0x7UL)
+#define RAVB_DESC_DT_LINKFIX		RAVB_DESC_DT(0x9UL)
+#define RAVB_DESC_DT_EOS		RAVB_DESC_DT(0xaUL)
+#define RAVB_DESC_DT_FEMPTY		RAVB_DESC_DT(0xcUL)
+#define RAVB_DESC_DT_EEMPTY		RAVB_DESC_DT(0x3UL)
+#define RAVB_DESC_DT_MASK		RAVB_DESC_DT(0xfUL)
 
 #define RAVB_DESC_DS(n)			(((n) & 0xfff) << 0)
 #define RAVB_DESC_DS_MASK		0xfff
@@ -342,8 +342,8 @@ static int ravb_write_hwaddr(struct udevice *dev)
 	struct eth_pdata *pdata = dev_get_platdata(dev);
 	unsigned char *mac = pdata->enetaddr;
 
-	writel((mac[0] << 24) | (mac[1] << 16) | (mac[2] << 8) | mac[3],
-	       eth->iobase + RAVB_REG_MAHR);
+	writel(((u32)mac[0] << 24) | ((u32)mac[1] << 16) | ((u32)mac[2] << 8) |
+	       mac[3], eth->iobase + RAVB_REG_MAHR);
 
 	writel((mac[4] << 8) | mac[5], eth->iobase + RAVB_REG_MALR);
 
-- 
2.18.0

[U-Boot] [PATCH v2 07/13] x86: Fix signed shift overflow in MSR_IA32_APICBASE_BASE

[Eugeniu Rosca]

Fix the following UBSAN report:
 ======================================================================
 UBSAN: Undefined behaviour in arch/x86/cpu/lapic.c:73:14
 left shift of 1048575 by 12 places cannot be represented in type 'int'
 ======================================================================

Steps to reproduce the above:
* echo CONFIG_UBSAN=y >> configs/qemu-x86_defconfig
* make ARCH=x86 qemu-x86_defconfig all
* qemu-system-i386 --version
  QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.31)
* qemu-system-i386 --nographic -bios u-boot.rom

Fixes: 98568f0fa96b ("x86: Import MSR/MTRR code from Linux")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---

Changes in v2:
 - None. Newly pushed.
---
 arch/x86/include/asm/msr-index.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 9c1dbe61d596..d8b7b8013c74 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -370,7 +370,7 @@
 #define MSR_IA32_APICBASE		0x0000001b
 #define MSR_IA32_APICBASE_BSP		(1<<8)
 #define MSR_IA32_APICBASE_ENABLE	(1<<11)
-#define MSR_IA32_APICBASE_BASE		(0xfffff<<12)
+#define MSR_IA32_APICBASE_BASE		(0xfffffUL << 12)
 
 #define MSR_IA32_TSCDEADLINE		0x000006e0
 
-- 
2.18.0

[U-Boot] [PATCH v2 08/13] disk: part_dos: Fix signed shift overflow

[Eugeniu Rosca]

Fix the following UBSAN report:
 ====================================================================
 UBSAN: Undefined behaviour in disk/part_dos.c:30:22
 left shift of 209 by 24 places cannot be represented in type 'int'
 ====================================================================

Steps to reproduce the above:
* echo CONFIG_UBSAN=y >> configs/qemu-x86_defconfig
* make ARCH=x86 qemu-x86_defconfig all
* qemu-system-i386 --version
  QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.31)
* qemu-system-i386 --nographic -bios u-boot.rom
* bootefi selftest

Fixes: fe8c2806cdba ("Initial revision")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---

Changes in v2:
 - None. Newly pushed.
---
 disk/part_dos.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/disk/part_dos.c b/disk/part_dos.c
index 936cee0d36ce..e19695846a95 100644
--- a/disk/part_dos.c
+++ b/disk/part_dos.c
@@ -27,11 +27,10 @@
  */
 static inline unsigned int le32_to_int(unsigned char *le32)
 {
-    return ((le32[3] << 24) +
-	    (le32[2] << 16) +
-	    (le32[1] << 8) +
-	     le32[0]
-	   );
+	return (((unsigned int)le32[3] << 24) +
+		((unsigned int)le32[2] << 16) +
+		((unsigned int)le32[1] << 8) +
+		 (unsigned int)le32[0]);
 }
 
 static inline int is_extended(int part_type)
-- 
2.18.0

[U-Boot] [PATCH v2 09/13] common.h: Fix signed shift overflow in cpumask_next()

[Eugeniu Rosca]

Fix the following UBSAN report:
 =================================================================
 UBSAN: Undefined behaviour in include/common.h:322:19
 left shift of 1 by 31 places cannot be represented in type 'int'
 =================================================================

Steps to reproduce the above:
* echo CONFIG_UBSAN=y >> configs/qemu-ppce500_defconfig
* make ARCH=powerpc CROSS_COMPILE=/usr/bin/powerpc-linux-gnu- \
       qemu-ppce500_defconfig all
* qemu-system-ppc --version
  QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.31)
* qemu-system-ppc -machine ppce500 -nographic -no-reboot -kernel u-boot

It looks like cpumask_next() intentionally uses shift overflow in its
for loop condition to break the loop. Relying on UB is not safe. Convert
the numeric literal 1 to 1UL and limit its maximum shift index to 31.

Fixes: fbb9ecf7493f ("powerpc/mp: add support for discontiguous cores")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---

Changes in v2:
 - None. Newly pushed.
---
 include/common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/common.h b/include/common.h
index 940161f1758b..5b92666d0e79 100644
--- a/include/common.h
+++ b/include/common.h
@@ -319,7 +319,7 @@ void	trap_init     (ulong);
 /* $(CPU)/cpu.c */
 static inline int cpumask_next(int cpu, unsigned int mask)
 {
-	for (cpu++; !((1 << cpu) & mask); cpu++)
+	for (cpu++; (cpu < 31) && !((1UL << cpu) & mask); cpu++)
 		;
 
 	return cpu;
-- 
2.18.0

[U-Boot] [PATCH v2 10/13] mmc: Fix read-past-end-of-array

[Eugeniu Rosca]

Running "mmc dev 0" on R-Car H3 Salvator-X (CONFIG_UBSAN=y) occasionally
results in:

=> mmc dev 0
 =================================================================
 UBSAN: Undefined behaviour in drivers/mmc/mmc.c:2233:14
 index 7 is out of range for type 'int [4]'
 =================================================================

Currently, fbase[] array consists of 4 elements:
-------8<-------
static const int fbase[] = {
        10000,
        100000,
        1000000,
        10000000,
};
-------8<-------

Adjust the mask used to compute the fbase[] index accordingly.

Fixes: 272cc70b211e ("Add MMC Framework")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---

Changes in v2:
 - Shorten the summary line
---
 drivers/mmc/mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 447519f46f15..01da99edb084 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -2230,7 +2230,7 @@ static int mmc_startup(struct mmc *mmc)
 	}
 
 	/* divide frequency by 10, since the mults are 10x bigger */
-	freq = fbase[(cmd.response[0] & 0x7)];
+	freq = fbase[(cmd.response[0] & 0x3)];
 	mult = multipliers[((cmd.response[0] >> 3) & 0xf)];
 
 	mmc->legacy_speed = freq * mult;
-- 
2.18.0

[U-Boot] [PATCH v2 11/13] hashtable: Fix zero-sized array

[Eugeniu Rosca]

Enabling CONFIG_UBSAN=y, below runtime warning occurs both in sandbox
and R-Car H3 Salvator-X U-Boot:

 =====================================================================
 UBSAN: Undefined behaviour in lib/hashtable.c:784:8
 variable length array bound value 0 <= 0
 =====================================================================

It has a slightly different wording when compiling sandbox U-Boot
with "gcc-8 -fsanitize=undefined -lubsan":

-----8<-----
lib/hashtable.c:784:8: runtime error: \
  variable length array bound evaluates to non-positive value 0
-----8<-----

Inspired from v4.11-rc1 commit 620711944459 ("crypto: algif_hash -
avoid zero-sized array"), which fixes the same type of UB.

Fixes: d5370febbcbc ("env: delete selected vars not present in imported env")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---

Changes in v2:
 - Switch to a more elegant solution, inspired from v4.11-rc1
   commit 620711944459 ("crypto: algif_hash - avoid zero-sized array")
 - Shorten the summary line
 - Drop Reviewed-by due to updates
---
 include/search.h | 2 +-
 lib/hashtable.c  | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/search.h b/include/search.h
index 5d07b49073cc..417dacdb4482 100644
--- a/include/search.h
+++ b/include/search.h
@@ -99,7 +99,7 @@ extern ssize_t hexport_r(struct hsearch_data *__htab,
  */
 extern int himport_r(struct hsearch_data *__htab,
 		     const char *__env, size_t __size, const char __sep,
-		     int __flag, int __crlf_is_lf, int nvars,
+		     int __flag, int __crlf_is_lf, unsigned int nvars,
 		     char * const vars[]);
 
 /* Walk the whole table calling the callback on each element */
diff --git a/lib/hashtable.c b/lib/hashtable.c
index 1c48692b69ed..e01a42993dd3 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -778,10 +778,10 @@ static int drop_var_from_set(const char *name, int nvars, char * vars[])
 
 int himport_r(struct hsearch_data *htab,
 		const char *env, size_t size, const char sep, int flag,
-		int crlf_is_lf, int nvars, char * const vars[])
+		int crlf_is_lf, unsigned int nvars, char * const vars[])
 {
 	char *data, *sp, *dp, *name, *value;
-	char *localvars[nvars];
+	char *localvars[nvars ? : 1];
 	int i;
 
 	/* Test for correct arguments.  */
-- 
2.18.0

[U-Boot] [PATCH v2 12/13] input: Fix zero-sized array

[Eugeniu Rosca]

Fix below UBSAN reports, thrown on sandbox:

 ============================================================
 UBSAN: Undefined behaviour in drivers/input/input.c:512:7
 variable length array bound value 0 <= 0
 ============================================================
 ============================================================
 UBSAN: Undefined behaviour in drivers/input/input.c:340:6
 variable length array bound value 0 <= 0
 ============================================================

Inspired from Linux v4.11-rc1 commit 620711944459 ("crypto:
algif_hash - avoid zero-sized array").

Fixes: 9bc590e5119f ("input: Add generic keyboard input handler")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---

Changes in v2:
 - None. Newly pushed.
---
 drivers/input/input.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 29620a9e2793..a8fd197883aa 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -337,7 +337,7 @@ static int array_search(int *array, int count, int key)
 static int sort_array_by_ordering(int *dest, int count, int *order,
 				   int ocount)
 {
-	int temp[count];
+	int temp[count ? : 1];
 	int dest_count;
 	int same;	/* number of elements which are the same */
 	int i;
@@ -509,7 +509,7 @@ static int input_keycodes_to_ascii(struct input_config *config,
 static int _input_send_keycodes(struct input_config *config, int keycode[],
 				int num_keycodes, bool do_send)
 {
-	char ch[num_keycodes * ANSI_CHAR_MAX];
+	char ch[(num_keycodes * ANSI_CHAR_MAX) ? : 1];
 	int count, i, same = 0;
 	int is_repeat = 0;
 	unsigned delay_ms;
-- 
2.18.0

[U-Boot] [PATCH v2 13/13] configs: sandbox*: Enable UBSAN

[Eugeniu Rosca]

On Sun, Aug 19, 2018 at 09:51:32PM -0400, Tom Rini wrote:
> [..] we should be able to say more broadly that just about everyone
> can enable this, but only out of the box sandbox should.

Hence, turn UBSAN on for every available sandbox flavor.
Make sure the inserted line complies with `make savedefconfig`.

The size increase of sandbox_defconfig U-Boot (gcc 8.1.0):
$ size u-boot.sandbox.*
    text    data     bss     dec     hex filename
 1234958   80048  291472 1606478  18834e u-boot.sandbox.default
 1422710  272240  291472 1986422  1e4f76 u-boot.sandbox.ubsan
 +187752 +192192       0 +379944

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---

Changes in v2:
 - None. Newly pushed.
---
 configs/sandbox64_defconfig        | 1 +
 configs/sandbox_defconfig          | 1 +
 configs/sandbox_flattree_defconfig | 1 +
 configs/sandbox_noblk_defconfig    | 1 +
 configs/sandbox_spl_defconfig      | 1 +
 5 files changed, 5 insertions(+)

diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index ebb3716e487a..006b0505edbf 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -195,6 +195,7 @@ CONFIG_TPM=y
 CONFIG_LZ4=y
 CONFIG_ERRNO_STR=y
 CONFIG_OF_LIBFDT_OVERLAY=y
+CONFIG_UBSAN=y
 CONFIG_UNIT_TEST=y
 CONFIG_UT_TIME=y
 CONFIG_UT_DM=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 96e951493698..1a8434f403cb 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -200,6 +200,7 @@ CONFIG_TPM=y
 CONFIG_LZ4=y
 CONFIG_ERRNO_STR=y
 CONFIG_OF_LIBFDT_OVERLAY=y
+CONFIG_UBSAN=y
 CONFIG_UNIT_TEST=y
 CONFIG_UT_TIME=y
 CONFIG_UT_DM=y
diff --git a/configs/sandbox_flattree_defconfig b/configs/sandbox_flattree_defconfig
index ca70f0437e40..0bcee60c4d77 100644
--- a/configs/sandbox_flattree_defconfig
+++ b/configs/sandbox_flattree_defconfig
@@ -171,6 +171,7 @@ CONFIG_CMD_DHRYSTONE=y
 CONFIG_TPM=y
 CONFIG_LZ4=y
 CONFIG_ERRNO_STR=y
+CONFIG_UBSAN=y
 CONFIG_UNIT_TEST=y
 CONFIG_UT_TIME=y
 CONFIG_UT_DM=y
diff --git a/configs/sandbox_noblk_defconfig b/configs/sandbox_noblk_defconfig
index f70088ad7e9b..5da95a8a1726 100644
--- a/configs/sandbox_noblk_defconfig
+++ b/configs/sandbox_noblk_defconfig
@@ -172,6 +172,7 @@ CONFIG_CMD_DHRYSTONE=y
 CONFIG_TPM=y
 CONFIG_LZ4=y
 CONFIG_ERRNO_STR=y
+CONFIG_UBSAN=y
 CONFIG_UNIT_TEST=y
 CONFIG_UT_TIME=y
 CONFIG_UT_DM=y
diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig
index 315f721266e9..ab331cb5ae2a 100644
--- a/configs/sandbox_spl_defconfig
+++ b/configs/sandbox_spl_defconfig
@@ -191,6 +191,7 @@ CONFIG_CMD_DHRYSTONE=y
 CONFIG_TPM=y
 CONFIG_LZ4=y
 CONFIG_ERRNO_STR=y
+CONFIG_UBSAN=y
 CONFIG_UNIT_TEST=y
 CONFIG_UT_TIME=y
 CONFIG_UT_DM=y
-- 
2.18.0

[U-Boot] [PATCH v2 06/13] net: ravb: Fix signed shift overflow

[Marek Vasut]

On 08/27/2018 01:13 AM, Eugeniu Rosca wrote:
> Running "tftp" on R-Car H3 Salvator-X with CONFIG_UBSAN=y results in:
> 
> => tftp
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:237:28
>  left shift of 10 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:258:44
>  left shift of 9 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:263:46
>  left shift of 9 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:283:31
>  left shift of 9 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:288:49
>  left shift of 9 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:293:46
>  left shift of 9 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:345:2
>  left shift of 222 by 24 places cannot be represented in type 'int'
>  =====================================================================
> 
> Pinging the host results in:
> 
> => ping 192.168.2.11
>  Using ethernet at e6800000 device
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:161:21
>  left shift of 15 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:182:25
>  left shift of 15 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:182:47
>  left shift of 12 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:205:20
>  left shift of 12 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  host 192.168.2.11 is alive
> 
> There are two issues behind:
>  - calculating RAVB_DESC_DT_* bitfields
>  - assembling MAC address from its char components
> 
> Fix both.
> 
> Fixes: 8ae51b6f324e ("net: ravb: Add Renesas Ethernet RAVB driver")
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Acked-by: Marek Vasut <marek.vasut@gmail.com>
> ---
> 
> Changes in v2:
>  - Shorten the summary line
>  - Add "Acked-by: Marek Vasut <marek.vasut@gmail.com>"
> ---
>  drivers/net/ravb.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ravb.c b/drivers/net/ravb.c
> index 749562db960e..5baff198889b 100644
> --- a/drivers/net/ravb.c
> +++ b/drivers/net/ravb.c
> @@ -73,12 +73,12 @@
>  #define RAVB_RX_QUEUE_OFFSET		4
>  
>  #define RAVB_DESC_DT(n)			((n) << 28)

What about changing this instead, ((u32)(n) << 28) ?

> -#define RAVB_DESC_DT_FSINGLE		RAVB_DESC_DT(0x7)
> -#define RAVB_DESC_DT_LINKFIX		RAVB_DESC_DT(0x9)
> -#define RAVB_DESC_DT_EOS		RAVB_DESC_DT(0xa)
> -#define RAVB_DESC_DT_FEMPTY		RAVB_DESC_DT(0xc)
> -#define RAVB_DESC_DT_EEMPTY		RAVB_DESC_DT(0x3)
> -#define RAVB_DESC_DT_MASK		RAVB_DESC_DT(0xf)
> +#define RAVB_DESC_DT_FSINGLE		RAVB_DESC_DT(0x7UL)
> +#define RAVB_DESC_DT_LINKFIX		RAVB_DESC_DT(0x9UL)
> +#define RAVB_DESC_DT_EOS		RAVB_DESC_DT(0xaUL)
> +#define RAVB_DESC_DT_FEMPTY		RAVB_DESC_DT(0xcUL)
> +#define RAVB_DESC_DT_EEMPTY		RAVB_DESC_DT(0x3UL)
> +#define RAVB_DESC_DT_MASK		RAVB_DESC_DT(0xfUL)
>  
>  #define RAVB_DESC_DS(n)			(((n) & 0xfff) << 0)
>  #define RAVB_DESC_DS_MASK		0xfff
> @@ -342,8 +342,8 @@ static int ravb_write_hwaddr(struct udevice *dev)
>  	struct eth_pdata *pdata = dev_get_platdata(dev);
>  	unsigned char *mac = pdata->enetaddr;
>  
> -	writel((mac[0] << 24) | (mac[1] << 16) | (mac[2] << 8) | mac[3],
> -	       eth->iobase + RAVB_REG_MAHR);
> +	writel(((u32)mac[0] << 24) | ((u32)mac[1] << 16) | ((u32)mac[2] << 8) |
> +	       mac[3], eth->iobase + RAVB_REG_MAHR);

Not a big fan of the casts here, I wonder if there isn't some more
elegant solution. If not, so be it.

>  	writel((mac[4] << 8) | mac[5], eth->iobase + RAVB_REG_MALR);
>  
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 01/13] UBSAN: run-time undefined behavior sanity checker

[Tom Rini]

On Mon, Aug 27, 2018 at 01:13:19AM +0200, Eugeniu Rosca wrote:

> Import Undefined Behavior SANitizer from Linux Kernel v4.18, as
> implemented by Andrey Ryabinin <aryabinin@virtuozzo.com>.
> 
> Roughly, the UBSAN development history in Linux kernel looks like:
> 
> v4.18     3ca17b1f3628 ("lib/ubsan: remove null-pointer checks")
> v4.17-rc1 317506009216 ("lib/test_ubsan.c: make test_ubsan_misaligned_access() static")
> v4.17-rc1 854686f4edf4 ("lib: add testing module for UBSAN")
> v4.16-rc1 bac7a1fff792 ("lib/ubsan: remove returns-nonnull-attribute checks")
> v4.16-rc1 42440c1f9911 ("lib/ubsan: add type mismatch handler for new GCC/Clang")
> v4.16-rc1 b8fe1120b4ba ("lib/ubsan.c: s/missaligned/misaligned/")
> v4.10-rc1 0462554707d6 ("Kconfig: lib/Kconfig.ubsan fix reference to ubsan documentation")
>  v4.9-rc5 a76bcf557ef4 ("Kbuild: enable -Wmaybe-uninitialized warning for "make W=1"")
>  v4.9-rc1 725c4d22bbc4 ("ubsan: allow to disable the null sanitizer")
>  v4.9-rc1 1ead009cd622 ("docs: sphinxify ubsan.txt and move it to dev-tools")
>  v4.8-rc1 901d805c33fc ("UBSAN: fix typo in format string")
>  v4.8-rc1 6e8d666e9253 ("Disable "maybe-uninitialized" warning globally")
>  v4.6-rc1 dde5cf39d4d2 ("ubsan: fix tree-wide -Wmaybe-uninitialized false positives")
>  v4.5-rc4 7707535ab95e ("ubsan: cosmetic fix to Kconfig text")
>  v4.5-rc1 bf76f73c5f65 ("powerpc: enable UBSAN support")
>  v4.5-rc1 c6d308534aef ("UBSAN: run-time undefined behavior sanity checker")
> 
> What's not interesting for U-Boot is:
>  - 317506009216 ("lib/test_ubsan.c: make test_ubsan_misaligned_access() static")
>  - 854686f4edf4 ("lib: add testing module for UBSAN")
>    since they add a module-only test functionality.
>  - any Documentation commits.
> 
> Since dump_stack() evaluates to NOOP in U-Boot, the UBSAN report
> retains only the header from the original kernel report.
> 
> As example, below is a UB found in U-Boot thanks to UBSAN:
> 
>  ====================================================================
>  UBSAN: Undefined behaviour in drivers/net/phy/phy.c:728:19
>  left shift of 1 by 31 places cannot be represented in type 'int'
>  ====================================================================
> 
> For comparison, below is a full-fledged kernel UBSAN report, based on
> v4.17-rc4 Linux commit 0dfc0c792d69 ("iommu/vt-d: fix shift-out-of-
> bounds in bug checking"):
> 
>  ================================================================================
> UBSAN: Undefined behaviour in drivers/iommu/dmar.c:1348:3
> shift exponent 64 is too large for 32-bit type 'int'
> CPU: 2 PID: 0 Comm: swapper/2 Tainted: G     U            4.17.0-rc1+ #89
> Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.2.8 01/26/2016
> Call Trace:
>  <IRQ>
>  dump_stack+0x90/0xfb
>  ubsan_epilogue+0x9/0x40
>  __ubsan_handle_shift_out_of_bounds+0x10e/0x170
>  ? qi_flush_dev_iotlb+0x124/0x180

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180827/bef3dbb9/attachment.sig>

[U-Boot] [PATCH v2 03/13] armv8: mmu: Fix signed shift overflow

[Tom Rini]

On Mon, Aug 27, 2018 at 01:13:21AM +0200, Eugeniu Rosca wrote:

> Fix the following UBSAN warnings:
> 

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180827/ee83dc77/attachment.sig>

[U-Boot] [PATCH v2 11/13] hashtable: Fix zero-sized array

[Tom Rini]

On Mon, Aug 27, 2018 at 01:13:29AM +0200, Eugeniu Rosca wrote:

> Enabling CONFIG_UBSAN=y, below runtime warning occurs both in sandbox
> and R-Car H3 Salvator-X U-Boot:
> 
>  =====================================================================
>  UBSAN: Undefined behaviour in lib/hashtable.c:784:8
>  variable length array bound value 0 <= 0
>  =====================================================================
> 
> It has a slightly different wording when compiling sandbox U-Boot
> with "gcc-8 -fsanitize=undefined -lubsan":
> 

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180827/5f869285/attachment.sig>

[U-Boot] [PATCH v2 12/13] input: Fix zero-sized array

[Tom Rini]

On Mon, Aug 27, 2018 at 01:13:30AM +0200, Eugeniu Rosca wrote:

> Fix below UBSAN reports, thrown on sandbox:
> 
>  ============================================================
>  UBSAN: Undefined behaviour in drivers/input/input.c:512:7
>  variable length array bound value 0 <= 0
>  ============================================================
>  ============================================================
>  UBSAN: Undefined behaviour in drivers/input/input.c:340:6
>  variable length array bound value 0 <= 0
>  ============================================================
> 
> Inspired from Linux v4.11-rc1 commit 620711944459 ("crypto:
> algif_hash - avoid zero-sized array").
> 
> Fixes: 9bc590e5119f ("input: Add generic keyboard input handler")
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180827/5d1abb6d/attachment.sig>

[U-Boot] [PATCH v2 06/13] net: ravb: Fix signed shift overflow

[Eugeniu Rosca]

Hi Marek,

On Mon, Aug 27, 2018 at 01:22:54AM +0200, Marek Vasut wrote:
> On 08/27/2018 01:13 AM, Eugeniu Rosca wrote:
[...]
> >  
> >  #define RAVB_DESC_DT(n)			((n) << 28)
> 
> What about changing this instead, ((u32)(n) << 28) ?

This works too.

[...]

> >  
> > -	writel((mac[0] << 24) | (mac[1] << 16) | (mac[2] << 8) | mac[3],
> > -	       eth->iobase + RAVB_REG_MAHR);
> > +	writel(((u32)mac[0] << 24) | ((u32)mac[1] << 16) | ((u32)mac[2] << 8) |
> > +	       mac[3], eth->iobase + RAVB_REG_MAHR);
> 
> Not a big fan of the casts here, I wonder if there isn't some more
> elegant solution. If not, so be it.

Actually one cast is enough to fix the UB.
Let me know if below patch looks better to you.

diff --git a/drivers/net/ravb.c b/drivers/net/ravb.c
index 749562db960e..2190811c53bb 100644
--- a/drivers/net/ravb.c
+++ b/drivers/net/ravb.c
@@ -72,7 +72,7 @@
 #define RAVB_TX_QUEUE_OFFSET		0
 #define RAVB_RX_QUEUE_OFFSET		4
 
-#define RAVB_DESC_DT(n)			((n) << 28)
+#define RAVB_DESC_DT(n)			((u32)(n) << 28)
 #define RAVB_DESC_DT_FSINGLE		RAVB_DESC_DT(0x7)
 #define RAVB_DESC_DT_LINKFIX		RAVB_DESC_DT(0x9)
 #define RAVB_DESC_DT_EOS		RAVB_DESC_DT(0xa)
@@ -342,7 +342,7 @@ static int ravb_write_hwaddr(struct udevice *dev)
 	struct eth_pdata *pdata = dev_get_platdata(dev);
 	unsigned char *mac = pdata->enetaddr;
 
-	writel((mac[0] << 24) | (mac[1] << 16) | (mac[2] << 8) | mac[3],
+	writel(((u32)mac[0] << 24) | (mac[1] << 16) | (mac[2] << 8) | mac[3],
 	       eth->iobase + RAVB_REG_MAHR);
 
 	writel((mac[4] << 8) | mac[5], eth->iobase + RAVB_REG_MALR);


Thanks,
Eugeniu.

[U-Boot] [PATCH v2 06/13] net: ravb: Fix signed shift overflow

[Marek Vasut]

On 08/27/2018 10:24 PM, Eugeniu Rosca wrote:
> Hi Marek,

Hi,

> On Mon, Aug 27, 2018 at 01:22:54AM +0200, Marek Vasut wrote:
>> On 08/27/2018 01:13 AM, Eugeniu Rosca wrote:
> [...]
>>>  
>>>  #define RAVB_DESC_DT(n)			((n) << 28)
>>
>> What about changing this instead, ((u32)(n) << 28) ?
> 
> This works too.
> 
> [...]
> 
>>>  
>>> -	writel((mac[0] << 24) | (mac[1] << 16) | (mac[2] << 8) | mac[3],
>>> -	       eth->iobase + RAVB_REG_MAHR);
>>> +	writel(((u32)mac[0] << 24) | ((u32)mac[1] << 16) | ((u32)mac[2] << 8) |
>>> +	       mac[3], eth->iobase + RAVB_REG_MAHR);
>>
>> Not a big fan of the casts here, I wonder if there isn't some more
>> elegant solution. If not, so be it.
> 
> Actually one cast is enough to fix the UB.
> Let me know if below patch looks better to you.

I guess, it's less intrusive. What do you think ?

> diff --git a/drivers/net/ravb.c b/drivers/net/ravb.c
> index 749562db960e..2190811c53bb 100644
> --- a/drivers/net/ravb.c
> +++ b/drivers/net/ravb.c
> @@ -72,7 +72,7 @@
>  #define RAVB_TX_QUEUE_OFFSET		0
>  #define RAVB_RX_QUEUE_OFFSET		4
>  
> -#define RAVB_DESC_DT(n)			((n) << 28)
> +#define RAVB_DESC_DT(n)			((u32)(n) << 28)
>  #define RAVB_DESC_DT_FSINGLE		RAVB_DESC_DT(0x7)
>  #define RAVB_DESC_DT_LINKFIX		RAVB_DESC_DT(0x9)
>  #define RAVB_DESC_DT_EOS		RAVB_DESC_DT(0xa)
> @@ -342,7 +342,7 @@ static int ravb_write_hwaddr(struct udevice *dev)
>  	struct eth_pdata *pdata = dev_get_platdata(dev);
>  	unsigned char *mac = pdata->enetaddr;
>  
> -	writel((mac[0] << 24) | (mac[1] << 16) | (mac[2] << 8) | mac[3],
> +	writel(((u32)mac[0] << 24) | (mac[1] << 16) | (mac[2] << 8) | mac[3],
>  	       eth->iobase + RAVB_REG_MAHR);
>  
>  	writel((mac[4] << 8) | mac[5], eth->iobase + RAVB_REG_MALR);
> 
> 
> Thanks,
> Eugeniu.
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 07/13] x86: Fix signed shift overflow in MSR_IA32_APICBASE_BASE

[Bin Meng]

Hi Eugeniu,

On Mon, Aug 27, 2018 at 7:19 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
>
> Fix the following UBSAN report:
>  ======================================================================
>  UBSAN: Undefined behaviour in arch/x86/cpu/lapic.c:73:14
>  left shift of 1048575 by 12 places cannot be represented in type 'int'
>  ======================================================================
>
> Steps to reproduce the above:
> * echo CONFIG_UBSAN=y >> configs/qemu-x86_defconfig
> * make ARCH=x86 qemu-x86_defconfig all
> * qemu-system-i386 --version
>   QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.31)
> * qemu-system-i386 --nographic -bios u-boot.rom
>
> Fixes: 98568f0fa96b ("x86: Import MSR/MTRR code from Linux")
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>
> Changes in v2:
>  - None. Newly pushed.
> ---
>  arch/x86/include/asm/msr-index.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 9c1dbe61d596..d8b7b8013c74 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -370,7 +370,7 @@
>  #define MSR_IA32_APICBASE              0x0000001b
>  #define MSR_IA32_APICBASE_BSP          (1<<8)
>  #define MSR_IA32_APICBASE_ENABLE       (1<<11)
> -#define MSR_IA32_APICBASE_BASE         (0xfffff<<12)
> +#define MSR_IA32_APICBASE_BASE         (0xfffffUL << 12)

I don't understand why such warnings is emitted: "left shift of
1048575 by 12 places cannot be represented in type 'int'"

Compilers don't complain this code and Linux kernel has the same
definition here.

Regards,
Bin

[U-Boot] [PATCH v2 07/13] x86: Fix signed shift overflow in MSR_IA32_APICBASE_BASE

[Eugeniu Rosca]

Hi Bin,

cc: Masahiro, Andrey

On Tue, Aug 28, 2018 at 10:05:51AM +0800, Bin Meng wrote:
> Hi Eugeniu,
> 
> On Mon, Aug 27, 2018 at 7:19 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> >
> > Fix the following UBSAN report:
> >  ======================================================================
> >  UBSAN: Undefined behaviour in arch/x86/cpu/lapic.c:73:14
> >  left shift of 1048575 by 12 places cannot be represented in type 'int'
> >  ======================================================================
> >
> > Steps to reproduce the above:
> > * echo CONFIG_UBSAN=y >> configs/qemu-x86_defconfig
> > * make ARCH=x86 qemu-x86_defconfig all
> > * qemu-system-i386 --version
> >   QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.31)
> > * qemu-system-i386 --nographic -bios u-boot.rom
> >
> > Fixes: 98568f0fa96b ("x86: Import MSR/MTRR code from Linux")
> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > ---
> >
> > Changes in v2:
> >  - None. Newly pushed.
> > ---
> >  arch/x86/include/asm/msr-index.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> > index 9c1dbe61d596..d8b7b8013c74 100644
> > --- a/arch/x86/include/asm/msr-index.h
> > +++ b/arch/x86/include/asm/msr-index.h
> > @@ -370,7 +370,7 @@
> >  #define MSR_IA32_APICBASE              0x0000001b
> >  #define MSR_IA32_APICBASE_BSP          (1<<8)
> >  #define MSR_IA32_APICBASE_ENABLE       (1<<11)
> > -#define MSR_IA32_APICBASE_BASE         (0xfffff<<12)
> > +#define MSR_IA32_APICBASE_BASE         (0xfffffUL << 12)
> 
> I don't understand why such warnings is emitted: "left shift of
> 1048575 by 12 places cannot be represented in type 'int'"
> 
> Compilers don't complain this code and Linux kernel has the same
> definition here.

I wrote a basic kernel module printing the result of "(0xfffff << 12)"
and kernel UBSAN doesn't complain indeed.

I started to compare the compiler flags between Linux and U-Boot and
nailed down empirically that Linux UBSAN warning is inhibited by the
-fno-strict-overflow gcc option, introduced in Linux commit [1]. The
latter actually replaces another gcc option -fwrapv, introduced in [2].

Any of the two flags makes the UBSAN error vanish in the kernel.
Neither of the two flags is used in U-Boot.

I am in the process of browsing some documentation related to -fwrapv
and -fno-strict-overflow (e.g. [3]). Please, feel free to share any
thoughts and/or cc anybody who might have dealt with these topics
in the past. I will come back with more feedback later.

[1] v2.6.31 commit a137802ee839 ("Don't use '-fwrapv' compiler option: it's buggy in gcc-4.1.x")
[2] v2.6.29 commit 68df3755e383 ("Add '-fwrapv' to gcc CFLAGS")
[3] https://www.airs.com/blog/archives/120

> Regards,
> Bin

Thanks,
Eugeniu.

[U-Boot] [PATCH v2 07/13] x86: Fix signed shift overflow in MSR_IA32_APICBASE_BASE

[Andy Shevchenko]

On Tue, Aug 28, 2018 at 5:06 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> On Mon, Aug 27, 2018 at 7:19 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:


> > -#define MSR_IA32_APICBASE_BASE         (0xfffff<<12)
> > +#define MSR_IA32_APICBASE_BASE         (0xfffffUL << 12)
>
> I don't understand why such warnings is emitted: "left shift of
> 1048575 by 12 places cannot be represented in type 'int'"

Because it can't.

1 << 30 (fine)
1 << 31 (UB!)

This is good explained in the C standard for ages.

> Compilers don't complain this code and Linux kernel has the same
> definition here.

Someone suppressed those warnings. But they are valid.

-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH v2 13/13] configs: sandbox*: Enable UBSAN

[Simon Glass]

Hi,

On 26 August 2018 at 17:13, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> On Sun, Aug 19, 2018 at 09:51:32PM -0400, Tom Rini wrote:
>> [..] we should be able to say more broadly that just about everyone
>> can enable this, but only out of the box sandbox should.
>
> Hence, turn UBSAN on for every available sandbox flavor.
> Make sure the inserted line complies with `make savedefconfig`.
>
> The size increase of sandbox_defconfig U-Boot (gcc 8.1.0):
> $ size u-boot.sandbox.*
>     text    data     bss     dec     hex filename
>  1234958   80048  291472 1606478  18834e u-boot.sandbox.default
>  1422710  272240  291472 1986422  1e4f76 u-boot.sandbox.ubsan
>  +187752 +192192       0 +379944
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>
> Changes in v2:
>  - None. Newly pushed.
> ---
>  configs/sandbox64_defconfig        | 1 +
>  configs/sandbox_defconfig          | 1 +
>  configs/sandbox_flattree_defconfig | 1 +
>  configs/sandbox_noblk_defconfig    | 1 +
>  configs/sandbox_spl_defconfig      | 1 +
>  5 files changed, 5 insertions(+)
>

Can you please do this with an 'imply' in arch/Kconfig?

Regards,
Simon

[U-Boot] [PATCH v2 07/13] x86: Fix signed shift overflow in MSR_IA32_APICBASE_BASE

[Eugeniu Rosca]

Hi there,

On Tue, Aug 28, 2018 at 08:42:01AM +0200, Eugeniu Rosca wrote:
> Hi Bin,
> 
> cc: Masahiro, Andrey
> 
> On Tue, Aug 28, 2018 at 10:05:51AM +0800, Bin Meng wrote:
> > Hi Eugeniu,
> > 
> > On Mon, Aug 27, 2018 at 7:19 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> > >
> > > Fix the following UBSAN report:
> > >  ======================================================================
> > >  UBSAN: Undefined behaviour in arch/x86/cpu/lapic.c:73:14
> > >  left shift of 1048575 by 12 places cannot be represented in type 'int'
> > >  ======================================================================
> > >
> > > Steps to reproduce the above:
> > > * echo CONFIG_UBSAN=y >> configs/qemu-x86_defconfig
> > > * make ARCH=x86 qemu-x86_defconfig all
> > > * qemu-system-i386 --version
> > >   QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.31)
> > > * qemu-system-i386 --nographic -bios u-boot.rom
> > >
> > > Fixes: 98568f0fa96b ("x86: Import MSR/MTRR code from Linux")
> > > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > > ---
> > >
> > > Changes in v2:
> > >  - None. Newly pushed.
> > > ---
> > >  arch/x86/include/asm/msr-index.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> > > index 9c1dbe61d596..d8b7b8013c74 100644
> > > --- a/arch/x86/include/asm/msr-index.h
> > > +++ b/arch/x86/include/asm/msr-index.h
> > > @@ -370,7 +370,7 @@
> > >  #define MSR_IA32_APICBASE              0x0000001b
> > >  #define MSR_IA32_APICBASE_BSP          (1<<8)
> > >  #define MSR_IA32_APICBASE_ENABLE       (1<<11)
> > > -#define MSR_IA32_APICBASE_BASE         (0xfffff<<12)
> > > +#define MSR_IA32_APICBASE_BASE         (0xfffffUL << 12)
> > 
> > I don't understand why such warnings is emitted: "left shift of
> > 1048575 by 12 places cannot be represented in type 'int'"
> > 
> > Compilers don't complain this code and Linux kernel has the same
> > definition here.
> 
> I wrote a basic kernel module printing the result of "(0xfffff << 12)"
> and kernel UBSAN doesn't complain indeed.
> 
> I started to compare the compiler flags between Linux and U-Boot and
> nailed down empirically that Linux UBSAN warning is inhibited by the
> -fno-strict-overflow gcc option, introduced in Linux commit [1]. The
> latter actually replaces another gcc option -fwrapv, introduced in [2].
> 
> Any of the two flags makes the UBSAN error vanish in the kernel.
> Neither of the two flags is used in U-Boot.
> 
> I am in the process of browsing some documentation related to -fwrapv
> and -fno-strict-overflow (e.g. [3]). Please, feel free to share any
> thoughts and/or cc anybody who might have dealt with these topics
> in the past. I will come back with more feedback later.
> 
> [1] v2.6.31 commit a137802ee839 ("Don't use '-fwrapv' compiler option: it's buggy in gcc-4.1.x")
> [2] v2.6.29 commit 68df3755e383 ("Add '-fwrapv' to gcc CFLAGS")
> [3] https://www.airs.com/blog/archives/120
> 
> > Regards,
> > Bin

Just wanted to let you know that coreboot folks are going through
similar discussions in [1]. Also, experimenting with various gcc
versions and flags in my spare time, I collected some evidence [2]
showing that the behavior of GCC UBSAN (-fsanitize=undefined &
friends) may differ a lot depending on the gcc version and below
flags (none used by U-Boot, but some used in Linux kernel):
 -fwrapv
 -fstrict-overflow
 -fno-strict-overflow

Checking how -fno-strict-overflow and -fwrapv compare to each other
(since they seem to accomplish similar goals according to many sources),
I've used the sample app from [3] to see how gcc handles signed integer
wraparound depending on gcc version, flags, optimization level and
on whether UBSAN is enabled or not. The variance/inconsistency of the
results [4] is very high in my opinion.

One clear conclusion of [4] is that questions like why gcc UBSAN
complains in U-Boot but not in the Kernel require knowing at least the
parameters  tracked in [4] (and maybe more).

[1] https://mail.coreboot.org/pipermail/coreboot/2018-February/086146.html
[2] UBSAN behavior (printing 1 << 31) is highly dependent on gcc version and flags

 +----------------------+-------------+-----+
 |   gcc flags          | gcc version | UB? |
 |----------------------|-------------|-----|
 |                      |  gcc-4.9.4  |  -  |
 | -fsanitize=undefined |  gcc-5.5.0  |  y  |
 |                      |  gcc-7.3.0  |  y  |
 |                      |  gcc-8.1.0  |  y  |
 +------------------------------------------+
 |                      |  gcc-4.9.4  |  -  |
 | -fsanitize=undefined |  gcc-5.5.0  |  y  |
 | -fstrict-overflow    |  gcc-7.3.0  |  y  |
 |                      |  gcc-8.1.0  |  y  |
 +------------------------------------------+
 |                      |  gcc-4.9.4  |  -  |
 | -fsanitize=undefined |  gcc-5.5.0  |  y  |
 | -fno-strict-overflow |  gcc-7.3.0  |  y  |
 |                      |  gcc-8.1.0  |  -  |
 +------------------------------------------+
 |                      |  gcc-4.9.4  |  -  |
 | -fsanitize=undefined |  gcc-5.5.0  |  y  |
 | -fwrapv              |  gcc-7.3.0  |  -  |
 |                      |  gcc-8.1.0  |  -  |
 +----------------------+-------------+-----+

[3] http://thiemonagel.de/2010/01/signed-integer-overflow/

[4] Wraparound [3] dependency on gcc version, flags, optimization level and -fsanitize=undefined
 
 |   gcc flags             |  gcc  |      Wrapped? (UB!)          |
 |-------------------------|-------|------|-----|-----|-----|-----|
 |                         |       | -O0  | -O1 | -O2 | -O3 | -Os |
 |                         | 4.9.4 | y/y! | y/y | n/n | n/n | n/n |
 | none                    | 5.5.0 | y/y! | y/y | n/y | n/y | n/y |
 | (/-fsanitize=undefined) | 7.3.0 | y/y! | y/y | n/y | n/y | n/y |
 |                         | 8.1.0 | n/n  | n/n | n/n | n/n | n/n |
 +----------------------------------------------------------------+
 |                         | 4.9.4 | n/n  | n/n | n/n | n/n | n/n |
 | -fstrict-overflow       | 5.5.0 | n/y! | n/y | n/y | n/y | n/y |
 | (/-fsanitize=undefined) | 7.3.0 | n/y! | n/y | n/y | n/y | n/y |
 |                         | 8.1.0 | n/n  | n/n | n/n | n/n | n/n |
 +----------------------------------------------------------------+
 |                         | 4.9.4 | y/y! | y/y | y/y | y/y | y/y |
 | -fno-strict-overflow    | 5.5.0 | y/y! | y/y | y/y | y/y | y/y |
 | (/-fsanitize=undefined) | 7.3.0 | y/y! | y/y | y/y | y/y | y/y |
 |                         | 8.1.0 | y/y  | y/y | y/y | y/y | y/y |
 +----------------------------------------------------------------+
 |                         | 4.9.4 | y/y  | y/y | y/y | y/y | y/y |
 | -fwrapv                 | 5.5.0 | y/y  | y/y | y/y | y/y | y/y |
 | (/-fsanitize=undefined) | 7.3.0 | y/y  | y/y | y/y | y/y | y/y |
 |                         | 8.1.0 | y/y  | y/y | y/y | y/y | y/y |
 +----------------------------------------------------------------+

 Comments/suggestions appreciated.

 Best regards,
 Eugeniu.

[U-Boot] [PATCH v2 07/13] x86: Fix signed shift overflow in MSR_IA32_APICBASE_BASE

[Bin Meng]

Hi Eugeniu,

On Sat, Sep 1, 2018 at 6:59 PM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
>
> Hi there,
>
> On Tue, Aug 28, 2018 at 08:42:01AM +0200, Eugeniu Rosca wrote:
> > Hi Bin,
> >
> > cc: Masahiro, Andrey
> >
> > On Tue, Aug 28, 2018 at 10:05:51AM +0800, Bin Meng wrote:
> > > Hi Eugeniu,
> > >
> > > On Mon, Aug 27, 2018 at 7:19 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> > > >
> > > > Fix the following UBSAN report:
> > > >  ======================================================================
> > > >  UBSAN: Undefined behaviour in arch/x86/cpu/lapic.c:73:14
> > > >  left shift of 1048575 by 12 places cannot be represented in type 'int'
> > > >  ======================================================================
> > > >
> > > > Steps to reproduce the above:
> > > > * echo CONFIG_UBSAN=y >> configs/qemu-x86_defconfig
> > > > * make ARCH=x86 qemu-x86_defconfig all
> > > > * qemu-system-i386 --version
> > > >   QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.31)
> > > > * qemu-system-i386 --nographic -bios u-boot.rom
> > > >
> > > > Fixes: 98568f0fa96b ("x86: Import MSR/MTRR code from Linux")
> > > > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > > > ---
> > > >
> > > > Changes in v2:
> > > >  - None. Newly pushed.
> > > > ---
> > > >  arch/x86/include/asm/msr-index.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> > > > index 9c1dbe61d596..d8b7b8013c74 100644
> > > > --- a/arch/x86/include/asm/msr-index.h
> > > > +++ b/arch/x86/include/asm/msr-index.h
> > > > @@ -370,7 +370,7 @@
> > > >  #define MSR_IA32_APICBASE              0x0000001b
> > > >  #define MSR_IA32_APICBASE_BSP          (1<<8)
> > > >  #define MSR_IA32_APICBASE_ENABLE       (1<<11)
> > > > -#define MSR_IA32_APICBASE_BASE         (0xfffff<<12)
> > > > +#define MSR_IA32_APICBASE_BASE         (0xfffffUL << 12)
> > >
> > > I don't understand why such warnings is emitted: "left shift of
> > > 1048575 by 12 places cannot be represented in type 'int'"
> > >
> > > Compilers don't complain this code and Linux kernel has the same
> > > definition here.
> >
> > I wrote a basic kernel module printing the result of "(0xfffff << 12)"
> > and kernel UBSAN doesn't complain indeed.
> >
> > I started to compare the compiler flags between Linux and U-Boot and
> > nailed down empirically that Linux UBSAN warning is inhibited by the
> > -fno-strict-overflow gcc option, introduced in Linux commit [1]. The
> > latter actually replaces another gcc option -fwrapv, introduced in [2].
> >
> > Any of the two flags makes the UBSAN error vanish in the kernel.
> > Neither of the two flags is used in U-Boot.
> >
> > I am in the process of browsing some documentation related to -fwrapv
> > and -fno-strict-overflow (e.g. [3]). Please, feel free to share any
> > thoughts and/or cc anybody who might have dealt with these topics
> > in the past. I will come back with more feedback later.
> >
> > [1] v2.6.31 commit a137802ee839 ("Don't use '-fwrapv' compiler option: it's buggy in gcc-4.1.x")
> > [2] v2.6.29 commit 68df3755e383 ("Add '-fwrapv' to gcc CFLAGS")
> > [3] https://www.airs.com/blog/archives/120
> >
> > > Regards,
> > > Bin
>
> Just wanted to let you know that coreboot folks are going through
> similar discussions in [1]. Also, experimenting with various gcc
> versions and flags in my spare time, I collected some evidence [2]
> showing that the behavior of GCC UBSAN (-fsanitize=undefined &
> friends) may differ a lot depending on the gcc version and below
> flags (none used by U-Boot, but some used in Linux kernel):
>  -fwrapv
>  -fstrict-overflow
>  -fno-strict-overflow
>
> Checking how -fno-strict-overflow and -fwrapv compare to each other
> (since they seem to accomplish similar goals according to many sources),
> I've used the sample app from [3] to see how gcc handles signed integer
> wraparound depending on gcc version, flags, optimization level and
> on whether UBSAN is enabled or not. The variance/inconsistency of the
> results [4] is very high in my opinion.
>
> One clear conclusion of [4] is that questions like why gcc UBSAN
> complains in U-Boot but not in the Kernel require knowing at least the
> parameters  tracked in [4] (and maybe more).
>
> [1] https://mail.coreboot.org/pipermail/coreboot/2018-February/086146.html
> [2] UBSAN behavior (printing 1 << 31) is highly dependent on gcc version and flags
>
>  +----------------------+-------------+-----+
>  |   gcc flags          | gcc version | UB? |
>  |----------------------|-------------|-----|
>  |                      |  gcc-4.9.4  |  -  |
>  | -fsanitize=undefined |  gcc-5.5.0  |  y  |
>  |                      |  gcc-7.3.0  |  y  |
>  |                      |  gcc-8.1.0  |  y  |
>  +------------------------------------------+
>  |                      |  gcc-4.9.4  |  -  |
>  | -fsanitize=undefined |  gcc-5.5.0  |  y  |
>  | -fstrict-overflow    |  gcc-7.3.0  |  y  |
>  |                      |  gcc-8.1.0  |  y  |
>  +------------------------------------------+
>  |                      |  gcc-4.9.4  |  -  |
>  | -fsanitize=undefined |  gcc-5.5.0  |  y  |
>  | -fno-strict-overflow |  gcc-7.3.0  |  y  |
>  |                      |  gcc-8.1.0  |  -  |
>  +------------------------------------------+
>  |                      |  gcc-4.9.4  |  -  |
>  | -fsanitize=undefined |  gcc-5.5.0  |  y  |
>  | -fwrapv              |  gcc-7.3.0  |  -  |
>  |                      |  gcc-8.1.0  |  -  |
>  +----------------------+-------------+-----+
>
> [3] http://thiemonagel.de/2010/01/signed-integer-overflow/
>
> [4] Wraparound [3] dependency on gcc version, flags, optimization level and -fsanitize=undefined
>
>  |   gcc flags             |  gcc  |      Wrapped? (UB!)          |
>  |-------------------------|-------|------|-----|-----|-----|-----|
>  |                         |       | -O0  | -O1 | -O2 | -O3 | -Os |
>  |                         | 4.9.4 | y/y! | y/y | n/n | n/n | n/n |
>  | none                    | 5.5.0 | y/y! | y/y | n/y | n/y | n/y |
>  | (/-fsanitize=undefined) | 7.3.0 | y/y! | y/y | n/y | n/y | n/y |
>  |                         | 8.1.0 | n/n  | n/n | n/n | n/n | n/n |
>  +----------------------------------------------------------------+
>  |                         | 4.9.4 | n/n  | n/n | n/n | n/n | n/n |
>  | -fstrict-overflow       | 5.5.0 | n/y! | n/y | n/y | n/y | n/y |
>  | (/-fsanitize=undefined) | 7.3.0 | n/y! | n/y | n/y | n/y | n/y |
>  |                         | 8.1.0 | n/n  | n/n | n/n | n/n | n/n |
>  +----------------------------------------------------------------+
>  |                         | 4.9.4 | y/y! | y/y | y/y | y/y | y/y |
>  | -fno-strict-overflow    | 5.5.0 | y/y! | y/y | y/y | y/y | y/y |
>  | (/-fsanitize=undefined) | 7.3.0 | y/y! | y/y | y/y | y/y | y/y |
>  |                         | 8.1.0 | y/y  | y/y | y/y | y/y | y/y |
>  +----------------------------------------------------------------+
>  |                         | 4.9.4 | y/y  | y/y | y/y | y/y | y/y |
>  | -fwrapv                 | 5.5.0 | y/y  | y/y | y/y | y/y | y/y |
>  | (/-fsanitize=undefined) | 7.3.0 | y/y  | y/y | y/y | y/y | y/y |
>  |                         | 8.1.0 | y/y  | y/y | y/y | y/y | y/y |
>  +----------------------------------------------------------------+
>
>  Comments/suggestions appreciated.

Thank you very much for all these details and links. I learned a lot
from this thread. It looks to me that coreboot folks are not in favor
of this UBSAN thing. I personally have no preference, but I suspect
there are a lot more in the U-Boot tree that have such warnings. Given
the behavior is quite dependent on GCC versions and flags, do kernel
folks have any final solution on this? Or do we have to ask GCC folks
to fix these inconsistencies?

Regards,
Bin

[U-Boot] [PATCH v2 07/13] x86: Fix signed shift overflow in MSR_IA32_APICBASE_BASE

[Eugeniu Rosca]

Hi Bin,

Apologize for the delay. I came back from vacation a few days ago.

On Tue, Sep 04, 2018 at 12:00:14PM +0800, Bin Meng wrote:
> Hi Eugeniu,
> 
> On Sat, Sep 1, 2018 at 6:59 PM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
[..]

> > Just wanted to let you know that coreboot folks are going through
> > similar discussions in [1]. Also, experimenting with various gcc
> > versions and flags in my spare time, I collected some evidence [2]
> > showing that the behavior of GCC UBSAN (-fsanitize=undefined &
> > friends) may differ a lot depending on the gcc version and below
> > flags (none used by U-Boot, but some used in Linux kernel):
> >  -fwrapv
> >  -fstrict-overflow
> >  -fno-strict-overflow
> >
> > Checking how -fno-strict-overflow and -fwrapv compare to each other
> > (since they seem to accomplish similar goals according to many sources),
> > I've used the sample app from [3] to see how gcc handles signed integer
> > wraparound depending on gcc version, flags, optimization level and
> > on whether UBSAN is enabled or not. The variance/inconsistency of the
> > results [4] is very high in my opinion.
> >
> > One clear conclusion of [4] is that questions like why gcc UBSAN
> > complains in U-Boot but not in the Kernel require knowing at least the
> > parameters  tracked in [4] (and maybe more).
> >
> > [1] https://mail.coreboot.org/pipermail/coreboot/2018-February/086146.html
> > [2] UBSAN behavior (printing 1 << 31) is highly dependent on gcc version and flags
[..]

> Thank you very much for all these details and links. I learned a lot
> from this thread. It looks to me that coreboot folks are not in favor
> of this UBSAN thing. I personally have no preference, but I suspect
> there are a lot more in the U-Boot tree that have such warnings.

I don't think they question the usefulness of UBSAN as a whole. Rather,
they seem to be annoyed by one particular (frequently reproduced) UBSAN
error, specifically what gcc man pages refer to as "left-shifting 1 into
the sign bit". Note that shifting *past* the sign bit is a different
(presumably more serious) subclass of signed integer shift overflow.
Both the coreboot discussion and the fixes from my series are limited
to "left-shifting into (not past) the sign bit" behavior.

Both the C11 [6] standard (to which U-Boot "adhered" via commit [5]) and
the later C18 [7] define the left shifts as follows (§6.5.7p4):

------8<------
The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits
are filled with zeros. If E1 has an unsigned type, the value of the result
is E1 × 2^E2, reduced modulo one more than the maximum value representable
in the result type. If E1 has a signed type and nonnegative value, and
E1 × 2^E2 is representable in the result type, then that is the resulting
value; otherwise, the behavior is undefined.
------8<------

With respect to the type of the result, both C11/C18 standards state
in §6.5.7p3:

------8<------
The type of the result is that of the promoted left operand.
------8<------

My understanding of the above is that, purely from C11/C18 standard
perspective, (1 << 31) is undefined behavior since the result can't be
represented in the type of the left operand (signed int).

In spite of this, things are not as simple as we would like them to be
and the DR463 entry of the "Defect Report Summary for C11" [8] tackles
exactly the "Left-shifting into the sign bit" by saying that it should
be harmonized with C++-14 [9]. The latter suffered a change in
"§5.8.2 Shift operators" chapter due to DR1457 ("Undefined behavior in
left-shift") [10], a defect report raised back in 2012. As a result,
C++-14 now considers the "left-shifting into the sign bit" as defined
behavior:

------8<------
...if E1 has a signed type and non-negative value, and E1 ⨯ 2^E2 is
representable in the **corresponding unsigned type of the result type**,
then that value, **converted to the result type**, is the resulting
value; otherwise, the behavior is undefined.
------8<------

To emphasize that the things are far from being settled, I will just
reference the UB-related talk [11] of Chandler Carruth@CppCon 2016,
which (amongst other topics) touches the left shifting of signed
integers and, to my understanding, conveys the message that there are
holes in the mental model of shifting signed integers to the left and
the solution to overcome those is still unclear.

> Given
> the behavior is quite dependent on GCC versions and flags, do kernel
> folks have any final solution on this?

I still didn't fully answer one of your first questions, more exactly
why UBSAN complains about (1 << 31) in U-Boot, but not in Linux kernel.
It turns out there is another gcc option heavily affecting the UBSAN
behavior and it is (somewhat expectedly) the language standard
"-std=" [12]. As already mentioned, U-Boot recently switched to C11
via commit [5], while Linux kernel still sticks to ANSI/C89 C
via commit [13].

Just for the record, the definition of left shift operator provided
by C89/C90 [14] is indeed different compared to C11/C18 and it sounds
like:

------8<------
The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated
bits are filled with zeros. If E1 has an unsigned type, the value of
the result is E1 multiplied by the quantity, 2 raised to the power E2,
reduced modulo ULONG_MAX+1 if E1 has type unsigned long, UINT_MAX+1
otherwise. (The constants ULONG_MAX and UINT_MAX are defined in the
header <limits.h> .)
------8<------

I am not sure if this makes left-shifting into the sign bit legitimate,
but the reality is that none of the x86_64 gcc versions I tried (this
includes 4.9.4, 5.5.0, 6.4.0, 7.3.0, 8.1.0) reported issues about
(1 << 31) when turning UBSAN on with -std=gnu89. This is the main
reason why Linux kernel UBSAN won't complain about (1 << 31).

On the basis of above experimental results, switching U-Boot build
system back to C89 would both keep it aligned to Linux kernel and
would consistently silent all the "left-shift 1 into sign bit" UBSAN
errors (of which there are potentially hundreds, according to the stats
presented in [15]). At the same time, this feels like a step back, so
I actually expect people to NAK this change. It still remains the
simplest I can think of (hence my favorite). It is also grounded
on my belief that the changes in the Kconfig/Kbuild should come top-down
to U-Boot from Linux kernel (where the actual development takes place).

> Or do we have to ask GCC folks to fix these inconsistencies?

While trying to create an account in GCC Bugzilla, I got the message:
"User account creation has been restricted" :(

Please, feel free to CC any member of GCC community.
Any feedback/comments which direction to take would be appreciated.

[5] fa893990e9b5 ("Makefile: Ensure we build with -std=gnu11")
[6] C11 draft: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf
[7] C18 draft: http://www.open-std.org/jtc1/sc22/wg14/www/abq/c17_updated_proposed_fdis.pdf
[8] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2148.htm#dr_463
[9] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4296.pdf
[10] http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1457
[11] https://youtu.be/yG1OZ69H_-o?t=1668
[12] https://gcc.gnu.org/onlinedocs/gcc/Standards.html
[13] Linux v3.18-rc5 commit 51b97e354ba9 ("kernel: use the gnu89 standard explicitly")
[14] https://port70.net/~nsz/c/c89/c89-draft.html#3.3.7
[15] http://patchwork.ozlabs.org/cover/962307/

Best regards,
Eugeniu.

[U-Boot] [PATCH v2 13/13] configs: sandbox*: Enable UBSAN

[Eugeniu Rosca]

Hi Simon,

On Wed, Aug 29, 2018 at 08:51:24PM -0600, Simon Glass wrote:
> Hi,
> 
> On 26 August 2018 at 17:13, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> > On Sun, Aug 19, 2018 at 09:51:32PM -0400, Tom Rini wrote:
> >> [..] we should be able to say more broadly that just about everyone
> >> can enable this, but only out of the box sandbox should.
> >
> > Hence, turn UBSAN on for every available sandbox flavor.
> > Make sure the inserted line complies with `make savedefconfig`.
> >
> > The size increase of sandbox_defconfig U-Boot (gcc 8.1.0):
> > $ size u-boot.sandbox.*
> >     text    data     bss     dec     hex filename
> >  1234958   80048  291472 1606478  18834e u-boot.sandbox.default
> >  1422710  272240  291472 1986422  1e4f76 u-boot.sandbox.ubsan
> >  +187752 +192192       0 +379944
> >
> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > ---
> >
> > Changes in v2:
> >  - None. Newly pushed.
> > ---
> >  configs/sandbox64_defconfig        | 1 +
> >  configs/sandbox_defconfig          | 1 +
> >  configs/sandbox_flattree_defconfig | 1 +
> >  configs/sandbox_noblk_defconfig    | 1 +
> >  configs/sandbox_spl_defconfig      | 1 +
> >  5 files changed, 5 insertions(+)
> >
> 
> Can you please do this with an 'imply' in arch/Kconfig?

Thanks for your review comment. I will incorporate the change in the
next patch revision, once we clarify how to deal with the "left-shifting
of 1 into the sign bit" UBSAN warnings, which is being discussed in
https://patchwork.ozlabs.org/patch/962305/#1991283 .

> Regards,
> Simon

Regards,
Eugeniu.

[U-Boot] [PATCH v2 07/13] x86: Fix signed shift overflow in MSR_IA32_APICBASE_BASE

[Eugeniu Rosca]

Hi Bin,

jFYI, I've created https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87392
("UBSAN behavior on left-shifting 1 into the sign bit is dependent on C
standard"), to get some recommendation from GCC guys how to handle
these warnings in U-Boot.

Regards,
Eugeniu.

[U-Boot] [PATCH v2 07/13] x86: Fix signed shift overflow in MSR_IA32_APICBASE_BASE

[Bin Meng]

Hi Eugeniu,

On Sun, Sep 23, 2018 at 7:10 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
>
> Hi Bin,
>
> jFYI, I've created https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87392
> ("UBSAN behavior on left-shifting 1 into the sign bit is dependent on C
> standard"), to get some recommendation from GCC guys how to handle
> these warnings in U-Boot.

Thank you very much for following up with the gcc folks! Let's see how
they respond.

BTW: your bug report is elaborate. Well done on the research!

Regards,
Bin

[U-Boot] [PATCH v2 07/13] x86: Fix signed shift overflow in MSR_IA32_APICBASE_BASE

[Eugeniu Rosca]

Hi Bin,

On Tue, Sep 25, 2018 at 10:06:52AM +0800, Bin Meng wrote:
> Hi Eugeniu,
> 
> On Sun, Sep 23, 2018 at 7:10 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> >
> > Hi Bin,
> >
> > jFYI, I've created https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87392
> > ("UBSAN behavior on left-shifting 1 into the sign bit is dependent on C
> > standard"), to get some recommendation from GCC guys how to handle
> > these warnings in U-Boot.
> 
> Thank you very much for following up with the gcc folks! Let's see how
> they respond.
> 
> BTW: your bug report is elaborate. Well done on the research!
> 
> Regards,
> Bin

I feel like before UBSAN reaches mainline U-Boot, we will make some
friends in the compiler communities. I have raised another bug
report [1], this time to LLVM folks, since U-Boot simply refuses to
boot when built with clang and UBSAN=y.

This new issue is related to the implementation of U-Boot
linker-generated arrays, as summarized in the cover letter [2] of my
series. Somehow, GCC UBSAN cooperates well with the linker-generated
arrays, while Clang UBSAN does not. Hopefully this will be clarified
in [1] and hopefully no significant changes will be needed in
include/linker_lists.h to allow booting -fsanitized clang-built U-Boot.

Regarding the GCC discussion [3], it is relatively settled, but not to
our advantage. GCC folks first clarified (credits to them for that)
how shifting into (not past) the sign bit is defined in the existing
C standards. Specifically, C89/C90 considers this
"implementation-defined", while more recent C standards (C99, C11, C18)
make this "undefined". Since U-Boot is compiled using -std=gnu11,
"shifting into the sign bit" errors look legitimate.

On the other hand, official GCC documentation says [4]:

> As an extension to the C language, GCC does not use the latitude given
> in C99 and C11 only to treat certain aspects of signed ‘<<’ as
> undefined. 

The above quote was used by GCC guys to actually support/convey the idea
that some aspects of left-shifting (e.g. left-shifting into the sign
bit) are still defined in GCC (i.e. they don't lead to UB). If so, then
I am really puzzled, since I do not understand the practicality of
bothering users with errors which reflect what C standard says on paper
instead of how it is implemented in the compiler internals.

This is pretty much the most recent status of the discussion and, as you
can see, it doesn't shed too much light on how to tackle the left-
shifting overflows into the sign bit (fix them, ignore them, roll back
the C standard, etc). This is still to be decided by the U-Boot
community.

[1] https://bugs.llvm.org/show_bug.cgi?id=39219
[2] https://patchwork.ozlabs.org/cover/962307/
[3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87392
[4] https://gcc.gnu.org/onlinedocs/gcc-8.2.0/gcc/Integers-implementation.html#Integers-implementation

Best regards,
Eugeniu.