Re: [RFC PATCH] UBSan: run-time undefined behavior sanity checker

[Sasha Levin]

On 10/20/2014 06:54 AM, Andrey Ryabinin wrote:
> 
> UBSan uses compile-time instrumentation to catch undefined behavior (UB).
> Compiler inserts code that perform certain kinds of
> checks before operations that could cause UB.
> If check fails (i.e. UB detected) __ubsan_handle_* function called.
> to print error message.
> 
> So the most of the work is done by compiler.
> This patch just implements ubsan handlers printing errors.
> 
> GCC supports this since 4.9, however upcoming GCC 5.0 has
> more checkers implemented.
> 
> Different kinds of checks could be enabled via boot parameter:
> ubsan_handle=OEAINVBSLF.
> If ubsan_handle not present in cmdline default options are used: ELNVBSLF
> 
> 	O - different kinds of overflows
> 	E - negation overflow, division overflow, division by zero.
> 	A - misaligned memory access.
> 	I - load from/store to an object with insufficient space.
> 	N - null argument declared with nonnull attribute,
> 	  returned null from function which never returns null, null ptr dereference.
> 	V - variable size array with non-positive length
> 	B - out-of-bounds accesses.
> 	S - shifting out-of-bounds.
> 	L - load of invalid value (value out of range for the enum type, loading other then 0/1 to bool type)
> 	F - call to function through pointer with incorrect function type
> 	    (AFAIK this is not implemented in gcc yet, probably works with clang, though
> 	    	   I didn't check ubsan with clang at all).
> 
> Instrumentation in kernel/printk/printk.c is disabled because struct printk_log is not properly aligned,
> therefore we are recursively taking logbuf_lock while trying to print error in __ubsan_handle*().
> 
> Signed-off-by: Andrey Ryabinin <a.ryabinin@xxxxxxxxxxx>
> ---
>  Makefile                              |  12 +-
>  arch/x86/Kconfig                      |   1 +
>  arch/x86/boot/Makefile                |   1 +
>  arch/x86/boot/compressed/Makefile     |   1 +
>  arch/x86/realmode/rm/Makefile         |   1 +
>  arch/x86/vdso/Makefile                |   2 +
>  drivers/firmware/efi/libstub/Makefile |   1 +
>  include/linux/sched.h                 |   4 +
>  kernel/printk/Makefile                |   1 +
>  lib/Kconfig.debug                     |  23 ++
>  lib/Makefile                          |   3 +
>  lib/ubsan.c                           | 559 ++++++++++++++++++++++++++++++++++
>  lib/ubsan.h                           |  84 +++++
>  scripts/Makefile.lib                  |   6 +
>  14 files changed, 698 insertions(+), 1 deletion(-)
>  create mode 100644 lib/ubsan.c
>  create mode 100644 lib/ubsan.h
> 
> diff --git a/Makefile b/Makefile
> index 05d67af..d3e23f9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -377,6 +377,9 @@ LDFLAGS_MODULE  =
>  CFLAGS_KERNEL	=
>  AFLAGS_KERNEL	=
>  CFLAGS_GCOV	= -fprofile-arcs -ftest-coverage
> +CFLAGS_UBSAN	= $(call cc-option, -fsanitize=undefined) \
> +			$(call cc-option, -fno-sanitize=unreachable) \
> +			$(call cc-option, -fno-sanitize=float-cast-overflow)

What's the reason behind those two -fno-sanitize?

[snip]

> +config HAVE_ARCH_UBSAN_SANTIZE_ALL
> +	bool
> +
> +config UBSAN
> +	bool "Undefined behaviour sanity checker"
> +	help
> +	  This option enables undefined behaviour sanity checker
> +	  Compile-time instrumentataion used to detect various undefined
		       instrumentation
> +	  behaviours in runtime. Different kinds of checks could be enabled
> +	  via boot parameter ubsan_handle (see: Documentation/ubsan.txt).
> +	  (TODO: write docs).
> +
> +config UBSAN_SANITIZE_ALL
> +	bool "Enable instrumentation for the entire kernel"
> +	depends on UBSAN
> +	depends on HAVE_ARCH_UBSAN_SANTIZE_ALL
> +	default y
> +	help
> +	  This option acitivates instrumentation for the entire kernel.
                      activates
> +	  If you don't enable this option, you have to explicitly specify
> +	  UBSAN_SANITIZE := y for the files/directories you want to check for UB.
> +
> +
[snip

> +/* By default enable everything except signed overflows and
> + * misaligned accesses
> + */

Why those two are disabled? Maybe we should be fixing them rather
than ignoring?

> +static unsigned long ubsan_handle = GENMASK(HANDLERS_END, 0) &
> +	~(BIT_MASK(SUM_OVERFLOW) | BIT_MASK(SUB_OVERFLOW) |
> +		BIT_MASK(NEG_OVERFLOW) | BIT_MASK(ALIGNMENT));
> +
> +static void enable_handler(unsigned int handler)
> +{
> +	set_bit(handler, &ubsan_handle);
> +}
> +
> +static bool handler_enabled(unsigned int handler)
> +{
> +	return test_bit(handler, &ubsan_handle);
> +}
> +
> +#define REPORTED_BIT 31
> +#define COLUMN_MASK (~(1U << REPORTED_BIT))
> +
> +static bool is_disabled(struct source_location *location)
> +{
> +	return test_and_set_bit(REPORTED_BIT,
> +				(unsigned long *)&location->column);
> +}
> +
> +static void print_source_location(const char *prefix, struct source_location *loc)
> +{
> +	pr_err("%s %s:%d:%d\n", prefix, loc->file_name,
> +		loc->line, loc->column & COLUMN_MASK);
> +}
> +
> +static bool type_is_int(struct type_descriptor *type)
> +{
> +	return type->type_kind == type_kind_int;
> +}
> +
> +static bool type_is_signed(struct type_descriptor *type)
> +{
> +	return type_is_int(type) && (type->type_info & 1);
> +}
> +
> +static unsigned type_bit_width(struct type_descriptor *type)
> +{
> +	return 1 << (type->type_info >> 1);
> +}
> +
> +static bool is_inline_int(struct type_descriptor *type)
> +{
> +	unsigned inline_bits = sizeof(unsigned long)*8;
> +	unsigned bits = type_bit_width(type);
> +

Shouldn't we check type_is_int() here as well?

> +	return bits <= inline_bits;
> +}

[snip]

> +void __ubsan_handle_negate_overflow(struct overflow_data *data,
> +				unsigned long old_val)
> +{
> +
> +	char old_val_str[60];
> +
> +	if (!handler_enabled(NEG_OVERFLOW))
> +		return;
> +
> +	if (current->in_ubsan)
> +		return;
> +
> +	if (is_disabled(&data->location))
> +		return;

This pattern of 3 if()s is repeating itself couple of times, maybe
it deserves a function of it's own?

> +	ubsan_prologue(&data->location);
> +
> +	val_to_string(old_val_str, sizeof(old_val_str), data->type, old_val);
> +
> +	pr_err("negation of %s cannot be represented in type %s:\n",
> +		old_val_str, data->type->type_name);
> +
> +	ubsan_epilogue();
> +}
> +EXPORT_SYMBOL(__ubsan_handle_negate_overflow);


Thanks,
Sasha

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/