[PATCH 0/5] sandbox: support -fsanitize={address,ubsan}

[PATCH 0/5] sandbox: support -fsanitize={address,ubsan}

[Ahmad Fatoum]

I get a double free inside of SDL's X11 backend when trying to use the
sandbox SDL video driver on wayland. I am not sure what to do about this
yet, but I made these few patches to get nicer looking backtraces for a
start.

Ahmad Fatoum (5):
  sandbox: include header to provide missing prototype
  common: add generic CONFIG_UBSAN_SANITIZE_ALL plumbing
  common: add generic CONFIG_KASAN option
  sandbox: support Address and UndefinedBehavior sanitizers
  commands: fix unaligned accesses of aliased commands on amd64

 Makefile                     |  5 +++++
 arch/sandbox/Kconfig         |  2 ++
 arch/sandbox/Makefile        | 13 ++++++++++++-
 arch/sandbox/board/devices.c |  1 +
 common/Kconfig               | 34 ++++++++++++++++++++++++++++++++++
 common/command.c             |  2 +-
 scripts/Makefile.lib         |  5 +++++
 scripts/Makefile.ubsan       | 19 +++++++++++++++++++
 8 files changed, 79 insertions(+), 2 deletions(-)
 create mode 100644 scripts/Makefile.ubsan

-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/5] sandbox: include header to provide missing prototype

[Ahmad Fatoum]

sandbox_add_device lacks a prototype. Include the appropriate header to
fix this.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 arch/sandbox/board/devices.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/sandbox/board/devices.c b/arch/sandbox/board/devices.c
index 242a0d718e91..72e62552a3b8 100644
--- a/arch/sandbox/board/devices.c
+++ b/arch/sandbox/board/devices.c
@@ -8,6 +8,7 @@
 #include <driver.h>
 #include <mach/linux.h>
 #include <init.h>
+#include <mach/linux.h>
 
 static LIST_HEAD(sandbox_device_list);
 
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/5] common: add generic CONFIG_UBSAN_SANITIZE_ALL plumbing

[Ahmad Fatoum]

-fsanitize=undefined allows compile-time instrumentation of code to detect
some classes of runtime undefined behavior.

In preparation for allowing arches to provide infrastructure in support
of this feature, add some generic UBSAN options and associated plumbing.

These are only shown in the debug menu when the arch selects the
appropriate symbol. The option is named equally to their Linux counterparts.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 Makefile               |  5 +++++
 common/Kconfig         | 25 +++++++++++++++++++++++++
 scripts/Makefile.lib   |  5 +++++
 scripts/Makefile.ubsan | 19 +++++++++++++++++++
 4 files changed, 54 insertions(+)
 create mode 100644 scripts/Makefile.ubsan

diff --git a/Makefile b/Makefile
index 0d6fc5f6beca..2117a332aa6f 100644
--- a/Makefile
+++ b/Makefile
@@ -262,6 +262,9 @@ MAKEFLAGS += --include-dir=$(srctree)
 include $(srctree)/scripts/Kbuild.include
 include $(srctree)/scripts/Makefile.lib
 
+# Populate Sanitizer CFLAGS
+include $(srctree)/scripts/Makefile.ubsan
+
 # Make variables (CC, etc...)
 
 AS		= $(CROSS_COMPILE)as
@@ -329,6 +332,8 @@ export CFLAGS CFLAGS_KERNEL
 export AFLAGS AFLAGS_KERNEL
 export LDFLAGS_barebox
 
+export CFLAGS_UBSAN
+
 # Files to ignore in find ... statements
 
 RCS_FIND_IGNORE := \( -name SCCS -o -name BitKeeper -o -name .svn -o -name CVS -o -name .pc -o -name .hg -o -name .git \) -prune -o
diff --git a/common/Kconfig b/common/Kconfig
index 8aad5baecdda..0db7b21a3b30 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -1288,6 +1288,7 @@ config DEBUG_INITCALLS
 	  If enabled this will print initcall traces.
 
 
+
 config PBL_BREAK
 	bool "Execute software break on pbl start"
 	depends on ARM
