* [PATCH rdma-next v4 0/3] overflow.h: Add left-shift helper
@ 2018-08-01 21:25 Kees Cook
2018-08-01 21:25 ` [PATCH rdma-next v4 1/3] overflow.h: Add arithmetic shift helper Kees Cook
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Kees Cook @ 2018-08-01 21:25 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Kees Cook, Rasmus Villemoes, Leon Romanovsky, Bart Van Assche,
Doug Ledford, Dan Carpenter, Randy Dunlap, linux-rdma,
linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH rdma-next v4 1/3] overflow.h: Add arithmetic shift helper 2018-08-01 21:25 [PATCH rdma-next v4 0/3] overflow.h: Add left-shift helper Kees Cook @ 2018-08-01 21:25 ` Kees Cook 2018-08-01 21:25 ` [PATCH rdma-next v4 2/3] test_overflow: Add shift overflow tests Kees Cook ` (2 subsequent siblings) 3 siblings, 0 replies; 6+ messages in thread From: Kees Cook @ 2018-08-01 21:25 UTC (permalink / raw) To: Jason Gunthorpe Cc: Kees Cook, Leon Romanovsky, Rasmus Villemoes, Leon Romanovsky, Bart Van Assche, Doug Ledford, Dan Carpenter, Randy Dunlap, linux-rdma, linux-kernel 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH rdma-next v4 2/3] test_overflow: Add shift overflow tests 2018-08-01 21:25 [PATCH rdma-next v4 0/3] overflow.h: Add left-shift helper Kees Cook 2018-08-01 21:25 ` [PATCH rdma-next v4 1/3] overflow.h: Add arithmetic shift helper Kees Cook @ 2018-08-01 21:25 ` Kees Cook 2018-08-06 22:54 ` Rasmus Villemoes 2018-08-01 21:25 ` [PATCH rdma-next v4 3/3] RDMA/mlx5: Fix shift overflow in mlx5_ib_create_wq Kees Cook 2018-08-08 15:53 ` [PATCH rdma-next v4 0/3] overflow.h: Add left-shift helper Jason Gunthorpe 3 siblings, 1 reply; 6+ messages in thread From: Kees Cook @ 2018-08-01 21:25 UTC (permalink / raw) To: Jason Gunthorpe Cc: Kees Cook, Rasmus Villemoes, Leon Romanovsky, Bart Van Assche, Doug Ledford, Dan Carpenter, Randy Dunlap, linux-rdma, linux-kernel 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH rdma-next v4 2/3] test_overflow: Add shift overflow tests 2018-08-01 21:25 ` [PATCH rdma-next v4 2/3] test_overflow: Add shift overflow tests Kees Cook @ 2018-08-06 22:54 ` Rasmus Villemoes 0 siblings, 0 replies; 6+ messages in thread From: Rasmus Villemoes @ 2018-08-06 22:54 UTC (permalink / raw) To: Kees Cook, Jason Gunthorpe Cc: Leon Romanovsky, Bart Van Assche, Doug Ledford, Dan Carpenter, Randy Dunlap, linux-rdma, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH rdma-next v4 3/3] RDMA/mlx5: Fix shift overflow in mlx5_ib_create_wq 2018-08-01 21:25 [PATCH rdma-next v4 0/3] overflow.h: Add left-shift helper Kees Cook 2018-08-01 21:25 ` [PATCH rdma-next v4 1/3] overflow.h: Add arithmetic shift helper Kees Cook 2018-08-01 21:25 ` [PATCH rdma-next v4 2/3] test_overflow: Add shift overflow tests Kees Cook @ 2018-08-01 21:25 ` Kees Cook 2018-08-08 15:53 ` [PATCH rdma-next v4 0/3] overflow.h: Add left-shift helper Jason Gunthorpe 3 siblings, 0 replies; 6+ messages in thread From: Kees Cook @ 2018-08-01 21:25 UTC (permalink / raw) To: Jason Gunthorpe Cc: Kees Cook, Leon Romanovsky, stable, syzkaller, Rasmus Villemoes, Leon Romanovsky, Bart Van Assche, Doug Ledford, Dan Carpenter, Randy Dunlap, linux-rdma, linux-kernel 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH rdma-next v4 0/3] overflow.h: Add left-shift helper 2018-08-01 21:25 [PATCH rdma-next v4 0/3] overflow.h: Add left-shift helper Kees Cook ` (2 preceding siblings ...) 2018-08-01 21:25 ` [PATCH rdma-next v4 3/3] RDMA/mlx5: Fix shift overflow in mlx5_ib_create_wq Kees Cook @ 2018-08-08 15:53 ` Jason Gunthorpe 3 siblings, 0 replies; 6+ messages in thread From: Jason Gunthorpe @ 2018-08-08 15:53 UTC (permalink / raw) To: Kees Cook Cc: Rasmus Villemoes, Leon Romanovsky, Bart Van Assche, Doug Ledford, Dan Carpenter, Randy Dunlap, linux-rdma, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-08-08 15:53 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-01 21:25 [PATCH rdma-next v4 0/3] overflow.h: Add left-shift helper Kees Cook 2018-08-01 21:25 ` [PATCH rdma-next v4 1/3] overflow.h: Add arithmetic shift helper Kees Cook 2018-08-01 21:25 ` [PATCH rdma-next v4 2/3] test_overflow: Add shift overflow tests Kees Cook 2018-08-06 22:54 ` Rasmus Villemoes 2018-08-01 21:25 ` [PATCH rdma-next v4 3/3] RDMA/mlx5: Fix shift overflow in mlx5_ib_create_wq Kees Cook 2018-08-08 15:53 ` [PATCH rdma-next v4 0/3] overflow.h: Add left-shift helper Jason Gunthorpe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).