All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: linux-kernel@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	stable@vger.kernel.org,
	Rasmus Villemoes <linux@rasmusvillemoes.dk>,
	Kees Cook <keescook@chromium.org>
Subject: [PATCH 4.9 25/44] compiler.h: enable builtin overflow checkers and add fallback code
Date: Tue, 29 Jan 2019 12:36:20 +0100	[thread overview]
Message-ID: <20190129113141.953541520@linuxfoundation.org> (raw)
In-Reply-To: <20190129113139.826927690@linuxfoundation.org>

4.9-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Rasmus Villemoes <linux@rasmusvillemoes.dk>

commit f0907827a8a9152aedac2833ed1b674a7b2a44f2 upstream.

This adds wrappers for the __builtin overflow checkers present in gcc
5.1+ as well as fallback implementations for earlier compilers. It's not
that easy to implement the fully generic __builtin_X_overflow(T1 a, T2
b, T3 *d) in macros, so the fallback code assumes that T1, T2 and T3 are
the same. We obviously don't want the wrappers to have different
semantics depending on $GCC_VERSION, so we also insist on that even when
using the builtins.

There are a few problems with the 'a+b < a' idiom for checking for
overflow: For signed types, it relies on undefined behaviour and is
not actually complete (it doesn't check underflow;
e.g. INT_MIN+INT_MIN == 0 isn't caught). Due to type promotion it
is wrong for all types (signed and unsigned) narrower than
int. Similarly, when a and b does not have the same type, there are
subtle cases like

  u32 a;

  if (a + sizeof(foo) < a)
    return -EOVERFLOW;
  a += sizeof(foo);

where the test is always false on 64 bit platforms. Add to that that it
is not always possible to determine the types involved at a glance.

The new overflow.h is somewhat bulky, but that's mostly a result of
trying to be type-generic, complete (e.g. catching not only overflow
but also signed underflow) and not relying on undefined behaviour.

Linus is of course right [1] that for unsigned subtraction a-b, the
right way to check for overflow (underflow) is "b > a" and not
"__builtin_sub_overflow(a, b, &d)", but that's just one out of six cases
covered here, and included mostly for completeness.

So is it worth it? I think it is, if nothing else for the documentation
value of seeing

  if (check_add_overflow(a, b, &d))
    return -EGOAWAY;
  do_stuff_with(d);

instead of the open-coded (and possibly wrong and/or incomplete and/or
UBsan-tickling)

  if (a+b < a)
    return -EGOAWAY;
  do_stuff_with(a+b);

While gcc does recognize the 'a+b < a' idiom for testing unsigned add
overflow, it doesn't do nearly as good for unsigned multiplication
(there's also no single well-established idiom). So using
check_mul_overflow in kcalloc and friends may also make gcc generate
slightly better code.

[1] https://lkml.org/lkml/2015/11/2/658

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 include/linux/compiler-clang.h |   14 ++
 include/linux/compiler-gcc.h   |    4 
 include/linux/compiler-intel.h |    4 
 include/linux/overflow.h       |  205 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 227 insertions(+)

--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -23,3 +23,17 @@
 #ifdef __noretpoline
 #undef __noretpoline
 #endif
+
+/*
+ * Not all versions of clang implement the the type-generic versions
+ * of the builtin overflow checkers. Fortunately, clang implements
+ * __has_builtin allowing us to avoid awkward version
+ * checks. Unfortunately, we don't know which version of gcc clang
+ * pretends to be, so the macro may or may not be defined.
+ */
+#undef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW
+#if __has_builtin(__builtin_mul_overflow) && \
+    __has_builtin(__builtin_add_overflow) && \
+    __has_builtin(__builtin_sub_overflow)
+#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
+#endif
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -334,3 +334,7 @@
  * code
  */
 #define uninitialized_var(x) x = x
+
+#if GCC_VERSION >= 50100
+#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
+#endif
--- a/include/linux/compiler-intel.h
+++ b/include/linux/compiler-intel.h
@@ -43,3 +43,7 @@
 #define __builtin_bswap16 _bswap16
 #endif
 
+/*
+ * icc defines __GNUC__, but does not implement the builtin overflow checkers.
+ */
+#undef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW
--- /dev/null
+++ b/include/linux/overflow.h
@@ -0,0 +1,205 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+#ifndef __LINUX_OVERFLOW_H
+#define __LINUX_OVERFLOW_H
+
+#include <linux/compiler.h>
+
+/*
+ * In the fallback code below, we need to compute the minimum and
+ * maximum values representable in a given type. These macros may also
+ * be useful elsewhere, so we provide them outside the
+ * COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW block.
+ *
+ * It would seem more obvious to do something like
+ *
+ * #define type_min(T) (T)(is_signed_type(T) ? (T)1 << (8*sizeof(T)-1) : 0)
+ * #define type_max(T) (T)(is_signed_type(T) ? ((T)1 << (8*sizeof(T)-1)) - 1 : ~(T)0)
+ *
+ * Unfortunately, the middle expressions, strictly speaking, have
+ * undefined behaviour, and at least some versions of gcc warn about
+ * the type_max expression (but not if -fsanitize=undefined is in
+ * effect; in that case, the warning is deferred to runtime...).
+ *
+ * The slightly excessive casting in type_min is to make sure the
+ * macros also produce sensible values for the exotic type _Bool. [The
+ * overflow checkers only almost work for _Bool, but that's
+ * a-feature-not-a-bug, since people shouldn't be doing arithmetic on
+ * _Bools. Besides, the gcc builtins don't allow _Bool* as third
+ * argument.]
+ *
+ * Idea stolen from
+ * https://mail-index.netbsd.org/tech-misc/2007/02/05/0000.html -
+ * credit to Christian Biere.
+ */
+#define is_signed_type(type)       (((type)(-1)) < (type)1)
+#define __type_half_max(type) ((type)1 << (8*sizeof(type) - 1 - is_signed_type(type)))
+#define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T)))
+#define type_min(T) ((T)((T)-type_max(T)-(T)1))
+
+
+#ifdef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW
+/*
+ * For simplicity and code hygiene, the fallback code below insists on
+ * a, b and *d having the same type (similar to the min() and max()
+ * macros), whereas gcc's type-generic overflow checkers accept
+ * different types. Hence we don't just make check_add_overflow an
+ * alias for __builtin_add_overflow, but add type checks similar to
+ * below.
+ */
+#define check_add_overflow(a, b, d) ({		\
+	typeof(a) __a = (a);			\
+	typeof(b) __b = (b);			\
+	typeof(d) __d = (d);			\
+	(void) (&__a == &__b);			\
+	(void) (&__a == __d);			\
+	__builtin_add_overflow(__a, __b, __d);	\
+})
+
+#define check_sub_overflow(a, b, d) ({		\
+	typeof(a) __a = (a);			\
+	typeof(b) __b = (b);			\
+	typeof(d) __d = (d);			\
+	(void) (&__a == &__b);			\
+	(void) (&__a == __d);			\
+	__builtin_sub_overflow(__a, __b, __d);	\
+})
+
+#define check_mul_overflow(a, b, d) ({		\
+	typeof(a) __a = (a);			\
+	typeof(b) __b = (b);			\
+	typeof(d) __d = (d);			\
+	(void) (&__a == &__b);			\
+	(void) (&__a == __d);			\
+	__builtin_mul_overflow(__a, __b, __d);	\
+})
+
+#else
+
+
+/* Checking for unsigned overflow is relatively easy without causing UB. */
+#define __unsigned_add_overflow(a, b, d) ({	\
+	typeof(a) __a = (a);			\
+	typeof(b) __b = (b);			\
+	typeof(d) __d = (d);			\
+	(void) (&__a == &__b);			\
+	(void) (&__a == __d);			\
+	*__d = __a + __b;			\
+	*__d < __a;				\
+})
+#define __unsigned_sub_overflow(a, b, d) ({	\
+	typeof(a) __a = (a);			\
+	typeof(b) __b = (b);			\
+	typeof(d) __d = (d);			\
+	(void) (&__a == &__b);			\
+	(void) (&__a == __d);			\
+	*__d = __a - __b;			\
+	__a < __b;				\
+})
+/*
+ * If one of a or b is a compile-time constant, this avoids a division.
+ */
+#define __unsigned_mul_overflow(a, b, d) ({		\
+	typeof(a) __a = (a);				\
+	typeof(b) __b = (b);				\
+	typeof(d) __d = (d);				\
+	(void) (&__a == &__b);				\
+	(void) (&__a == __d);				\
+	*__d = __a * __b;				\
+	__builtin_constant_p(__b) ?			\
+	  __b > 0 && __a > type_max(typeof(__a)) / __b : \
+	  __a > 0 && __b > type_max(typeof(__b)) / __a;	 \
+})
+
+/*
+ * For signed types, detecting overflow is much harder, especially if
+ * we want to avoid UB. But the interface of these macros is such that
+ * we must provide a result in *d, and in fact we must produce the
+ * result promised by gcc's builtins, which is simply the possibly
+ * wrapped-around value. Fortunately, we can just formally do the
+ * operations in the widest relevant unsigned type (u64) and then
+ * truncate the result - gcc is smart enough to generate the same code
+ * with and without the (u64) casts.
+ */
+
+/*
+ * Adding two signed integers can overflow only if they have the same
+ * sign, and overflow has happened iff the result has the opposite
+ * sign.
+ */
+#define __signed_add_overflow(a, b, d) ({	\
+	typeof(a) __a = (a);			\
+	typeof(b) __b = (b);			\
+	typeof(d) __d = (d);			\
+	(void) (&__a == &__b);			\
+	(void) (&__a == __d);			\
+	*__d = (u64)__a + (u64)__b;		\
+	(((~(__a ^ __b)) & (*__d ^ __a))	\
+		& type_min(typeof(__a))) != 0;	\
+})
+
+/*
+ * Subtraction is similar, except that overflow can now happen only
+ * when the signs are opposite. In this case, overflow has happened if
+ * the result has the opposite sign of a.
+ */
+#define __signed_sub_overflow(a, b, d) ({	\
+	typeof(a) __a = (a);			\
+	typeof(b) __b = (b);			\
+	typeof(d) __d = (d);			\
+	(void) (&__a == &__b);			\
+	(void) (&__a == __d);			\
+	*__d = (u64)__a - (u64)__b;		\
+	((((__a ^ __b)) & (*__d ^ __a))		\
+		& type_min(typeof(__a))) != 0;	\
+})
+
+/*
+ * Signed multiplication is rather hard. gcc always follows C99, so
+ * division is truncated towards 0. This means that we can write the
+ * overflow check like this:
+ *
+ * (a > 0 && (b > MAX/a || b < MIN/a)) ||
+ * (a < -1 && (b > MIN/a || b < MAX/a) ||
+ * (a == -1 && b == MIN)
+ *
+ * The redundant casts of -1 are to silence an annoying -Wtype-limits
+ * (included in -Wextra) warning: When the type is u8 or u16, the
+ * __b_c_e in check_mul_overflow obviously selects
+ * __unsigned_mul_overflow, but unfortunately gcc still parses this
+ * code and warns about the limited range of __b.
+ */
+
+#define __signed_mul_overflow(a, b, d) ({				\
+	typeof(a) __a = (a);						\
+	typeof(b) __b = (b);						\
+	typeof(d) __d = (d);						\
+	typeof(a) __tmax = type_max(typeof(a));				\
+	typeof(a) __tmin = type_min(typeof(a));				\
+	(void) (&__a == &__b);						\
+	(void) (&__a == __d);						\
+	*__d = (u64)__a * (u64)__b;					\
+	(__b > 0   && (__a > __tmax/__b || __a < __tmin/__b)) ||	\
+	(__b < (typeof(__b))-1  && (__a > __tmin/__b || __a < __tmax/__b)) || \
+	(__b == (typeof(__b))-1 && __a == __tmin);			\
+})
+
+
+#define check_add_overflow(a, b, d)					\
+	__builtin_choose_expr(is_signed_type(typeof(a)),		\
+			__signed_add_overflow(a, b, d),			\
+			__unsigned_add_overflow(a, b, d))
+
+#define check_sub_overflow(a, b, d)					\
+	__builtin_choose_expr(is_signed_type(typeof(a)),		\
+			__signed_sub_overflow(a, b, d),			\
+			__unsigned_sub_overflow(a, b, d))
+
+#define check_mul_overflow(a, b, d)					\
+	__builtin_choose_expr(is_signed_type(typeof(a)),		\
+			__signed_mul_overflow(a, b, d),			\
+			__unsigned_mul_overflow(a, b, d))
+
+
+#endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */
+
+#endif /* __LINUX_OVERFLOW_H */



  parent reply	other threads:[~2019-01-29 11:52 UTC|newest]

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-29 11:35 [PATCH 4.9 00/44] 4.9.154-stable review Greg Kroah-Hartman
2019-01-29 11:35 ` [PATCH 4.9 01/44] net: bridge: Fix ethernet header pointer before check skb forwardable Greg Kroah-Hartman
2019-01-29 11:35   ` [Bridge] " Greg Kroah-Hartman
2019-01-29 11:35 ` [PATCH 4.9 02/44] net: Fix usage of pskb_trim_rcsum Greg Kroah-Hartman
2019-01-29 11:35 ` [PATCH 4.9 03/44] openvswitch: Avoid OOB read when parsing flow nlattrs Greg Kroah-Hartman
2019-01-29 11:35 ` [PATCH 4.9 04/44] vhost: log dirty page correctly Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 05/44] net: ipv4: Fix memory leak in network namespace dismantle Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 06/44] net_sched: refetch skb protocol for each filter Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 07/44] ipfrag: really prevent allocation on netns exit Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 08/44] mmc: Kconfig: Enable CONFIG_MMC_SDHCI_IO_ACCESSORS Greg Kroah-Hartman
2019-01-29 12:54   ` Georgi Djakov
2019-01-29 13:31     ` Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 09/44] USB: serial: simple: add Motorola Tetra TPG2200 device id Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 10/44] USB: serial: pl2303: add new PID to support PL2303TB Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 11/44] ASoC: atom: fix a missing check of snd_pcm_lib_malloc_pages Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 12/44] ASoC: rt5514-spi: Fix potential NULL pointer dereference Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 13/44] ARCv2: lib: memeset: fix doing prefetchw outside of buffer Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 14/44] ARC: perf: map generic branches to correct hardware condition Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 15/44] s390/early: improve machine detection Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 16/44] s390/smp: fix CPU hotplug deadlock with CPU rescan Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 17/44] char/mwave: fix potential Spectre v1 vulnerability Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 18/44] staging: rtl8188eu: Add device code for D-Link DWA-121 rev B1 Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 19/44] tty: Handle problem if line discipline does not have receive_buf Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 20/44] uart: Fix crash in uart_write and uart_put_char Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 21/44] tty/n_hdlc: fix __might_sleep warning Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 22/44] CIFS: Fix possible hang during async MTU reads and writes Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 23/44] CIFS: Do not reconnect TCP session in add_credits() Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 24/44] Input: xpad - add support for SteelSeries Stratus Duo Greg Kroah-Hartman
2019-01-29 11:36 ` Greg Kroah-Hartman [this message]
2019-01-29 11:36 ` [PATCH 4.9 26/44] Input: uinput - fix undefined behavior in uinput_validate_absinfo() Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 27/44] acpi/nfit: Block function zero DSMs Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 28/44] acpi/nfit: Fix command-supported detection Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 29/44] dm thin: fix passdown_double_checking_shared_status() Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 30/44] KVM: x86: Fix single-step debugging Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 31/44] x86/selftests/pkeys: Fork() to check for state being preserved Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 32/44] x86/kaslr: Fix incorrect i8254 outb() parameters Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 33/44] can: dev: __can_get_echo_skb(): fix bogous check for non-existing skb by removing it Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 34/44] can: bcm: check timer values before ktime conversion Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 35/44] vt: invoke notifier on screen size change Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 36/44] perf unwind: Unwind with libdw doesnt take symfs into account Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 37/44] perf unwind: Take pgoff into account when reporting elf to libdwfl Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 38/44] irqchip/gic-v3-its: Align PCI Multi-MSI allocation on their size Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 39/44] s390/smp: Fix calling smp_call_ipl_cpu() from ipl CPU Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 40/44] nvmet-rdma: Add unlikely for response allocated check Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 41/44] nvmet-rdma: fix null dereference under heavy load Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 42/44] f2fs: read page index before freeing Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 43/44] btrfs: fix error handling in btrfs_dev_replace_start Greg Kroah-Hartman
2019-01-29 11:36 ` [PATCH 4.9 44/44] btrfs: dev-replace: go back to suspended state if target device is missing Greg Kroah-Hartman
2019-01-29 19:02 ` [PATCH 4.9 00/44] 4.9.154-stable review Guenter Roeck
2019-01-29 19:26   ` Greg Kroah-Hartman
2019-01-29 19:43     ` Greg Kroah-Hartman
2019-01-30  1:51       ` shuah
2019-01-29 19:43 ` Greg Kroah-Hartman
2019-01-30 12:50 ` Jon Hunter
2019-01-30 12:50   ` Jon Hunter
2019-01-31 10:15   ` Jon Hunter
2019-01-31 10:15     ` Jon Hunter
2019-01-30 13:01 ` Naresh Kamboju
2019-01-30 13:01   ` [LTP] " Naresh Kamboju
2019-01-30 22:12 ` Guenter Roeck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190129113141.953541520@linuxfoundation.org \
    --to=gregkh@linuxfoundation.org \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@rasmusvillemoes.dk \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.