* [PATCH 0/2] overflow: Implement size_t saturating arithmetic helpers
@ 2021-09-20 18:08 Kees Cook
2021-09-20 18:08 ` [PATCH 1/2] " Kees Cook
2021-09-20 18:08 ` [PATCH 2/2] test_overflow: Regularize test reporting output Kees Cook
0 siblings, 2 replies; 12+ messages in thread
From: Kees Cook @ 2021-09-20 18:08 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Kees Cook, Gustavo A. R. Silva, Nathan Chancellor,
Jason Gunthorpe, Nick Desaulniers, Leon Romanovsky, Keith Busch,
Len Baker, linux-kernel, linux-hardening
Hi,
While doing more array_size() scans on the kernel, and reviewing recent
struct_size() work[1], it became clear we needed helpers to perform
composed saturating add and multiplies. This creates those helpers and
updates the self tests to check them.
Thanks,
-Kees
[1] https://lore.kernel.org/lkml/?q=%22open-coded+arithmetic%22
Kees Cook (2):
overflow: Implement size_t saturating arithmetic helpers
test_overflow: Regularize test reporting output
include/linux/overflow.h | 140 ++++++++++++++++++++++++---------------
lib/test_overflow.c | 111 ++++++++++++++++++++++++-------
2 files changed, 175 insertions(+), 76 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] overflow: Implement size_t saturating arithmetic helpers
2021-09-20 18:08 [PATCH 0/2] overflow: Implement size_t saturating arithmetic helpers Kees Cook
@ 2021-09-20 18:08 ` Kees Cook
2021-09-20 22:06 ` Nick Desaulniers
2021-09-21 6:51 ` Rasmus Villemoes
2021-09-20 18:08 ` [PATCH 2/2] test_overflow: Regularize test reporting output Kees Cook
1 sibling, 2 replies; 12+ messages in thread
From: Kees Cook @ 2021-09-20 18:08 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Kees Cook, Gustavo A . R . Silva, Nathan Chancellor,
Jason Gunthorpe, Nick Desaulniers, Leon Romanovsky, Keith Busch,
Len Baker, linux-kernel, linux-hardening
In order to perform more open-coded replacements of common allocation
size arithmetic, the kernel needs a saturating (SIZE_MAX) helper for
addition and multiplication. It is common in allocators, especially on
realloc, to add to an existing size. For example:
p = krealloc(map->patch,
sizeof(struct reg_sequence) * (map->patch_regs + num_regs),
GFP_KERNEL);
There is no existing saturating replacement for this calculation, and
just leaving the addition open coded inside array_size() could
potentially overflow as well. For example, an overflow in an expression
for a size_t argument might wrap to zero:
array_size(anything, something_at_size_max + 1) == 0
Introduce size_mul() and size_add() helpers to perform size_t promotion
and saturated calculations for use in such allocations. With these
helpers it is also possible to redefine array_size(), array3_size(),
flex_array_size(), and struct_size() in terms of the new helpers.
As with the check_*_overflow() helpers, the helpers are wrapped in a
__must_check function that passes through the size_t result. In order
for the helpers to be composed with themselves, they must have unique
variable names to avoid shadowing, so this uses a similar method to what
is already done in minmax.h for constructing unique names to be passed
in as arguments to the statement expression that does the actual work.
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Leon Romanovsky <leon@kernel.org>
Cc: Keith Busch <kbusch@kernel.org>
Cc: Len Baker <len.baker@gmx.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/linux/overflow.h | 140 ++++++++++++++++++++++++---------------
lib/test_overflow.c | 61 +++++++++++++++++
2 files changed, 149 insertions(+), 52 deletions(-)
diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index 4669632bd72b..cd154d47807c 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -117,6 +117,77 @@ static inline bool __must_check __must_check_overflow(bool overflow)
(*_d >> _to_shift) != _a); \
}))
+/*
+ * As with __must_check_overflow() above, this is used to wrap the
+ * size_t output from a statement expression macro.
+ */
+static inline size_t __must_check __must_check_size(size_t size)
+{
+ return size;
+}
+
+/*
+ * Internal logic for size_mul(). Takes variable names from UNIQUE_ID
+ * so that the local variables here will never collide with other local
+ * variables (for example, with itself).
+ */
+#define __size_mul(factor1, factor2, __factor1, __factor2, __product) \
+({ \
+ size_t __product; \
+ size_t __factor1 = (factor1); \
+ size_t __factor2 = (factor2); \
+ if (check_mul_overflow(__factor1, __factor2, &__product)) \
+ __product = SIZE_MAX; \
+ __product; \
+})
+
+/**
+ * size_mul() - Calculate size_t multiplication with saturation at SIZE_MAX
+ *
+ * @factor1: first factor
+ * @factor2: second factor
+ *
+ * Returns: calculate @factor1 * @factor2, where both values are
+ * evaluated as size_t, with any overflow causing the return value to
+ * be SIZE_MAX.
+ */
+#define size_mul(factor1, factor2) \
+ __must_check_size(__size_mul(factor1, factor2, \
+ __UNIQUE_ID(__factor1_), \
+ __UNIQUE_ID(__factor2_), \
+ __UNIQUE_ID(__product_)))
+
+/*
+ * Internal logic for size_add(). Takes variable names from UNIQUE_ID
+ * so that the local variables here will never collide with other local
+ * variables (for example, with itself).
+ */
+#define __size_add(addend1, addend2, __addend1, __addend2, __sum) \
+({ \
+ size_t __sum; \
+ size_t __addend1 = (addend1); \
+ size_t __addend2 = (addend2); \
+ if (check_add_overflow(__addend1, __addend2, &__sum)) \
+ __sum = SIZE_MAX; \
+ __sum; \
+})
+
+/**
+ * size_add() - Calculate size_t addition with saturation at SIZE_MAX
+ *
+ * @addend1: first addend
+ * @addend2: second addend
+ *
+ * Returns: calculate @addend1 + @addend2, where both values are
+ * evaluated as size_t, with any overflow causing the return value to
+ * be SIZE_MAX.
+ */
+#define size_add(addend1, addend2) \
+ __must_check_size(__size_add(addend1, addend2, \
+ __UNIQUE_ID(__addend1_), \
+ __UNIQUE_ID(__addend2_), \
+ __UNIQUE_ID(__sum_)))
+
/**
* array_size() - Calculate size of 2-dimensional array.
*
@@ -128,15 +199,7 @@ static inline bool __must_check __must_check_overflow(bool overflow)
* Returns: number of bytes needed to represent the array or SIZE_MAX on
* overflow.
*/
-static inline __must_check size_t array_size(size_t a, size_t b)
-{
- size_t bytes;
-
- if (check_mul_overflow(a, b, &bytes))
- return SIZE_MAX;
-
- return bytes;
-}
+#define array_size(a, b) size_mul(a, b)
/**
* array3_size() - Calculate size of 3-dimensional array.
@@ -150,65 +213,38 @@ static inline __must_check size_t array_size(size_t a, size_t b)
* Returns: number of bytes needed to represent the array or SIZE_MAX on
* overflow.
*/
-static inline __must_check size_t array3_size(size_t a, size_t b, size_t c)
-{
- size_t bytes;
-
- if (check_mul_overflow(a, b, &bytes))
- return SIZE_MAX;
- if (check_mul_overflow(bytes, c, &bytes))
- return SIZE_MAX;
-
- return bytes;
-}
-
-/*
- * Compute a*b+c, returning SIZE_MAX on overflow. Internal helper for
- * struct_size() below.
- */
-static inline __must_check size_t __ab_c_size(size_t a, size_t b, size_t c)
-{
- size_t bytes;
-
- if (check_mul_overflow(a, b, &bytes))
- return SIZE_MAX;
- if (check_add_overflow(bytes, c, &bytes))
- return SIZE_MAX;
-
- return bytes;
-}
+#define array3_size(a, b, c) size_mul(size_mul(a, b), c)
/**
- * struct_size() - Calculate size of structure with trailing array.
+ * flex_array_size() - Calculate size of a flexible array member
+ * within an enclosing structure.
+ *
* @p: Pointer to the structure.
- * @member: Name of the array member.
+ * @member: Name of the flexible array member.
* @count: Number of elements in the array.
*
- * Calculates size of memory needed for structure @p followed by an
- * array of @count number of @member elements.
+ * Calculates size of a flexible array of @count number of @member
+ * elements, at the end of structure @p.
*
* Return: number of bytes needed or SIZE_MAX on overflow.
*/
-#define struct_size(p, member, count) \
- __ab_c_size(count, \
- sizeof(*(p)->member) + __must_be_array((p)->member),\
- sizeof(*(p)))
+#define flex_array_size(p, member, count) \
+ size_mul(count, \
+ sizeof(*(p)->member) + __must_be_array((p)->member))
/**
- * flex_array_size() - Calculate size of a flexible array member
- * within an enclosing structure.
+ * struct_size() - Calculate size of structure with trailing flexible array.
*
* @p: Pointer to the structure.
- * @member: Name of the flexible array member.
+ * @member: Name of the array member.
* @count: Number of elements in the array.
*
- * Calculates size of a flexible array of @count number of @member
- * elements, at the end of structure @p.
+ * Calculates size of memory needed for structure @p followed by an
+ * array of @count number of @member elements.
*
* Return: number of bytes needed or SIZE_MAX on overflow.
*/
-#define flex_array_size(p, member, count) \
- array_size(count, \
- sizeof(*(p)->member) + __must_be_array((p)->member))
+#define struct_size(p, member, count) \
+ size_add(sizeof(*(p)), flex_array_size(p, member, count))
#endif /* __LINUX_OVERFLOW_H */
diff --git a/lib/test_overflow.c b/lib/test_overflow.c
index 7a4b6f6c5473..01a469ff7ff6 100644
--- a/lib/test_overflow.c
+++ b/lib/test_overflow.c
@@ -588,12 +588,73 @@ static int __init test_overflow_allocation(void)
return err;
}
+struct __test_flex_array {
+ unsigned long flags;
+ size_t count;
+ unsigned long data[];
+};
+
+static int __init test_overflow_size_helpers(void)
+{
+ struct __test_flex_array *obj;
+ int count = 0;
+ int err = 0;
+
+#define check_one_size_helper(expected, func, args...) ({ \
+ bool __failure = false; \
+ size_t _r; \
+ \
+ _r = func(args); \
+ if (_r != (expected)) { \
+ pr_warn("expected " #func "(" #args ") " \
+ "to return %zu but got %zu instead\n", \
+ (size_t)(expected), _r); \
+ __failure = true; \
+ } \
+ count++; \
+ __failure; \
+})
+
+ err |= check_one_size_helper(5, size_add, 2, 3);
+ err |= check_one_size_helper(SIZE_MAX, size_add, SIZE_MAX, 1);
+ err |= check_one_size_helper(SIZE_MAX, size_add, SIZE_MAX, 3);
+ err |= check_one_size_helper(SIZE_MAX, size_add, SIZE_MAX, -3);
+
+ err |= check_one_size_helper(6, size_mul, 2, 3);
+ err |= check_one_size_helper(SIZE_MAX, size_mul, SIZE_MAX, 1);
+ err |= check_one_size_helper(SIZE_MAX, size_mul, SIZE_MAX, 3);
+ err |= check_one_size_helper(SIZE_MAX, size_mul, SIZE_MAX, -3);
+
+ err |= check_one_size_helper(0, flex_array_size, obj, data, 0);
+ err |= check_one_size_helper(sizeof(*obj->data),
+ flex_array_size, obj, data, 1);
+ err |= check_one_size_helper(7 * sizeof(*obj->data),
+ flex_array_size, obj, data, 7);
+ err |= check_one_size_helper(SIZE_MAX,
+ flex_array_size, obj, data, -1);
+ err |= check_one_size_helper(SIZE_MAX,
+ flex_array_size, obj, data, SIZE_MAX - 4);
+
+ err |= check_one_size_helper(sizeof(*obj), struct_size, obj, data, 0);
+ err |= check_one_size_helper(sizeof(*obj) + sizeof(*obj->data),
+ struct_size, obj, data, 1);
+ err |= check_one_size_helper(SIZE_MAX,
+ struct_size, obj, data, -3);
+ err |= check_one_size_helper(SIZE_MAX,
+ struct_size, obj, data, SIZE_MAX - 3);
+
+ pr_info("%d overflow size helper tests finished\n", count);
+
+ return err;
+}
+
static int __init test_module_init(void)
{
int err = 0;
err |= test_overflow_calculation();
err |= test_overflow_shift();
+ err |= test_overflow_size_helpers();
err |= test_overflow_allocation();
if (err) {
--
2.30.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] test_overflow: Regularize test reporting output
2021-09-20 18:08 [PATCH 0/2] overflow: Implement size_t saturating arithmetic helpers Kees Cook
2021-09-20 18:08 ` [PATCH 1/2] " Kees Cook
@ 2021-09-20 18:08 ` Kees Cook
2021-09-20 22:10 ` Nick Desaulniers
1 sibling, 1 reply; 12+ messages in thread
From: Kees Cook @ 2021-09-20 18:08 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Kees Cook, Gustavo A. R. Silva, Nathan Chancellor,
Jason Gunthorpe, Nick Desaulniers, Leon Romanovsky, Keith Busch,
Len Baker, linux-kernel, linux-hardening
Report test run summaries more regularly, so it's easier to understand
the output:
- Remove noisy "ok" reports for shift and allocator tests.
- Reorganize per-type output to the end of each type's tests.
- Replace redundant vmalloc tests with __vmalloc so that __GFP_NO_WARN
can be used to keep the expected failure warnings out of dmesg,
similar to commit 8e060c21ae2c ("lib/test_overflow.c: avoid tainting
the kernel and fix wrap size")
Resulting output:
test_overflow: 18 u8 arithmetic tests finished
test_overflow: 19 s8 arithmetic tests finished
test_overflow: 17 u16 arithmetic tests finished
test_overflow: 17 s16 arithmetic tests finished
test_overflow: 17 u32 arithmetic tests finished
test_overflow: 17 s32 arithmetic tests finished
test_overflow: 17 u64 arithmetic tests finished
test_overflow: 21 s64 arithmetic tests finished
test_overflow: 113 shift tests finished
test_overflow: 17 overflow size helper tests finished
test_overflow: 11 allocation overflow tests finished
test_overflow: all tests passed
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
lib/test_overflow.c | 50 +++++++++++++++++++++++----------------------
1 file changed, 26 insertions(+), 24 deletions(-)
diff --git a/lib/test_overflow.c b/lib/test_overflow.c
index 01a469ff7ff6..e1fd2d72dc61 100644
--- a/lib/test_overflow.c
+++ b/lib/test_overflow.c
@@ -252,10 +252,10 @@ static int __init test_ ## t ## _overflow(void) { \
int err = 0; \
unsigned i; \
\
- 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]); \
+ pr_info("%zu %s arithmetic tests finished\n", \
+ ARRAY_SIZE(t ## _tests), #t); \
return err; \
}
@@ -291,6 +291,7 @@ static int __init test_overflow_calculation(void)
static int __init test_overflow_shift(void)
{
int err = 0;
+ int count = 0;
/* Args are: value, shift, type, expected result, overflow expected */
#define TEST_ONE_SHIFT(a, s, t, expect, of) ({ \
@@ -313,9 +314,7 @@ static int __init test_overflow_shift(void)
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); \
+ count++; \
__failed; \
})
@@ -479,6 +478,8 @@ static int __init test_overflow_shift(void)
err |= TEST_ONE_SHIFT(0, 31, s32, 0, false);
err |= TEST_ONE_SHIFT(0, 63, s64, 0, false);
+ pr_info("%d shift tests finished\n", count);
+
return err;
}
@@ -530,7 +531,6 @@ static int __init test_ ## func (void *arg) \
free ## want_arg (free_func, arg, ptr); \
return 1; \
} \
- pr_info(#func " detected saturation\n"); \
return 0; \
}
@@ -544,10 +544,7 @@ DEFINE_TEST_ALLOC(kmalloc, kfree, 0, 1, 0);
DEFINE_TEST_ALLOC(kmalloc_node, kfree, 0, 1, 1);
DEFINE_TEST_ALLOC(kzalloc, kfree, 0, 1, 0);
DEFINE_TEST_ALLOC(kzalloc_node, kfree, 0, 1, 1);
-DEFINE_TEST_ALLOC(vmalloc, vfree, 0, 0, 0);
-DEFINE_TEST_ALLOC(vmalloc_node, vfree, 0, 0, 1);
-DEFINE_TEST_ALLOC(vzalloc, vfree, 0, 0, 0);
-DEFINE_TEST_ALLOC(vzalloc_node, vfree, 0, 0, 1);
+DEFINE_TEST_ALLOC(__vmalloc, vfree, 0, 1, 0);
DEFINE_TEST_ALLOC(kvmalloc, kvfree, 0, 1, 0);
DEFINE_TEST_ALLOC(kvmalloc_node, kvfree, 0, 1, 1);
DEFINE_TEST_ALLOC(kvzalloc, kvfree, 0, 1, 0);
@@ -559,8 +556,14 @@ static int __init test_overflow_allocation(void)
{
const char device_name[] = "overflow-test";
struct device *dev;
+ int count = 0;
int err = 0;
+#define check_allocation_overflow(alloc) ({ \
+ count++; \
+ test_ ## alloc(dev); \
+})
+
/* Create dummy device for devm_kmalloc()-family tests. */
dev = root_device_register(device_name);
if (IS_ERR(dev)) {
@@ -568,23 +571,22 @@ static int __init test_overflow_allocation(void)
return 1;
}
- err |= test_kmalloc(NULL);
- err |= test_kmalloc_node(NULL);
- err |= test_kzalloc(NULL);
- err |= test_kzalloc_node(NULL);
- err |= test_kvmalloc(NULL);
- err |= test_kvmalloc_node(NULL);
- err |= test_kvzalloc(NULL);
- err |= test_kvzalloc_node(NULL);
- err |= test_vmalloc(NULL);
- err |= test_vmalloc_node(NULL);
- err |= test_vzalloc(NULL);
- err |= test_vzalloc_node(NULL);
- err |= test_devm_kmalloc(dev);
- err |= test_devm_kzalloc(dev);
+ err |= check_allocation_overflow(kmalloc);
+ err |= check_allocation_overflow(kmalloc_node);
+ err |= check_allocation_overflow(kzalloc);
+ err |= check_allocation_overflow(kzalloc_node);
+ err |= check_allocation_overflow(__vmalloc);
+ err |= check_allocation_overflow(kvmalloc);
+ err |= check_allocation_overflow(kvmalloc_node);
+ err |= check_allocation_overflow(kvzalloc);
+ err |= check_allocation_overflow(kvzalloc_node);
+ err |= check_allocation_overflow(devm_kmalloc);
+ err |= check_allocation_overflow(devm_kzalloc);
device_unregister(dev);
+ pr_info("%d allocation overflow tests finished\n", count);
+
return err;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] overflow: Implement size_t saturating arithmetic helpers
2021-09-20 18:08 ` [PATCH 1/2] " Kees Cook
@ 2021-09-20 22:06 ` Nick Desaulniers
2021-09-21 1:38 ` Kees Cook
2021-09-21 6:51 ` Rasmus Villemoes
1 sibling, 1 reply; 12+ messages in thread
From: Nick Desaulniers @ 2021-09-20 22:06 UTC (permalink / raw)
To: Kees Cook
Cc: Rasmus Villemoes, Gustavo A . R . Silva, Nathan Chancellor,
Jason Gunthorpe, Leon Romanovsky, Keith Busch, Len Baker,
linux-kernel, linux-hardening
On Mon, Sep 20, 2021 at 11:09 AM Kees Cook <keescook@chromium.org> wrote:
>
> In order to perform more open-coded replacements of common allocation
> size arithmetic, the kernel needs a saturating (SIZE_MAX) helper for
> addition and multiplication. It is common in allocators, especially on
> realloc, to add to an existing size. For example:
>
> p = krealloc(map->patch,
> sizeof(struct reg_sequence) * (map->patch_regs + num_regs),
> GFP_KERNEL);
>
> There is no existing saturating replacement for this calculation, and
> just leaving the addition open coded inside array_size() could
> potentially overflow as well. For example, an overflow in an expression
> for a size_t argument might wrap to zero:
>
> array_size(anything, something_at_size_max + 1) == 0
>
> Introduce size_mul() and size_add() helpers to perform size_t promotion
> and saturated calculations for use in such allocations. With these
> helpers it is also possible to redefine array_size(), array3_size(),
> flex_array_size(), and struct_size() in terms of the new helpers.
>
> As with the check_*_overflow() helpers, the helpers are wrapped in a
> __must_check function that passes through the size_t result. In order
> for the helpers to be composed with themselves, they must have unique
> variable names to avoid shadowing, so this uses a similar method to what
> is already done in minmax.h for constructing unique names to be passed
> in as arguments to the statement expression that does the actual work.
>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Leon Romanovsky <leon@kernel.org>
> Cc: Keith Busch <kbusch@kernel.org>
> Cc: Len Baker <len.baker@gmx.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> include/linux/overflow.h | 140 ++++++++++++++++++++++++---------------
> lib/test_overflow.c | 61 +++++++++++++++++
> 2 files changed, 149 insertions(+), 52 deletions(-)
>
> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> index 4669632bd72b..cd154d47807c 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -117,6 +117,77 @@ static inline bool __must_check __must_check_overflow(bool overflow)
> (*_d >> _to_shift) != _a); \
> }))
>
> +/*
> + * As with __must_check_overflow() above, this is used to wrap the
> + * size_t output from a statement expression macro.
> + */
> +static inline size_t __must_check __must_check_size(size_t size)
> +{
> + return size;
> +}
> +
> +/*
> + * Internal logic for size_mul(). Takes variable names from UNIQUE_ID
> + * so that the local variables here will never collide with other local
> + * variables (for example, with itself).
> + */
> +#define __size_mul(factor1, factor2, __factor1, __factor2, __product) \
> +({ \
> + size_t __product; \
> + size_t __factor1 = (factor1); \
> + size_t __factor2 = (factor2); \
> + if (check_mul_overflow(__factor1, __factor2, &__product)) \
> + __product = SIZE_MAX; \
> + __product; \
> +})
> +
> +/**
> + * size_mul() - Calculate size_t multiplication with saturation at SIZE_MAX
> + *
> + * @factor1: first factor
> + * @factor2: second factor
> + *
> + * Returns: calculate @factor1 * @factor2, where both values are
> + * evaluated as size_t, with any overflow causing the return value to
> + * be SIZE_MAX.
> + */
> +#define size_mul(factor1, factor2) \
> + __must_check_size(__size_mul(factor1, factor2, \
> + __UNIQUE_ID(__factor1_), \
> + __UNIQUE_ID(__factor2_), \
> + __UNIQUE_ID(__product_)))
> +
> +/*
> + * Internal logic for size_add(). Takes variable names from UNIQUE_ID
> + * so that the local variables here will never collide with other local
> + * variables (for example, with itself).
> + */
> +#define __size_add(addend1, addend2, __addend1, __addend2, __sum) \
> +({ \
> + size_t __sum; \
> + size_t __addend1 = (addend1); \
> + size_t __addend2 = (addend2); \
> + if (check_add_overflow(__addend1, __addend2, &__sum)) \
> + __sum = SIZE_MAX; \
> + __sum; \
> +})
> +
> +/**
> + * size_add() - Calculate size_t addition with saturation at SIZE_MAX
> + *
> + * @addend1: first addend
> + * @addend2: second addend
> + *
> + * Returns: calculate @addend1 + @addend2, where both values are
> + * evaluated as size_t, with any overflow causing the return value to
> + * be SIZE_MAX.
> + */
> +#define size_add(addend1, addend2) \
> + __must_check_size(__size_add(addend1, addend2, \
> + __UNIQUE_ID(__addend1_), \
> + __UNIQUE_ID(__addend2_), \
> + __UNIQUE_ID(__sum_)))
Is the use of __UNIQUE_ID really necessary? Is the point to avoid some
kind of variable shadowing? (As opposed to just using names for the
new variables in the scope of the statement expressions? ie.
+#define __size_add(addend1, addend2, __sum) \
+({ \
+ size_t __sum; \
+ if (check_add_overflow((size_t)__addend1, (size_t)__addend2,
&__sum)) \
+ __sum = SIZE_MAX; \
+ __sum; \
+})
Do the double-underscore-prefixed really need to be a separate
#define, or can their definitions be inlined into the expansion sites;
there seems like there's no other users of the
double-underscore-prefixed versions otherwise. ie.
#define size_add(addend1, addend2) \
__must_check_size(({ \
size_t sum; \
if (check_add_overflow((size_t)addend1, (size_t)addend2), ∑ \
sum = SIZE_MAX; \
sum; \
})
> +
> /**
> * array_size() - Calculate size of 2-dimensional array.
> *
> @@ -128,15 +199,7 @@ static inline bool __must_check __must_check_overflow(bool overflow)
> * Returns: number of bytes needed to represent the array or SIZE_MAX on
> * overflow.
> */
> -static inline __must_check size_t array_size(size_t a, size_t b)
> -{
> - size_t bytes;
> -
> - if (check_mul_overflow(a, b, &bytes))
> - return SIZE_MAX;
> -
> - return bytes;
> -}
> +#define array_size(a, b) size_mul(a, b)
>
> /**
> * array3_size() - Calculate size of 3-dimensional array.
> @@ -150,65 +213,38 @@ static inline __must_check size_t array_size(size_t a, size_t b)
> * Returns: number of bytes needed to represent the array or SIZE_MAX on
> * overflow.
> */
> -static inline __must_check size_t array3_size(size_t a, size_t b, size_t c)
> -{
> - size_t bytes;
> -
> - if (check_mul_overflow(a, b, &bytes))
> - return SIZE_MAX;
> - if (check_mul_overflow(bytes, c, &bytes))
> - return SIZE_MAX;
> -
> - return bytes;
> -}
> -
> -/*
> - * Compute a*b+c, returning SIZE_MAX on overflow. Internal helper for
> - * struct_size() below.
> - */
> -static inline __must_check size_t __ab_c_size(size_t a, size_t b, size_t c)
> -{
> - size_t bytes;
> -
> - if (check_mul_overflow(a, b, &bytes))
> - return SIZE_MAX;
> - if (check_add_overflow(bytes, c, &bytes))
> - return SIZE_MAX;
> -
> - return bytes;
> -}
> +#define array3_size(a, b, c) size_mul(size_mul(a, b), c)
>
> /**
> - * struct_size() - Calculate size of structure with trailing array.
> + * flex_array_size() - Calculate size of a flexible array member
> + * within an enclosing structure.
> + *
> * @p: Pointer to the structure.
> - * @member: Name of the array member.
> + * @member: Name of the flexible array member.
> * @count: Number of elements in the array.
> *
> - * Calculates size of memory needed for structure @p followed by an
> - * array of @count number of @member elements.
> + * Calculates size of a flexible array of @count number of @member
> + * elements, at the end of structure @p.
> *
> * Return: number of bytes needed or SIZE_MAX on overflow.
> */
> -#define struct_size(p, member, count) \
> - __ab_c_size(count, \
> - sizeof(*(p)->member) + __must_be_array((p)->member),\
> - sizeof(*(p)))
> +#define flex_array_size(p, member, count) \
> + size_mul(count, \
> + sizeof(*(p)->member) + __must_be_array((p)->member))
>
> /**
> - * flex_array_size() - Calculate size of a flexible array member
> - * within an enclosing structure.
> + * struct_size() - Calculate size of structure with trailing flexible array.
> *
> * @p: Pointer to the structure.
> - * @member: Name of the flexible array member.
> + * @member: Name of the array member.
> * @count: Number of elements in the array.
> *
> - * Calculates size of a flexible array of @count number of @member
> - * elements, at the end of structure @p.
> + * Calculates size of memory needed for structure @p followed by an
> + * array of @count number of @member elements.
> *
> * Return: number of bytes needed or SIZE_MAX on overflow.
> */
> -#define flex_array_size(p, member, count) \
> - array_size(count, \
> - sizeof(*(p)->member) + __must_be_array((p)->member))
> +#define struct_size(p, member, count) \
> + size_add(sizeof(*(p)), flex_array_size(p, member, count))
>
> #endif /* __LINUX_OVERFLOW_H */
> diff --git a/lib/test_overflow.c b/lib/test_overflow.c
> index 7a4b6f6c5473..01a469ff7ff6 100644
> --- a/lib/test_overflow.c
> +++ b/lib/test_overflow.c
> @@ -588,12 +588,73 @@ static int __init test_overflow_allocation(void)
> return err;
> }
>
> +struct __test_flex_array {
> + unsigned long flags;
> + size_t count;
> + unsigned long data[];
> +};
> +
> +static int __init test_overflow_size_helpers(void)
> +{
> + struct __test_flex_array *obj;
> + int count = 0;
> + int err = 0;
> +
> +#define check_one_size_helper(expected, func, args...) ({ \
> + bool __failure = false; \
> + size_t _r; \
> + \
> + _r = func(args); \
> + if (_r != (expected)) { \
> + pr_warn("expected " #func "(" #args ") " \
> + "to return %zu but got %zu instead\n", \
> + (size_t)(expected), _r); \
> + __failure = true; \
> + } \
> + count++; \
> + __failure; \
> +})
> +
> + err |= check_one_size_helper(5, size_add, 2, 3);
> + err |= check_one_size_helper(SIZE_MAX, size_add, SIZE_MAX, 1);
> + err |= check_one_size_helper(SIZE_MAX, size_add, SIZE_MAX, 3);
> + err |= check_one_size_helper(SIZE_MAX, size_add, SIZE_MAX, -3);
Sorry, is this ^ case saying that we expect SIZE_MAX + -3 == SIZE_MAX?
This is because the helpers performed unsigned arithmetic on size_t?
> +
> + err |= check_one_size_helper(6, size_mul, 2, 3);
> + err |= check_one_size_helper(SIZE_MAX, size_mul, SIZE_MAX, 1);
> + err |= check_one_size_helper(SIZE_MAX, size_mul, SIZE_MAX, 3);
> + err |= check_one_size_helper(SIZE_MAX, size_mul, SIZE_MAX, -3);
> +
> + err |= check_one_size_helper(0, flex_array_size, obj, data, 0);
> + err |= check_one_size_helper(sizeof(*obj->data),
> + flex_array_size, obj, data, 1);
> + err |= check_one_size_helper(7 * sizeof(*obj->data),
> + flex_array_size, obj, data, 7);
> + err |= check_one_size_helper(SIZE_MAX,
> + flex_array_size, obj, data, -1);
> + err |= check_one_size_helper(SIZE_MAX,
> + flex_array_size, obj, data, SIZE_MAX - 4);
> +
> + err |= check_one_size_helper(sizeof(*obj), struct_size, obj, data, 0);
> + err |= check_one_size_helper(sizeof(*obj) + sizeof(*obj->data),
> + struct_size, obj, data, 1);
> + err |= check_one_size_helper(SIZE_MAX,
> + struct_size, obj, data, -3);
> + err |= check_one_size_helper(SIZE_MAX,
> + struct_size, obj, data, SIZE_MAX - 3);
> +
> + pr_info("%d overflow size helper tests finished\n", count);
> +
> + return err;
> +}
> +
> static int __init test_module_init(void)
> {
> int err = 0;
>
> err |= test_overflow_calculation();
> err |= test_overflow_shift();
> + err |= test_overflow_size_helpers();
> err |= test_overflow_allocation();
>
> if (err) {
> --
> 2.30.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] test_overflow: Regularize test reporting output
2021-09-20 18:08 ` [PATCH 2/2] test_overflow: Regularize test reporting output Kees Cook
@ 2021-09-20 22:10 ` Nick Desaulniers
2021-09-21 6:56 ` Rasmus Villemoes
0 siblings, 1 reply; 12+ messages in thread
From: Nick Desaulniers @ 2021-09-20 22:10 UTC (permalink / raw)
To: Kees Cook
Cc: Rasmus Villemoes, Gustavo A. R. Silva, Nathan Chancellor,
Jason Gunthorpe, Leon Romanovsky, Keith Busch, Len Baker,
linux-kernel, linux-hardening
On Mon, Sep 20, 2021 at 11:09 AM Kees Cook <keescook@chromium.org> wrote:
>
> Report test run summaries more regularly, so it's easier to understand
> the output:
> - Remove noisy "ok" reports for shift and allocator tests.
> - Reorganize per-type output to the end of each type's tests.
> - Replace redundant vmalloc tests with __vmalloc so that __GFP_NO_WARN
> can be used to keep the expected failure warnings out of dmesg,
> similar to commit 8e060c21ae2c ("lib/test_overflow.c: avoid tainting
> the kernel and fix wrap size")
>
> Resulting output:
>
> test_overflow: 18 u8 arithmetic tests finished
> test_overflow: 19 s8 arithmetic tests finished
> test_overflow: 17 u16 arithmetic tests finished
> test_overflow: 17 s16 arithmetic tests finished
> test_overflow: 17 u32 arithmetic tests finished
> test_overflow: 17 s32 arithmetic tests finished
> test_overflow: 17 u64 arithmetic tests finished
> test_overflow: 21 s64 arithmetic tests finished
> test_overflow: 113 shift tests finished
> test_overflow: 17 overflow size helper tests finished
> test_overflow: 11 allocation overflow tests finished
> test_overflow: all tests passed
>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Kees Cook <keescook@chromium.org>
Much appreciated!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> lib/test_overflow.c | 50 +++++++++++++++++++++++----------------------
> 1 file changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/lib/test_overflow.c b/lib/test_overflow.c
> index 01a469ff7ff6..e1fd2d72dc61 100644
> --- a/lib/test_overflow.c
> +++ b/lib/test_overflow.c
> @@ -252,10 +252,10 @@ static int __init test_ ## t ## _overflow(void) { \
> int err = 0; \
> unsigned i; \
> \
> - 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]); \
> + pr_info("%zu %s arithmetic tests finished\n", \
> + ARRAY_SIZE(t ## _tests), #t); \
> return err; \
> }
>
> @@ -291,6 +291,7 @@ static int __init test_overflow_calculation(void)
> static int __init test_overflow_shift(void)
> {
> int err = 0;
> + int count = 0;
>
> /* Args are: value, shift, type, expected result, overflow expected */
> #define TEST_ONE_SHIFT(a, s, t, expect, of) ({ \
> @@ -313,9 +314,7 @@ static int __init test_overflow_shift(void)
> 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); \
> + count++; \
> __failed; \
> })
>
> @@ -479,6 +478,8 @@ static int __init test_overflow_shift(void)
> err |= TEST_ONE_SHIFT(0, 31, s32, 0, false);
> err |= TEST_ONE_SHIFT(0, 63, s64, 0, false);
>
> + pr_info("%d shift tests finished\n", count);
> +
> return err;
> }
>
> @@ -530,7 +531,6 @@ static int __init test_ ## func (void *arg) \
> free ## want_arg (free_func, arg, ptr); \
> return 1; \
> } \
> - pr_info(#func " detected saturation\n"); \
> return 0; \
> }
>
> @@ -544,10 +544,7 @@ DEFINE_TEST_ALLOC(kmalloc, kfree, 0, 1, 0);
> DEFINE_TEST_ALLOC(kmalloc_node, kfree, 0, 1, 1);
> DEFINE_TEST_ALLOC(kzalloc, kfree, 0, 1, 0);
> DEFINE_TEST_ALLOC(kzalloc_node, kfree, 0, 1, 1);
> -DEFINE_TEST_ALLOC(vmalloc, vfree, 0, 0, 0);
> -DEFINE_TEST_ALLOC(vmalloc_node, vfree, 0, 0, 1);
> -DEFINE_TEST_ALLOC(vzalloc, vfree, 0, 0, 0);
> -DEFINE_TEST_ALLOC(vzalloc_node, vfree, 0, 0, 1);
> +DEFINE_TEST_ALLOC(__vmalloc, vfree, 0, 1, 0);
> DEFINE_TEST_ALLOC(kvmalloc, kvfree, 0, 1, 0);
> DEFINE_TEST_ALLOC(kvmalloc_node, kvfree, 0, 1, 1);
> DEFINE_TEST_ALLOC(kvzalloc, kvfree, 0, 1, 0);
> @@ -559,8 +556,14 @@ static int __init test_overflow_allocation(void)
> {
> const char device_name[] = "overflow-test";
> struct device *dev;
> + int count = 0;
> int err = 0;
>
> +#define check_allocation_overflow(alloc) ({ \
> + count++; \
> + test_ ## alloc(dev); \
> +})
> +
> /* Create dummy device for devm_kmalloc()-family tests. */
> dev = root_device_register(device_name);
> if (IS_ERR(dev)) {
> @@ -568,23 +571,22 @@ static int __init test_overflow_allocation(void)
> return 1;
> }
>
> - err |= test_kmalloc(NULL);
> - err |= test_kmalloc_node(NULL);
> - err |= test_kzalloc(NULL);
> - err |= test_kzalloc_node(NULL);
> - err |= test_kvmalloc(NULL);
> - err |= test_kvmalloc_node(NULL);
> - err |= test_kvzalloc(NULL);
> - err |= test_kvzalloc_node(NULL);
> - err |= test_vmalloc(NULL);
> - err |= test_vmalloc_node(NULL);
> - err |= test_vzalloc(NULL);
> - err |= test_vzalloc_node(NULL);
> - err |= test_devm_kmalloc(dev);
> - err |= test_devm_kzalloc(dev);
> + err |= check_allocation_overflow(kmalloc);
> + err |= check_allocation_overflow(kmalloc_node);
> + err |= check_allocation_overflow(kzalloc);
> + err |= check_allocation_overflow(kzalloc_node);
> + err |= check_allocation_overflow(__vmalloc);
> + err |= check_allocation_overflow(kvmalloc);
> + err |= check_allocation_overflow(kvmalloc_node);
> + err |= check_allocation_overflow(kvzalloc);
> + err |= check_allocation_overflow(kvzalloc_node);
> + err |= check_allocation_overflow(devm_kmalloc);
> + err |= check_allocation_overflow(devm_kzalloc);
>
> device_unregister(dev);
>
> + pr_info("%d allocation overflow tests finished\n", count);
> +
> return err;
> }
>
> --
> 2.30.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] overflow: Implement size_t saturating arithmetic helpers
2021-09-20 22:06 ` Nick Desaulniers
@ 2021-09-21 1:38 ` Kees Cook
0 siblings, 0 replies; 12+ messages in thread
From: Kees Cook @ 2021-09-21 1:38 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Rasmus Villemoes, Gustavo A . R . Silva, Nathan Chancellor,
Jason Gunthorpe, Leon Romanovsky, Keith Busch, Len Baker,
linux-kernel, linux-hardening
On Mon, Sep 20, 2021 at 03:06:33PM -0700, Nick Desaulniers wrote:
> On Mon, Sep 20, 2021 at 11:09 AM Kees Cook <keescook@chromium.org> wrote:
> > [...]
> > +/*
> > + * Internal logic for size_add(). Takes variable names from UNIQUE_ID
> > + * so that the local variables here will never collide with other local
> > + * variables (for example, with itself).
> > + */
> > +#define __size_add(addend1, addend2, __addend1, __addend2, __sum) \
> > +({ \
> > + size_t __sum; \
> > + size_t __addend1 = (addend1); \
> > + size_t __addend2 = (addend2); \
> > + if (check_add_overflow(__addend1, __addend2, &__sum)) \
> > + __sum = SIZE_MAX; \
> > + __sum; \
> > +})
> > +
> > +/**
> > + * size_add() - Calculate size_t addition with saturation at SIZE_MAX
> > + *
> > + * @addend1: first addend
> > + * @addend2: second addend
> > + *
> > + * Returns: calculate @addend1 + @addend2, where both values are
> > + * evaluated as size_t, with any overflow causing the return value to
> > + * be SIZE_MAX.
> > + */
> > +#define size_add(addend1, addend2) \
> > + __must_check_size(__size_add(addend1, addend2, \
> > + __UNIQUE_ID(__addend1_), \
> > + __UNIQUE_ID(__addend2_), \
> > + __UNIQUE_ID(__sum_)))
>
> Is the use of __UNIQUE_ID really necessary? Is the point to avoid some
> kind of variable shadowing? (As opposed to just using names for the
> new variables in the scope of the statement expressions? ie.
Yes, when composed[1], they would shadow (under -Wshadow). I'd rather
not knowingly add yet more[2] shadowed variables to the kernel. :)
[1] https://godbolt.org/z/1rM6Ko1j3
[2] https://github.com/KSPP/linux/issues/152
> +#define __size_add(addend1, addend2, __sum) \
> +({ \
> + size_t __sum; \
> + if (check_add_overflow((size_t)__addend1, (size_t)__addend2,
> &__sum)) \
> + __sum = SIZE_MAX; \
> + __sum; \
> +})
>
> Do the double-underscore-prefixed really need to be a separate
> #define, or can their definitions be inlined into the expansion sites;
> there seems like there's no other users of the
> double-underscore-prefixed versions otherwise. ie.
>
> #define size_add(addend1, addend2) \
> __must_check_size(({ \
> size_t sum; \
> if (check_add_overflow((size_t)addend1, (size_t)addend2), ∑ \
> sum = SIZE_MAX; \
> sum; \
> })
Right, there aren't, but that's the way to pass such variable names in.
(See minmax.h.) This also leaves the door open for using these helpers
as constant expressions, if the need arises.
> > + err |= check_one_size_helper(SIZE_MAX, size_add, SIZE_MAX, -3);
>
> Sorry, is this ^ case saying that we expect SIZE_MAX + -3 == SIZE_MAX?
> This is because the helpers performed unsigned arithmetic on size_t?
Correct. When I wrote this I hadn't yet found any cases of needing to
shrink an allocation size that followed a common pattern like this. But it
turns out we do have some:
drivers/infiniband/core/sa_query.c: sa_dev = kzalloc(struct_size(sa_dev, port, e - s + 1), GFP_KERNEL);
drivers/infiniband/core/user_mad.c: umad_dev = kzalloc(struct_size(umad_dev, ports, e - s + 1), GFP_KERNEL);
drivers/net/ethernet/intel/iavf/iavf_virtchnl.c: len = struct_size(vti, list, adapter->num_tc - 1);
I'll need to add size_sub() as well.
Thanks for looking this over!
--
Kees Cook
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] overflow: Implement size_t saturating arithmetic helpers
2021-09-20 18:08 ` [PATCH 1/2] " Kees Cook
2021-09-20 22:06 ` Nick Desaulniers
@ 2021-09-21 6:51 ` Rasmus Villemoes
2021-09-21 19:07 ` Kees Cook
2022-01-24 21:13 ` Kees Cook
1 sibling, 2 replies; 12+ messages in thread
From: Rasmus Villemoes @ 2021-09-21 6:51 UTC (permalink / raw)
To: Kees Cook
Cc: Gustavo A . R . Silva, Nathan Chancellor, Jason Gunthorpe,
Nick Desaulniers, Leon Romanovsky, Keith Busch, Len Baker,
linux-kernel, linux-hardening
On 20/09/2021 20.08, Kees Cook wrote:
> + * Internal logic for size_mul(). Takes variable names from UNIQUE_ID
> + * so that the local variables here will never collide with other local
> + * variables (for example, with itself).
> + */
> +#define __size_mul(factor1, factor2, __factor1, __factor2, __product) \
> +({ \
> + size_t __product; \
> + size_t __factor1 = (factor1); \
> + size_t __factor2 = (factor2); \
> + if (check_mul_overflow(__factor1, __factor2, &__product)) \
> + __product = SIZE_MAX; \
> + __product; \
> +})
> +
Why can't this just be a static inline taking and returning size_ts,
avoiding all the unique_id ritual and triple layers of macros?
> -static inline __must_check size_t array_size(size_t a, size_t b)
> -{
> - size_t bytes;
> -
> - if (check_mul_overflow(a, b, &bytes))
> - return SIZE_MAX;
> -
> - return bytes;
> -}
> +#define array_size(a, b) size_mul(a, b)
For example, it could be the very function that you remove here and then
add a compat alias. IDGI, if you want that functionality by another
name, just rename array_size, or let size_mul be a #define for array_size.
And we don't have a size_add, but that could just as well be a static
inline that has the __must_check itself.
Not that I can see that the __must_check matters much for these anyway;
if anybody does
size_mul(foo, bar);
that's just a statement with no side effects, so probably the compiler
would warn anyway, or at least nobody can then go on to do anything
"wrong". Unlike the check_*_overflow(), which have the (possibly
wrapped) result in a output-pointer and the "did it overflow" as the
return value, so you can do
check_mul_overflow(a, b, &d);
do_stuff_with(d);
were it not for the __must_check wrapper.
[Reminder: __must_check is a bit of a misnomer, the attribute is really
warn_unused_result, and there's no requirement that the result is part
of the controlling expression of an if() or while() - just passing the
result on directly to some other function counts as a "use", which is
indeed what we do with the size wrappers.]
Rasmus
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] test_overflow: Regularize test reporting output
2021-09-20 22:10 ` Nick Desaulniers
@ 2021-09-21 6:56 ` Rasmus Villemoes
0 siblings, 0 replies; 12+ messages in thread
From: Rasmus Villemoes @ 2021-09-21 6:56 UTC (permalink / raw)
To: Nick Desaulniers, Kees Cook
Cc: Gustavo A. R. Silva, Nathan Chancellor, Jason Gunthorpe,
Leon Romanovsky, Keith Busch, Len Baker, linux-kernel,
linux-hardening
On 21/09/2021 00.10, Nick Desaulniers wrote:
> On Mon, Sep 20, 2021 at 11:09 AM Kees Cook <keescook@chromium.org> wrote:
>>
>> @@ -544,10 +544,7 @@ DEFINE_TEST_ALLOC(kmalloc, kfree, 0, 1, 0);
>> DEFINE_TEST_ALLOC(kmalloc_node, kfree, 0, 1, 1);
>> DEFINE_TEST_ALLOC(kzalloc, kfree, 0, 1, 0);
>> DEFINE_TEST_ALLOC(kzalloc_node, kfree, 0, 1, 1);
>> -DEFINE_TEST_ALLOC(vmalloc, vfree, 0, 0, 0);
>> -DEFINE_TEST_ALLOC(vmalloc_node, vfree, 0, 0, 1);
>> -DEFINE_TEST_ALLOC(vzalloc, vfree, 0, 0, 0);
>> -DEFINE_TEST_ALLOC(vzalloc_node, vfree, 0, 0, 1);
>> +DEFINE_TEST_ALLOC(__vmalloc, vfree, 0, 1, 0);
>> DEFINE_TEST_ALLOC(kvmalloc, kvfree, 0, 1, 0);
>> DEFINE_TEST_ALLOC(kvmalloc_node, kvfree, 0, 1, 1);
>> DEFINE_TEST_ALLOC(kvzalloc, kvfree, 0, 1, 0);
>> @@ -559,8 +556,14 @@ static int __init test_overflow_allocation(void)
>> {
>> const char device_name[] = "overflow-test";
>> struct device *dev;
>> + int count = 0;
>> int err = 0;
>>
>> +#define check_allocation_overflow(alloc) ({ \
>> + count++; \
>> + test_ ## alloc(dev); \
>> +})
>> +
>> /* Create dummy device for devm_kmalloc()-family tests. */
>> dev = root_device_register(device_name);
>> if (IS_ERR(dev)) {
>> @@ -568,23 +571,22 @@ static int __init test_overflow_allocation(void)
>> return 1;
>> }
>>
>> - err |= test_kmalloc(NULL);
>> - err |= test_kmalloc_node(NULL);
>> - err |= test_kzalloc(NULL);
>> - err |= test_kzalloc_node(NULL);
>> - err |= test_kvmalloc(NULL);
>> - err |= test_kvmalloc_node(NULL);
>> - err |= test_kvzalloc(NULL);
>> - err |= test_kvzalloc_node(NULL);
>> - err |= test_vmalloc(NULL);
>> - err |= test_vmalloc_node(NULL);
>> - err |= test_vzalloc(NULL);
>> - err |= test_vzalloc_node(NULL);
>> - err |= test_devm_kmalloc(dev);
>> - err |= test_devm_kzalloc(dev);
>> + err |= check_allocation_overflow(kmalloc);
>> + err |= check_allocation_overflow(kmalloc_node);
>> + err |= check_allocation_overflow(kzalloc);
>> + err |= check_allocation_overflow(kzalloc_node);
>> + err |= check_allocation_overflow(__vmalloc);
>> + err |= check_allocation_overflow(kvmalloc);
>> + err |= check_allocation_overflow(kvmalloc_node);
>> + err |= check_allocation_overflow(kvzalloc);
>> + err |= check_allocation_overflow(kvzalloc_node);
>> + err |= check_allocation_overflow(devm_kmalloc);
>> + err |= check_allocation_overflow(devm_kzalloc);
>>
>> device_unregister(dev);
I'd prefer if such a local helper macro was defined immediately prior to
the sequence of uses, and then #undef'ed at the end.
Other than that:
Acked-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] overflow: Implement size_t saturating arithmetic helpers
2021-09-21 6:51 ` Rasmus Villemoes
@ 2021-09-21 19:07 ` Kees Cook
2022-01-24 21:13 ` Kees Cook
1 sibling, 0 replies; 12+ messages in thread
From: Kees Cook @ 2021-09-21 19:07 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Gustavo A . R . Silva, Nathan Chancellor, Jason Gunthorpe,
Nick Desaulniers, Leon Romanovsky, Keith Busch, Len Baker,
linux-kernel, linux-hardening
On Tue, Sep 21, 2021 at 08:51:53AM +0200, Rasmus Villemoes wrote:
> On 20/09/2021 20.08, Kees Cook wrote:
>
> > + * Internal logic for size_mul(). Takes variable names from UNIQUE_ID
> > + * so that the local variables here will never collide with other local
> > + * variables (for example, with itself).
> > + */
> > +#define __size_mul(factor1, factor2, __factor1, __factor2, __product) \
> > +({ \
> > + size_t __product; \
> > + size_t __factor1 = (factor1); \
> > + size_t __factor2 = (factor2); \
> > + if (check_mul_overflow(__factor1, __factor2, &__product)) \
> > + __product = SIZE_MAX; \
> > + __product; \
> > +})
> > +
>
> Why can't this just be a static inline taking and returning size_ts,
> avoiding all the unique_id ritual and triple layers of macros?
*hold face* Yeah. I've been doing so much type-agnostic macro work
lately that I completely looked past the characteristics I was
describing for this macro are ... in fact ... how functions actually
work. *sigh* Yes, I'll change all of these to just be static inlines.
If we ever need them as constant expressions, we can fix them then.
> Not that I can see that the __must_check matters much for these anyway;
> if anybody does
>
> size_mul(foo, bar);
>
> that's just a statement with no side effects, so probably the compiler
> would warn anyway, or at least nobody can then go on to do anything
> "wrong". Unlike the check_*_overflow(), which have the (possibly
> wrapped) result in a output-pointer and the "did it overflow" as the
> return value, so you can do
>
> check_mul_overflow(a, b, &d);
> do_stuff_with(d);
>
> were it not for the __must_check wrapper.
>
> [Reminder: __must_check is a bit of a misnomer, the attribute is really
> warn_unused_result, and there's no requirement that the result is part
> of the controlling expression of an if() or while() - just passing the
> result on directly to some other function counts as a "use", which is
> indeed what we do with the size wrappers.]
Yeah, all a good point. My knee-jerk reaction is to include __must_check
just because it's "free" here.
Thanks!
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] overflow: Implement size_t saturating arithmetic helpers
2021-09-21 6:51 ` Rasmus Villemoes
2021-09-21 19:07 ` Kees Cook
@ 2022-01-24 21:13 ` Kees Cook
2022-01-24 21:16 ` Nick Desaulniers
2022-01-25 12:58 ` Jason Gunthorpe
1 sibling, 2 replies; 12+ messages in thread
From: Kees Cook @ 2022-01-24 21:13 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Gustavo A . R . Silva, Nathan Chancellor, Jason Gunthorpe,
Nick Desaulniers, Leon Romanovsky, Keith Busch, Len Baker,
linux-kernel, linux-hardening
*thread necromancy*
On Tue, Sep 21, 2021 at 08:51:53AM +0200, Rasmus Villemoes wrote:
> Not that I can see that the __must_check matters much for these anyway;
> if anybody does
>
> size_mul(foo, bar);
>
> that's just a statement with no side effects, so probably the compiler
> would warn anyway, or at least nobody can then go on to do anything
> "wrong". Unlike the check_*_overflow(), which have the (possibly
> wrapped) result in a output-pointer and the "did it overflow" as the
> return value, so you can do
>
> check_mul_overflow(a, b, &d);
> do_stuff_with(d);
>
> were it not for the __must_check wrapper.
>
> [Reminder: __must_check is a bit of a misnomer, the attribute is really
> warn_unused_result, and there's no requirement that the result is part
> of the controlling expression of an if() or while() - just passing the
> result on directly to some other function counts as a "use", which is
> indeed what we do with the size wrappers.]
What I'd really like is a "store this in a size_t" check to catch dumb
storage size problems (or related overflows). In other words:
size_t big1 = 2147483647;
size_t big2 = 2147483647;
/* Doesn't overflow, but 4611686014132420609 becomes a 1 for int */
int size = size_mul(big1, big2);
...
ptr = kmalloc(size, GFP_KERNEL); /* Allocates a 1 instead... */
I could solve this but removing the assignment, but then I can't compose
calls:
static inline size_t __size_mul(size_t f1, size_t f2)
{
size_t out;
if (check_mul_overflow(f1, f2, &out))
out = SIZE_MAX;
return out;
}
#define size_mul(f1, f2, out) do { \
BUILD_BUG_ON(!__same_type(out, size_t)); \
out = __size_mul(f1, f2); \
} while (0)
i.e. now I can't do size_mul(size_add(...), size_add(...))
Better would be to build the entire kernel with -Wconversion. :)
--
Kees Cook
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] overflow: Implement size_t saturating arithmetic helpers
2022-01-24 21:13 ` Kees Cook
@ 2022-01-24 21:16 ` Nick Desaulniers
2022-01-25 12:58 ` Jason Gunthorpe
1 sibling, 0 replies; 12+ messages in thread
From: Nick Desaulniers @ 2022-01-24 21:16 UTC (permalink / raw)
To: Kees Cook
Cc: Rasmus Villemoes, Gustavo A . R . Silva, Nathan Chancellor,
Jason Gunthorpe, Leon Romanovsky, Keith Busch, Len Baker,
linux-kernel, linux-hardening
On Mon, Jan 24, 2022 at 1:13 PM Kees Cook <keescook@chromium.org> wrote:
>
> What I'd really like is a "store this in a size_t" check to catch dumb
> storage size problems (or related overflows). In other words:
>
> size_t big1 = 2147483647;
> size_t big2 = 2147483647;
>
> /* Doesn't overflow, but 4611686014132420609 becomes a 1 for int */
> int size = size_mul(big1, big2);
> ...
> ptr = kmalloc(size, GFP_KERNEL); /* Allocates a 1 instead... */
-Wshorten-64-to-32
?
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] overflow: Implement size_t saturating arithmetic helpers
2022-01-24 21:13 ` Kees Cook
2022-01-24 21:16 ` Nick Desaulniers
@ 2022-01-25 12:58 ` Jason Gunthorpe
1 sibling, 0 replies; 12+ messages in thread
From: Jason Gunthorpe @ 2022-01-25 12:58 UTC (permalink / raw)
To: Kees Cook
Cc: Rasmus Villemoes, Gustavo A . R . Silva, Nathan Chancellor,
Nick Desaulniers, Leon Romanovsky, Keith Busch, Len Baker,
linux-kernel, linux-hardening
On Mon, Jan 24, 2022 at 01:13:20PM -0800, Kees Cook wrote:
> *thread necromancy*
>
> On Tue, Sep 21, 2021 at 08:51:53AM +0200, Rasmus Villemoes wrote:
> > Not that I can see that the __must_check matters much for these anyway;
> > if anybody does
> >
> > size_mul(foo, bar);
> >
> > that's just a statement with no side effects, so probably the compiler
> > would warn anyway, or at least nobody can then go on to do anything
> > "wrong". Unlike the check_*_overflow(), which have the (possibly
> > wrapped) result in a output-pointer and the "did it overflow" as the
> > return value, so you can do
> >
> > check_mul_overflow(a, b, &d);
> > do_stuff_with(d);
> >
> > were it not for the __must_check wrapper.
> >
> > [Reminder: __must_check is a bit of a misnomer, the attribute is really
> > warn_unused_result, and there's no requirement that the result is part
> > of the controlling expression of an if() or while() - just passing the
> > result on directly to some other function counts as a "use", which is
> > indeed what we do with the size wrappers.]
>
> What I'd really like is a "store this in a size_t" check to catch dumb
> storage size problems (or related overflows). In other words:
Yes, this. The overflow things are nice, but quite often we need to
get things into a size_t to use with an allocator and the rigorous
type checking in the normal overflows is a problem.
Jason
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-01-25 13:00 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-20 18:08 [PATCH 0/2] overflow: Implement size_t saturating arithmetic helpers Kees Cook
2021-09-20 18:08 ` [PATCH 1/2] " Kees Cook
2021-09-20 22:06 ` Nick Desaulniers
2021-09-21 1:38 ` Kees Cook
2021-09-21 6:51 ` Rasmus Villemoes
2021-09-21 19:07 ` Kees Cook
2022-01-24 21:13 ` Kees Cook
2022-01-24 21:16 ` Nick Desaulniers
2022-01-25 12:58 ` Jason Gunthorpe
2021-09-20 18:08 ` [PATCH 2/2] test_overflow: Regularize test reporting output Kees Cook
2021-09-20 22:10 ` Nick Desaulniers
2021-09-21 6:56 ` Rasmus Villemoes
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).