@@ -1295,6 +1296,26 @@ config PBL_BREAK
 	  If this enabled, barebox will be compiled with BKPT instruction
 	  on early pbl init. This option should be used only with JTAG debugger!
 
+config UBSAN_SANITIZE_ALL
+	bool "Undefined behaviour sanity checker"
+	depends on ARCH_HAS_UBSAN_SANITIZE_ALL
+	help
+	  This option enables the UBSan compile-time instrumentation
+	  for detecting various undefined behaviours during barebox
+	  runtime.
+
+config UBSAN_NO_ALIGNMENT
+	bool "Disable checking of pointers alignment"
+	depends on UBSAN
+	default y if HAVE_EFFICIENT_UNALIGNED_ACCESS
+	help
+	  This option disables the check of unaligned memory accesses.
+	  Disabling this option on architectures that support unaligned
+	  accesses may produce a lot of false positives.
+
+config UBSAN_ALIGNMENT
+	def_bool !UBSAN_NO_ALIGNMENT
+
 endmenu
 
 config HAS_DEBUG_LL
@@ -1303,3 +1324,7 @@ config HAS_DEBUG_LL
 config DDR_SPD
 	bool
 	select CRC_ITU_T
+
+config ARCH_HAS_UBSAN_SANITIZE_ALL
+	bool
+
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index fc5fe3d7e8bc..21feb07dcd9a 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -120,6 +120,11 @@ _c_flags       = $(CFLAGS) $(EXTRA_CFLAGS) $(CFLAGS_$(basetarget).o)
 _a_flags       = $(AFLAGS) $(EXTRA_AFLAGS) $(AFLAGS_$(basetarget).o)
 _cpp_flags     = $(CPPFLAGS) $(EXTRA_CPPFLAGS) $(CPPFLAGS_$(@F))
 
+ifeq ($(CONFIG_UBSAN),y)
+_c_flags += $(CFLAGS_UBSAN)
+LDFLAGS  += $(LDFLAGS_UBSAN)
+endif
+
 # If building barebox 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..019771b845c5
--- /dev/null
+++ b/scripts/Makefile.ubsan
@@ -0,0 +1,19 @@
+# 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=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.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/5] common: add generic CONFIG_KASAN option

[Ahmad Fatoum]

-fsanitize=[kernel-]address-sanitizer allows compile-time instrumentation
of memory accesses to detect some classes of runtime undefined behavior.

In preparation for allowing arches to provide infrastructure in support
of this feature, add the generic KASAN options.

These are only shown in the debug menu when the arch selects the
appropriate symbol. The option is named equally to their Linux counterparts.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 common/Kconfig | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/common/Kconfig b/common/Kconfig
index 0db7b21a3b30..edbc4b3a160f 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -1316,6 +1316,13 @@ config UBSAN_NO_ALIGNMENT
 config UBSAN_ALIGNMENT
 	def_bool !UBSAN_NO_ALIGNMENT
 
+config KASAN
+	bool "KASAN: runtime memory debugger"
+	depends on HAVE_ARCH_KASAN
+	help
+	  Enables KASAN (KernelAddressSANitizer) - runtime memory debugger,
+	  designed to find out-of-bounds accesses and use-after-free bugs.
+
 endmenu
 
 config HAS_DEBUG_LL
@@ -1328,3 +1335,5 @@ config DDR_SPD
 config ARCH_HAS_UBSAN_SANITIZE_ALL
 	bool
 
+config HAVE_ARCH_KASAN
+	bool
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/5] sandbox: support Address and UndefinedBehavior sanitizers

[Ahmad Fatoum]

For the hosted implementation, we just need to pass the appropriate
options at compile and link time. No further arch-specific
code needed.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 arch/sandbox/Kconfig  |  2 ++
 arch/sandbox/Makefile | 13 ++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/sandbox/Kconfig b/arch/sandbox/Kconfig
index 5227cb624f2b..254ae9a5d30c 100644
--- a/arch/sandbox/Kconfig
+++ b/arch/sandbox/Kconfig
@@ -2,6 +2,8 @@ config SANDBOX
 	bool
 	select OFTREE
 	select GPIOLIB
+	select ARCH_HAS_UBSAN_SANITIZE_ALL
+	select HAVE_ARCH_KASAN
 	default y
 
 config ARCH_TEXT_BASE
