[PATCH rdma-next v4 0/3] overflow.h: Add left-shift helper

[PATCH rdma-next v4 0/3] overflow.h: Add left-shift helper

[Kees Cook]

This adds the left-shift overflow helper, selftests, and first usage.

-Kees

v4:
- actually fix docs typo
- rename to check_shl_overflow()
v3:
- add even more test cases (type mismatches, more signed overflows).
- fix documentation typo on argument name.
v2:
- swap out selftests with framework from Rasmus, add lots more tests.
- drop double-assignment in helper.


Jason Gunthorpe (1):
  overflow.h: Add arithmetic shift helper

Kees Cook (1):
  test_overflow: Add shift overflow tests

Leon Romanovsky (1):
  RDMA/mlx5: Fix shift overflow in mlx5_ib_create_wq

 drivers/infiniband/hw/mlx5/qp.c |   4 +-
 include/linux/overflow.h        |  31 +++++
 lib/test_overflow.c             | 198 +++++++++++++++++++++++++++++++-
 3 files changed, 231 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH rdma-next v4 1/3] overflow.h: Add arithmetic shift helper

[Kees Cook]

From: Jason Gunthorpe <jgg@mellanox.com>

Add shift_overflow() helper to assist driver authors in ensuring that
shift operations don't cause overflows or other odd conditions.

Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
[kees: tweaked comments and commit log, dropped unneeded assignment]
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/overflow.h | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index 8712ff70995f..40b48e2133cb 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -202,6 +202,37 @@
 
 #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */
 