diff --git a/arch/sandbox/Makefile b/arch/sandbox/Makefile
index 85c70b5e8059..1504b2390ddd 100644
--- a/arch/sandbox/Makefile
+++ b/arch/sandbox/Makefile
@@ -45,9 +45,20 @@ ifeq ($(CONFIG_GPIO_LIBFTDI1),y)
 FTDI1_LIBS := $(shell pkg-config libftdi1 --libs)
 endif
 
+ifeq ($(CONFIG_KASAN),y)
+SANITIZERS += -fsanitize=address
+endif
+
+ifeq ($(CONFIG_UBSAN),y)
+SANITIZERS += $(CFLAGS_UBSAN)
+endif
+
+CPPFLAGS += $(SANITIZERS)
+
 cmd_barebox__ = $(CC) -o $@ -Wl,-T,$(barebox-lds) \
 	-Wl,--start-group $(barebox-common) -Wl,--end-group \
-	-lrt -lpthread $(SDL_LIBS) $(FTDI1_LIBS)
+	-lrt -lpthread $(SDL_LIBS) $(FTDI1_LIBS) \
+	$(SANITIZERS)
 
 common-y += $(BOARD) arch/sandbox/os/
 
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 5/5] commands: fix unaligned accesses of aliased commands on amd64

[Ahmad Fatoum]

If a command has aliases (e.g. edit and sedit), register_command
allocates a new struct command for the alias. As struct command has a
alignment of 64 specified on __x86_64__, this new allocation needs to
observe the alignment lest unaligned access could occur. I don't think
it's likely that GCC would generate SIMD code here that expects a 64 byte
alignment, but heed UBSan's advice and use xmemalign with the appropriate
alignment.

Fixes: 8c14b97758 ("svn_rev_477")
Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
I am not sure about this, because I don't understand why there was a 64
bit alignment in the x86_64 linker script in the first place.
The fix is trivial though, so even if it's only a theoretical, lets have
it? 
---
 common/command.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/command.c b/common/command.c
index d9cc4a6d4887..09655e9e78d5 100644
--- a/common/command.c
+++ b/common/command.c
@@ -106,7 +106,7 @@ int register_command(struct command *cmd)
 	if (cmd->aliases) {
 		const char * const *aliases = cmd->aliases;
 		while(*aliases) {
-			struct command *c = xzalloc(sizeof(struct command));
+			struct command *c = xmemalign(__alignof__(*c), sizeof *c);
 
 			memcpy(c, cmd, sizeof(struct command));
 
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 5/5] commands: fix unaligned accesses of aliased commands on amd64

[Sascha Hauer]

On Thu, Aug 22, 2019 at 08:19:41AM +0200, Ahmad Fatoum wrote:
> If a command has aliases (e.g. edit and sedit), register_command
> allocates a new struct command for the alias. As struct command has a
> alignment of 64 specified on __x86_64__, this new allocation needs to
> observe the alignment lest unaligned access could occur. I don't think
> it's likely that GCC would generate SIMD code here that expects a 64 byte
> alignment, but heed UBSan's advice and use xmemalign with the appropriate
> alignment.
> 
> Fixes: 8c14b97758 ("svn_rev_477")
> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
> ---
> I am not sure about this, because I don't understand why there was a 64
> bit alignment in the x86_64 linker script in the first place.
> The fix is trivial though, so even if it's only a theoretical, lets have
> it?

Have you tried removing the alignment? AFAIK without it we are not able
to iterate over the commands array generated by the linker. I don't know
if this issue is still present. There is no requirement to align a
struct command to 64bit otherwise.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 5/5] commands: fix unaligned accesses of aliased commands on amd64

[Ahmad Fatoum]