+/** check_shl_overflow() - Calculate a left-shifted value and check overflow
+ *
+ * @a: Value to be shifted
+ * @s: How many bits left to shift
+ * @d: Pointer to where to store the result
+ *
+ * Computes *@d = (@a << @s)
+ *
+ * Returns true if '*d' cannot hold the result or when 'a << s' doesn't
+ * make sense. Example conditions:
+ * - 'a << s' causes bits to be lost when stored in *d.
+ * - 's' is garbage (e.g. negative) or so large that the result of
+ *   'a << s' is guaranteed to be 0.
+ * - 'a' is negative.
+ * - 'a << s' sets the sign bit, if any, in '*d'.
+ *
+ * '*d' will hold the results of the attempted shift, but is not
+ * considered "safe for use" if false is returned.
+ */
+#define check_shl_overflow(a, s, d) ({					\
+	typeof(a) _a = a;						\
+	typeof(s) _s = s;						\
+	typeof(d) _d = d;						\
+	u64 _a_full = _a;						\
+	unsigned int _to_shift =					\
+		_s >= 0 && _s < 8 * sizeof(*d) ? _s : 0;		\
+	*_d = (_a_full << _to_shift);					\
+	(_to_shift != _s || *_d < 0 || _a < 0 ||			\
+		(*_d >> _to_shift) != _a);				\
+})
+
 /**
  * array_size() - Calculate size of 2-dimensional array.
  *
-- 
2.17.1

[PATCH rdma-next v4 2/3] test_overflow: Add shift overflow tests

[Kees Cook]

This adds overflow tests for the new check_shift_overflow() helper to
validate overflow, signedness glitches, storage glitches, etc.

Co-developed-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 lib/test_overflow.c | 198 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 197 insertions(+), 1 deletion(-)

diff --git a/lib/test_overflow.c b/lib/test_overflow.c
index 2278fe05a1b0..fc680562d8b6 100644
--- a/lib/test_overflow.c
+++ b/lib/test_overflow.c
@@ -252,7 +252,8 @@ static int __init test_ ## t ## _overflow(void) {			\
 	int err = 0;							\
 	unsigned i;							\
 									\
-	pr_info("%-3s: %zu tests\n", #t, ARRAY_SIZE(t ## _tests));	\
+	pr_info("%-3s: %zu arithmetic tests\n", #t,			\
+		ARRAY_SIZE(t ## _tests));				\
 	for (i = 0; i < ARRAY_SIZE(t ## _tests); ++i)			\
 		err |= do_test_ ## t(&t ## _tests[i]);			\
 	return err;							\
@@ -287,6 +288,200 @@ static int __init test_overflow_calculation(void)
 	return err;
 }
 
+static int __init test_overflow_shift(void)
+{
+	int err = 0;
+
+/* Args are: value, shift, type, expected result, overflow expected */
+#define TEST_ONE_SHIFT(a, s, t, expect, of) ({				\
+	int __failed = 0;						\
+	typeof(a) __a = (a);						\
+	typeof(s) __s = (s);						\
+	t __e = (expect);						\
+	t __d;								\
+	bool __of = check_shl_overflow(__a, __s, &__d);			\
+	if (__of != of) {						\
+		pr_warn("expected (%s)(%s << %s) to%s overflow\n",	\
+			#t, #a, #s, of ? "" : " not");			\
+		__failed = 1;						\
+	} else if (!__of && __d != __e) {				\
+		pr_warn("expected (%s)(%s << %s) == %s\n",		\
+			#t, #a, #s, #expect);				\
+		if ((t)-1 < 0)						\
+			pr_warn("got %lld\n", (s64)__d);		\
+		else							\
+			pr_warn("got %llu\n", (u64)__d);		\
+		__failed = 1;						\
+	}								\
+	if (!__failed)							\
+		pr_info("ok: (%s)(%s << %s) == %s\n", #t, #a, #s,	\
+			of ? "overflow" : #expect);			\
+	__failed;							\
+})
+
+	/* Sane shifts. */
+	err |= TEST_ONE_SHIFT(1, 0, u8, 1 << 0, false);
+	err |= TEST_ONE_SHIFT(1, 4, u8, 1 << 4, false);
+	err |= TEST_ONE_SHIFT(1, 7, u8, 1 << 7, false);
+	err |= TEST_ONE_SHIFT(0xF, 4, u8, 0xF << 4, false);
+	err |= TEST_ONE_SHIFT(1, 0, u16, 1 << 0, false);
+	err |= TEST_ONE_SHIFT(1, 10, u16, 1 << 10, false);
+	err |= TEST_ONE_SHIFT(1, 15, u16, 1 << 15, false);
+	err |= TEST_ONE_SHIFT(0xFF, 8, u16, 0xFF << 8, false);
+	err |= TEST_ONE_SHIFT(1, 0, int, 1 << 0, false);
+	err |= TEST_ONE_SHIFT(1, 16, int, 1 << 16, false);
+	err |= TEST_ONE_SHIFT(1, 30, int, 1 << 30, false);
+	err |= TEST_ONE_SHIFT(1, 0, s32, 1 << 0, false);
+	err |= TEST_ONE_SHIFT(1, 16, s32, 1 << 16, false);
+	err |= TEST_ONE_SHIFT(1, 30, s32, 1 << 30, false);
+	err |= TEST_ONE_SHIFT(1, 0, unsigned int, 1U << 0, false);
+	err |= TEST_ONE_SHIFT(1, 20, unsigned int, 1U << 20, false);
+	err |= TEST_ONE_SHIFT(1, 31, unsigned int, 1U << 31, false);
+	err |= TEST_ONE_SHIFT(0xFFFFU, 16, unsigned int, 0xFFFFU << 16, false);
+	err |= TEST_ONE_SHIFT(1, 0, u32, 1U << 0, false);
+	err |= TEST_ONE_SHIFT(1, 20, u32, 1U << 20, false);
+	err |= TEST_ONE_SHIFT(1, 31, u32, 1U << 31, false);
+	err |= TEST_ONE_SHIFT(0xFFFFU, 16, u32, 0xFFFFU << 16, false);
+	err |= TEST_ONE_SHIFT(1, 0, u64, 1ULL << 0, false);
+	err |= TEST_ONE_SHIFT(1, 40, u64, 1ULL << 40, false);
+	err |= TEST_ONE_SHIFT(1, 63, u64, 1ULL << 63, false);
+	err |= TEST_ONE_SHIFT(0xFFFFFFFFULL, 32, u64,
+			      0xFFFFFFFFULL << 32, false);
+
+	/* Sane shift: start and end with 0, without a too-wide shift. */
+	err |= TEST_ONE_SHIFT(0, 7, u8, 0, false);
+	err |= TEST_ONE_SHIFT(0, 15, u16, 0, false);
+	err |= TEST_ONE_SHIFT(0, 31, unsigned int, 0, false);
+	err |= TEST_ONE_SHIFT(0, 31, u32, 0, false);
+	err |= TEST_ONE_SHIFT(0, 63, u64, 0, false);
+
+	/* Sane shift: start and end with 0, without reaching signed bit. */
+	err |= TEST_ONE_SHIFT(0, 6, s8, 0, false);
+	err |= TEST_ONE_SHIFT(0, 14, s16, 0, false);
+	err |= TEST_ONE_SHIFT(0, 30, int, 0, false);
+	err |= TEST_ONE_SHIFT(0, 30, s32, 0, false);
+	err |= TEST_ONE_SHIFT(0, 62, s64, 0, false);
+
+	/* Overflow: shifted the bit off the end. */
+	err |= TEST_ONE_SHIFT(1, 8, u8, 0, true);
+	err |= TEST_ONE_SHIFT(1, 16, u16, 0, true);
+	err |= TEST_ONE_SHIFT(1, 32, unsigned int, 0, true);
+	err |= TEST_ONE_SHIFT(1, 32, u32, 0, true);
+	err |= TEST_ONE_SHIFT(1, 64, u64, 0, true);
+
+	/* Overflow: shifted into the signed bit. */
+	err |= TEST_ONE_SHIFT(1, 7, s8, 0, true);
+	err |= TEST_ONE_SHIFT(1, 15, s16, 0, true);
+	err |= TEST_ONE_SHIFT(1, 31, int, 0, true);
+	err |= TEST_ONE_SHIFT(1, 31, s32, 0, true);
+	err |= TEST_ONE_SHIFT(1, 63, s64, 0, true);
+
+	/* Overflow: high bit falls off unsigned types. */
+	/* 10010110 */
+	err |= TEST_ONE_SHIFT(150, 1, u8, 0, true);
+	/* 1000100010010110 */
+	err |= TEST_ONE_SHIFT(34966, 1, u16, 0, true);
+	/* 10000100000010001000100010010110 */
+	err |= TEST_ONE_SHIFT(2215151766U, 1, u32, 0, true);
+	err |= TEST_ONE_SHIFT(2215151766U, 1, unsigned int, 0, true);
+	/* 1000001000010000010000000100000010000100000010001000100010010110 */
+	err |= TEST_ONE_SHIFT(9372061470395238550ULL, 1, u64, 0, true);
+
+	/* Overflow: bit shifted into signed bit on signed types. */
+	/* 01001011 */
+	err |= TEST_ONE_SHIFT(75, 1, s8, 0, true);
+	/* 0100010001001011 */
+	err |= TEST_ONE_SHIFT(17483, 1, s16, 0, true);
+	/* 01000010000001000100010001001011 */
+	err |= TEST_ONE_SHIFT(1107575883, 1, s32, 0, true);
+	err |= TEST_ONE_SHIFT(1107575883, 1, int, 0, true);
+	/* 0100000100001000001000000010000001000010000001000100010001001011 */
+	err |= TEST_ONE_SHIFT(4686030735197619275LL, 1, s64, 0, true);
+
+	/* Overflow: bit shifted past signed bit on signed types. */
+	/* 01001011 */
+	err |= TEST_ONE_SHIFT(75, 2, s8, 0, true);
+	/* 0100010001001011 */
+	err |= TEST_ONE_SHIFT(17483, 2, s16, 0, true);
+	/* 01000010000001000100010001001011 */
+	err |= TEST_ONE_SHIFT(1107575883, 2, s32, 0, true);
+	err |= TEST_ONE_SHIFT(1107575883, 2, int, 0, true);
+	/* 0100000100001000001000000010000001000010000001000100010001001011 */
+	err |= TEST_ONE_SHIFT(4686030735197619275LL, 2, s64, 0, true);
+
+	/* Overflow: values larger than destination type. */
+	err |= TEST_ONE_SHIFT(0x100, 0, u8, 0, true);
+	err |= TEST_ONE_SHIFT(0xFF, 0, s8, 0, true);
+	err |= TEST_ONE_SHIFT(0x10000U, 0, u16, 0, true);
+	err |= TEST_ONE_SHIFT(0xFFFFU, 0, s16, 0, true);
+	err |= TEST_ONE_SHIFT(0x100000000ULL, 0, u32, 0, true);
+	err |= TEST_ONE_SHIFT(0x100000000ULL, 0, unsigned int, 0, true);
+	err |= TEST_ONE_SHIFT(0xFFFFFFFFUL, 0, s32, 0, true);
+	err |= TEST_ONE_SHIFT(0xFFFFFFFFUL, 0, int, 0, true);
+	err |= TEST_ONE_SHIFT(0xFFFFFFFFFFFFFFFFULL, 0, s64, 0, true);
+
+	/* Nonsense: negative initial value. */
+	err |= TEST_ONE_SHIFT(-1, 0, s8, 0, true);
+	err |= TEST_ONE_SHIFT(-1, 0, u8, 0, true);
+	err |= TEST_ONE_SHIFT(-5, 0, s16, 0, true);
+	err |= TEST_ONE_SHIFT(-5, 0, u16, 0, true);
+	err |= TEST_ONE_SHIFT(-10, 0, int, 0, true);
+	err |= TEST_ONE_SHIFT(-10, 0, unsigned int, 0, true);
+	err |= TEST_ONE_SHIFT(-100, 0, s32, 0, true);
+	err |= TEST_ONE_SHIFT(-100, 0, u32, 0, true);
+	err |= TEST_ONE_SHIFT(-10000, 0, s64, 0, true);
+	err |= TEST_ONE_SHIFT(-10000, 0, u64, 0, true);
+
+	/* Nonsense: negative shift values. */
+	err |= TEST_ONE_SHIFT(0, -5, s8, 0, true);
+	err |= TEST_ONE_SHIFT(0, -5, u8, 0, true);
+	err |= TEST_ONE_SHIFT(0, -10, s16, 0, true);
+	err |= TEST_ONE_SHIFT(0, -10, u16, 0, true);
+	err |= TEST_ONE_SHIFT(0, -15, int, 0, true);
+	err |= TEST_ONE_SHIFT(0, -15, unsigned int, 0, true);
+	err |= TEST_ONE_SHIFT(0, -20, s32, 0, true);
+	err |= TEST_ONE_SHIFT(0, -20, u32, 0, true);
+	err |= TEST_ONE_SHIFT(0, -30, s64, 0, true);
+	err |= TEST_ONE_SHIFT(0, -30, u64, 0, true);
+
+	/* Overflow: shifted at or beyond entire type's bit width. */
+	err |= TEST_ONE_SHIFT(0, 8, u8, 0, true);
+	err |= TEST_ONE_SHIFT(0, 9, u8, 0, true);
+	err |= TEST_ONE_SHIFT(0, 8, s8, 0, true);
+	err |= TEST_ONE_SHIFT(0, 9, s8, 0, true);
+	err |= TEST_ONE_SHIFT(0, 16, u16, 0, true);
+	err |= TEST_ONE_SHIFT(0, 17, u16, 0, true);
+	err |= TEST_ONE_SHIFT(0, 16, s16, 0, true);
+	err |= TEST_ONE_SHIFT(0, 17, s16, 0, true);
+	err |= TEST_ONE_SHIFT(0, 32, u32, 0, true);
+	err |= TEST_ONE_SHIFT(0, 33, u32, 0, true);
+	err |= TEST_ONE_SHIFT(0, 32, int, 0, true);
+	err |= TEST_ONE_SHIFT(0, 33, int, 0, true);
+	err |= TEST_ONE_SHIFT(0, 32, s32, 0, true);
+	err |= TEST_ONE_SHIFT(0, 33, s32, 0, true);
+	err |= TEST_ONE_SHIFT(0, 64, u64, 0, true);
+	err |= TEST_ONE_SHIFT(0, 65, u64, 0, true);
+	err |= TEST_ONE_SHIFT(0, 64, s64, 0, true);
+	err |= TEST_ONE_SHIFT(0, 65, s64, 0, true);
+
+	/*
+	 * Corner case: for unsigned types, we fail when we've shifted
+	 * through the entire width of bits. For signed types, we might
+	 * want to match this behavior, but that would mean noticing if
+	 * we shift through all but the signed bit, and this is not
+	 * currently detected (but we'll notice an overflow into the
+	 * signed bit). So, for now, we will test this condition but
+	 * mark it as not expected to overflow.
+	 */
+	err |= TEST_ONE_SHIFT(0, 7, s8, 0, false);
+	err |= TEST_ONE_SHIFT(0, 15, s16, 0, false);
+	err |= TEST_ONE_SHIFT(0, 31, int, 0, false);
+	err |= TEST_ONE_SHIFT(0, 31, s32, 0, false);
+	err |= TEST_ONE_SHIFT(0, 63, s64, 0, false);
+
+	return err;
+}
+
 /*
  * Deal with the various forms of allocator arguments. See comments above
  * the DEFINE_TEST_ALLOC() instances for mapping of the "bits".
@@ -397,6 +592,7 @@ static int __init test_module_init(void)
 	int err = 0;
 
 	err |= test_overflow_calculation();
+	err |= test_overflow_shift();
 	err |= test_overflow_allocation();
 
 	if (err) {
-- 
2.17.1

[PATCH rdma-next v4 3/3] RDMA/mlx5: Fix shift overflow in mlx5_ib_create_wq

[Kees Cook]

From: Leon Romanovsky <leonro@mellanox.com>

[   61.182439] UBSAN: Undefined behaviour in drivers/infiniband/hw/mlx5/qp.c:5366:34
[   61.183673] shift exponent 4294967288 is too large for 32-bit type 'unsigned int'
[   61.185530] CPU: 0 PID: 639 Comm: qp Not tainted 4.18.0-rc1-00037-g4aa1d69a9c60-dirty #96
[   61.186981] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-2.fc27 04/01/2014
[   61.188315] Call Trace:
[   61.188661]  dump_stack+0xc7/0x13b
[   61.190427]  ubsan_epilogue+0x9/0x49
[   61.190899]  __ubsan_handle_shift_out_of_bounds+0x1ea/0x22f
[   61.197040]  mlx5_ib_create_wq+0x1c99/0x1d50
[   61.206632]  ib_uverbs_ex_create_wq+0x499/0x820
[   61.213892]  ib_uverbs_write+0x77e/0xae0
[   61.248018]  vfs_write+0x121/0x3b0
[   61.249831]  ksys_write+0xa1/0x120
[   61.254024]  do_syscall_64+0x7c/0x2a0
[   61.256178]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   61.259211] RIP: 0033:0x7f54bab70e99
[   61.262125] Code: 00 f3 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89
[   61.268678] RSP: 002b:00007ffe1541c318 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[   61.271076] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f54bab70e99
[   61.273795] RDX: 0000000000000070 RSI: 0000000020000240 RDI: 0000000000000003
[   61.276982] RBP: 00007ffe1541c330 R08: 00000000200078e0 R09: 0000000000000002
[   61.280035] R10: 0000000000000000 R11: 0000000000000246 R12: 00000000004005c0
[   61.283279] R13: 00007ffe1541c420 R14: 0000000000000000 R15: 0000000000000000

Cc: <stable@vger.kernel.org> # 4.7
Fixes: 79b20a6c3014 ("IB/mlx5: Add receive Work Queue verbs")
Cc: syzkaller <syzkaller@googlegroups.com>
Reported-by: Noa Osherovich <noaos@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/infiniband/hw/mlx5/qp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
index a4f1f638509f..fa3c315f6d94 100644
--- a/drivers/infiniband/hw/mlx5/qp.c
+++ b/drivers/infiniband/hw/mlx5/qp.c
@@ -5365,7 +5365,9 @@ static int set_user_rq_size(struct mlx5_ib_dev *dev,
 
 	rwq->wqe_count = ucmd->rq_wqe_count;
 	rwq->wqe_shift = ucmd->rq_wqe_shift;
-	rwq->buf_size = (rwq->wqe_count << rwq->wqe_shift);
+	if (check_shl_overflow(rwq->wqe_count, rwq->wqe_shift, &rwq->buf_size))
+		return -EINVAL;
+
 	rwq->log_rq_stride = rwq->wqe_shift;
 	rwq->log_rq_size = ilog2(rwq->wqe_count);
 	return 0;
-- 
2.17.1

Re: [PATCH rdma-next v4 2/3] test_overflow: Add shift overflow tests

[Rasmus Villemoes]

On 2018-08-01 23:25, Kees Cook wrote:
> This adds overflow tests for the new check_shift_overflow() helper to
> validate overflow, signedness glitches, storage glitches, etc.
> 

Just a few random comments, not really anything worth a v5 by itself.
IOW, I can live with this being sent upstream during next merge window.

> Co-developed-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> +static int __init test_overflow_shift(void)
> +{
> +	int err = 0;
> +
> +/* Args are: value, shift, type, expected result, overflow expected */
> +#define TEST_ONE_SHIFT(a, s, t, expect, of) ({				\
> +	int __failed = 0;						\
> +	typeof(a) __a = (a);						\
> +	typeof(s) __s = (s);						\
> +	t __e = (expect);						\
> +	t __d;								\
> +	bool __of = check_shl_overflow(__a, __s, &__d);			\
> +	if (__of != of) {						\
> +		pr_warn("expected (%s)(%s << %s) to%s overflow\n",	\
> +			#t, #a, #s, of ? "" : " not");			\
> +		__failed = 1;						\
> +	} else if (!__of && __d != __e) {				\
> +		pr_warn("expected (%s)(%s << %s) == %s\n",		\
> +			#t, #a, #s, #expect);				\
> +		if ((t)-1 < 0)						\
> +			pr_warn("got %lld\n", (s64)__d);		\
> +		else							\
> +			pr_warn("got %llu\n", (u64)__d);		\
> +		__failed = 1;						\
> +	}								\
> +	if (!__failed)							\
> +		pr_info("ok: (%s)(%s << %s) == %s\n", #t, #a, #s,	\
> +			of ? "overflow" : #expect);			\

Isn't that a bit much, one pr_info for every test case (especially with
the number of test cases you've done, and thanks for that btw.)? For the
others, there's just one "doing nnn tests". It's harder here because we
can only let the tests count themselves, but we could have
TEST_ONE_SHIFT do exactly that and then print a pr_info at the end (or
pr_warn if any failed).

[Or, if we're willing to do something a bit ugly, forward-declaring
file-scope statics is actually allowed, and can be combined with
__COUNTER__...

#define foo() printf("test number %d\n", __COUNTER__)

static int start;
static int end;

static int start = __COUNTER__;
void t(void)
{
	printf("Will do %d tests\n", end - start);
	foo();
	foo();
}
static int end = __COUNTER__ - 1;

the expansion of TEST_ONE_SHIFT wouldn't have to use __COUNTER__ in any
meaningful way. This of course goes out the window if
check_shl_overflow is robustified with UNIQUE_ID. There's even a way to
work around that, but I'd better stop here.]


> +	__failed;							\
> +})
> +
> +	err |= TEST_ONE_SHIFT(1, 0, int, 1 << 0, false);
> +	err |= TEST_ONE_SHIFT(1, 16, int, 1 << 16, false);
> +	err |= TEST_ONE_SHIFT(1, 30, int, 1 << 30, false);
> +	err |= TEST_ONE_SHIFT(1, 0, s32, 1 << 0, false);
> +	err |= TEST_ONE_SHIFT(1, 16, s32, 1 << 16, false);
> +	err |= TEST_ONE_SHIFT(1, 30, s32, 1 << 30, false);

I don't see much point in doing both int and s32 as they are AFAIK
unconditionally the same on all architectures. Similarly for unsigned
int/u32.

> +
> +	/* Overflow: shifted at or beyond entire type's bit width. */
> +	err |= TEST_ONE_SHIFT(0, 8, u8, 0, true);
> +	err |= TEST_ONE_SHIFT(0, 9, u8, 0, true);

Hmm, seeing these I'd actually rather we didn't base the upper bound for
the shift count on sizeof(*d) but rather used a fixed 64, since that's
what we use for the temporary variable, and capping the shift count is
mostly for avoiding UB in the implementation. For any non-zero input
value, a shift count greater than 8 would still report overflow because
of the truncation.

More generally, I think that if the actual C expression

  a << s

doesn't invoke UB (i.e., a is non-negative and s is sane according to
the type of a), no bits are lots during the shift, and the result fits
in *d, we should not report overflow. Hence we should not impose
arbitrary limits on s just because the destination happens to be u8.
That some shift count/destination type combos then happen to guarantee
overflow for all but an input of 0 is just the way the math works.

> +
> +	/*
> +	 * Corner case: for unsigned types, we fail when we've shifted
> +	 * through the entire width of bits. For signed types, we might
> +	 * want to match this behavior, but that would mean noticing if
> +	 * we shift through all but the signed bit, and this is not
> +	 * currently detected (but we'll notice an overflow into the
> +	 * signed bit). So, for now, we will test this condition but
> +	 * mark it as not expected to overflow.
> +	 */
> +	err |= TEST_ONE_SHIFT(0, 7, s8, 0, false);
> +	err |= TEST_ONE_SHIFT(0, 15, s16, 0, false);
> +	err |= TEST_ONE_SHIFT(0, 31, int, 0, false);
> +	err |= TEST_ONE_SHIFT(0, 31, s32, 0, false);
> +	err |= TEST_ONE_SHIFT(0, 63, s64, 0, false);

I can't really see how this is a "corner case". For an s8 destination
and a shift count of, say, 4, there happens to be 8 input values that
won't lead to "overflow". When the shift count is 7, there's 1 input
value that won't lead to "overflow". So?

Rasmus

Re: [PATCH rdma-next v4 0/3] overflow.h: Add left-shift helper

[Jason Gunthorpe]

On Wed, Aug 01, 2018 at 02:25:38PM -0700, Kees Cook wrote:
> This adds the left-shift overflow helper, selftests, and first usage.
> 
> -Kees
> 
> v4:
> - actually fix docs typo
> - rename to check_shl_overflow()
> v3:
> - add even more test cases (type mismatches, more signed overflows).
> - fix documentation typo on argument name.
> v2:
> - swap out selftests with framework from Rasmus, add lots more tests.
> - drop double-assignment in helper.
> 
> 
> Jason Gunthorpe (1):
>   overflow.h: Add arithmetic shift helper
> 
> Kees Cook (1):
>   test_overflow: Add shift overflow tests
> 
> Leon Romanovsky (1):
>   RDMA/mlx5: Fix shift overflow in mlx5_ib_create_wq

Applied to the RDMA for-next tree, thanks

Jason