On 8/23/19 9:36 AM, Sascha Hauer wrote:
> On Thu, Aug 22, 2019 at 08:19:41AM +0200, Ahmad Fatoum wrote:
>> If a command has aliases (e.g. edit and sedit), register_command
>> allocates a new struct command for the alias. As struct command has a
>> alignment of 64 specified on __x86_64__, this new allocation needs to
>> observe the alignment lest unaligned access could occur. I don't think
>> it's likely that GCC would generate SIMD code here that expects a 64 byte
>> alignment, but heed UBSan's advice and use xmemalign with the appropriate
>> alignment.
>>
>> Fixes: 8c14b97758 ("svn_rev_477")
>> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
>> ---
>> I am not sure about this, because I don't understand why there was a 64
>> bit alignment in the x86_64 linker script in the first place.
>> The fix is trivial though, so even if it's only a theoretical, lets have
>> it?
> 
> Have you tried removing the alignment? AFAIK without it we are not able
> to iterate over the commands array generated by the linker. I don't know
> if this issue is still present. There is no requirement to align a
> struct command to 64bit otherwise.

Ah, I see it now. The issue isn't the first element's alignment, but the
follow up ones. sizeof(struct command) is 88 on Linux amd64, but the linker
without being told anything decided to leave 128 between the start of each
element, while barebox assumes a stride of sizeof(struct command) and thus
it trips over while parsing the second command.

Hmm, please drop this patch here for now.

Thanks
Ahmad


> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/5] sandbox: support -fsanitize={address,ubsan}

[Ahmad Fatoum]

On 8/22/19 8:19 AM, Ahmad Fatoum wrote:
> I get a double free inside of SDL's X11 backend when trying to use the
> sandbox SDL video driver on wayland. I am not sure what to do about this
> yet, but I made these few patches to get nicer looking backtraces for a
> start.

FTR: SDL video driver is broken since b3fbfad7ae ("fs: dentry cache implementation")
Adding -fvisibility=hidden to the sandbox barebox link line fixes it, so I assume the
commit causes some symbol collision with another library that barebox is linked against
on my system. Haven't found out which symbol it is yet though.

> 
> Ahmad Fatoum (5):
>   sandbox: include header to provide missing prototype
>   common: add generic CONFIG_UBSAN_SANITIZE_ALL plumbing
>   common: add generic CONFIG_KASAN option
>   sandbox: support Address and UndefinedBehavior sanitizers
>   commands: fix unaligned accesses of aliased commands on amd64
> 
>  Makefile                     |  5 +++++
>  arch/sandbox/Kconfig         |  2 ++
>  arch/sandbox/Makefile        | 13 ++++++++++++-
>  arch/sandbox/board/devices.c |  1 +
>  common/Kconfig               | 34 ++++++++++++++++++++++++++++++++++
>  common/command.c             |  2 +-
>  scripts/Makefile.lib         |  5 +++++
>  scripts/Makefile.ubsan       | 19 +++++++++++++++++++
>  8 files changed, 79 insertions(+), 2 deletions(-)
>  create mode 100644 scripts/Makefile.ubsan
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/5] sandbox: support -fsanitize={address,ubsan}

[Ahmad Fatoum]

On 8/22/19 8:19 AM, Ahmad Fatoum wrote:
> I get a double free inside of SDL's X11 backend when trying to use the
> sandbox SDL video driver on wayland. I am not sure what to do about this
> yet, but I made these few patches to get nicer looking backtraces for a
> start.
> 
> Ahmad Fatoum (5):
>   sandbox: include header to provide missing prototype
>   common: add generic CONFIG_UBSAN_SANITIZE_ALL plumbing
>   common: add generic CONFIG_KASAN option
>   sandbox: support Address and UndefinedBehavior sanitizers
>   commands: fix unaligned accesses of aliased commands on amd64


Please dismiss. I pushed out a broken revision of the series. I will
send out a fixed v2 later and drop the last commit as well.

> 
>  Makefile                     |  5 +++++
>  arch/sandbox/Kconfig         |  2 ++
>  arch/sandbox/Makefile        | 13 ++++++++++++-
>  arch/sandbox/board/devices.c |  1 +
>  common/Kconfig               | 34 ++++++++++++++++++++++++++++++++++
>  common/command.c             |  2 +-
>  scripts/Makefile.lib         |  5 +++++
>  scripts/Makefile.ubsan       | 19 +++++++++++++++++++
>  8 files changed, 79 insertions(+), 2 deletions(-)
>  create mode 100644 scripts/Makefile.ubsan
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox