Hi,
v3:
- reduce patches to just docs and new helpers; we don't need pointer fixes yet.
v2: https://lore.kernel.org/all/[email protected]/
v1: https://lore.kernel.org/lkml/[email protected]/
In preparation for gaining instrumentation for signed[1], unsigned[2], and
pointer[3] wrap-around, expand the overflow header to include wrap-around
helpers that can be used to annotate arithmetic where wrapped calculations
are expected (e.g. atomics).
After spending time getting the unsigned integer wrap-around sanitizer
running warning-free on a basic x86_64 boot[4], I think the *_wrap()
helpers first argument being the output type makes the most sense (as
suggested by Rasmus).
-Kees
Link: https://github.com/KSPP/linux/issues/26 [1]
Link: https://github.com/KSPP/linux/issues/27 [2]
Link: https://github.com/KSPP/linux/issues/344 [3]
Link: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=devel/overflow/enable-unsigned-sanitizer [4]
Kees Cook (3):
overflow: Adjust check_*_overflow() kern-doc to reflect results
overflow: Introduce add_wrap(), sub_wrap(), and mul_wrap()
overflow: Introduce inc_wrap() and dec_wrap()
include/linux/overflow.h | 100 ++++++++++++++++++++++++++++++++++-----
lib/overflow_kunit.c | 65 +++++++++++++++++++++++--
2 files changed, 149 insertions(+), 16 deletions(-)
--
2.34.1
Provide helpers that will perform wrapping addition, subtraction, or
multiplication without tripping the arithmetic wrap-around sanitizers. The
first argument is the type under which the wrap-around should happen
with. In other words, these two calls will get very different results:
mul_wrap(int, 50, 50) == 2500
mul_wrap(u8, 50, 50) == 196
Add to the selftests to validate behavior and lack of side-effects.
Cc: Rasmus Villemoes <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
include/linux/overflow.h | 54 ++++++++++++++++++++++++++++++++++++++++
lib/overflow_kunit.c | 23 ++++++++++++++---
2 files changed, 73 insertions(+), 4 deletions(-)
diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index 4e741ebb8005..9b8c05bdb788 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -64,6 +64,24 @@ static inline bool __must_check __must_check_overflow(bool overflow)
#define check_add_overflow(a, b, d) \
__must_check_overflow(__builtin_add_overflow(a, b, d))
+/**
+ * add_wrap() - Intentionally perform a wrapping addition
+ * @type: type for result of calculation
+ * @a: first addend
+ * @b: second addend
+ *
+ * Return the potentially wrapped-around addition without
+ * tripping any wrap-around sanitizers that may be enabled.
+ */
+#define add_wrap(type, a, b) \
+ ({ \
+ type __val; \
+ if (check_add_overflow(a, b, &__val)) { \
+ /* do nothing */ \
+ } \
+ __val; \
+ })
+
/**
* check_sub_overflow() - Calculate subtraction with overflow checking
* @a: minuend; value to subtract from
@@ -77,6 +95,24 @@ static inline bool __must_check __must_check_overflow(bool overflow)
#define check_sub_overflow(a, b, d) \
__must_check_overflow(__builtin_sub_overflow(a, b, d))
+/**
+ * sub_wrap() - Intentionally perform a wrapping subtraction
+ * @type: type for result of calculation
+ * @a: minuend; value to subtract from
+ * @b: subtrahend; value to subtract from @a
+ *
+ * Return the potentially wrapped-around subtraction without
+ * tripping any wrap-around sanitizers that may be enabled.
+ */
+#define sub_wrap(type, a, b) \
+ ({ \
+ type __val; \
+ if (check_sub_overflow(a, b, &__val)) { \
+ /* do nothing */ \
+ } \
+ __val; \
+ })
+
/**
* check_mul_overflow() - Calculate multiplication with overflow checking
* @a: first factor
@@ -90,6 +126,24 @@ static inline bool __must_check __must_check_overflow(bool overflow)
#define check_mul_overflow(a, b, d) \
__must_check_overflow(__builtin_mul_overflow(a, b, d))
+/**
+ * mul_wrap() - Intentionally perform a wrapping multiplication
+ * @type: type for result of calculation
+ * @a: first factor
+ * @b: second factor
+ *
+ * Return the potentially wrapped-around multiplication without
+ * tripping any wrap-around sanitizers that may be enabled.
+ */
+#define mul_wrap(type, a, b) \
+ ({ \
+ type __val; \
+ if (check_mul_overflow(a, b, &__val)) { \
+ /* do nothing */ \
+ } \
+ __val; \
+ })
+
/**
* check_shl_overflow() - Calculate a left-shifted value and check overflow
* @a: Value to be shifted
diff --git a/lib/overflow_kunit.c b/lib/overflow_kunit.c
index c527f6b75789..064dccd973ad 100644
--- a/lib/overflow_kunit.c
+++ b/lib/overflow_kunit.c
@@ -258,15 +258,30 @@ DEFINE_TEST_ARRAY(s64) = {
\
_of = check_ ## op ## _overflow(a, b, &_r); \
KUNIT_EXPECT_EQ_MSG(test, _of, of, \
- "expected "fmt" "sym" "fmt" to%s overflow (type %s)\n", \
+ "expected check "fmt" "sym" "fmt" to%s overflow (type %s)\n", \
a, b, of ? "" : " not", #t); \
KUNIT_EXPECT_EQ_MSG(test, _r, r, \
- "expected "fmt" "sym" "fmt" == "fmt", got "fmt" (type %s)\n", \
+ "expected check "fmt" "sym" "fmt" == "fmt", got "fmt" (type %s)\n", \
a, b, r, _r, #t); \
/* Check for internal macro side-effects. */ \
_of = check_ ## op ## _overflow(_a_orig++, _b_orig++, &_r); \
- KUNIT_EXPECT_EQ_MSG(test, _a_orig, _a_bump, "Unexpected " #op " macro side-effect!\n"); \
- KUNIT_EXPECT_EQ_MSG(test, _b_orig, _b_bump, "Unexpected " #op " macro side-effect!\n"); \
+ KUNIT_EXPECT_EQ_MSG(test, _a_orig, _a_bump, \
+ "Unexpected check " #op " macro side-effect!\n"); \
+ KUNIT_EXPECT_EQ_MSG(test, _b_orig, _b_bump, \
+ "Unexpected check " #op " macro side-effect!\n"); \
+ \
+ _r = op ## _wrap(t, a, b); \
+ KUNIT_EXPECT_TRUE_MSG(test, _r == r, \
+ "expected wrap "fmt" "sym" "fmt" == "fmt", got "fmt" (type %s)\n", \
+ a, b, r, _r, #t); \
+ /* Check for internal macro side-effects. */ \
+ _a_orig = a; \
+ _b_orig = b; \
+ _r = op ## _wrap(t, _a_orig++, _b_orig++); \
+ KUNIT_EXPECT_EQ_MSG(test, _a_orig, _a_bump, \
+ "Unexpected wrap " #op " macro side-effect!\n"); \
+ KUNIT_EXPECT_EQ_MSG(test, _b_orig, _b_bump, \
+ "Unexpected wrap " #op " macro side-effect!\n"); \
} while (0)
#define DEFINE_TEST_FUNC_TYPED(n, t, fmt) \
--
2.34.1
The check_*_overflow() helpers will return results with potentially
wrapped-around values. These values have always been checked by the
selftests, so avoid the confusing language in the kern-doc. The idea of
"safe for use" was relative to the expectation of whether or not the
caller wants a wrapped value -- the calculation itself will always follow
arithmetic wrapping rules.
Cc: "Gustavo A. R. Silva" <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
include/linux/overflow.h | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index 7b5cf4a5cd19..4e741ebb8005 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -57,11 +57,9 @@ static inline bool __must_check __must_check_overflow(bool overflow)
* @b: second addend
* @d: pointer to store sum
*
- * Returns 0 on success.
+ * Returns 0 on success, 1 on wrap-around.
*
- * *@d holds the results of the attempted addition, but is not considered
- * "safe for use" on a non-zero return value, which indicates that the
- * sum has overflowed or been truncated.
+ * *@d holds the results of the attempted addition, which may wrap-around.
*/
#define check_add_overflow(a, b, d) \
__must_check_overflow(__builtin_add_overflow(a, b, d))
@@ -72,11 +70,9 @@ static inline bool __must_check __must_check_overflow(bool overflow)
* @b: subtrahend; value to subtract from @a
* @d: pointer to store difference
*
- * Returns 0 on success.
+ * Returns 0 on success, 1 on wrap-around.
*
- * *@d holds the results of the attempted subtraction, but is not considered
- * "safe for use" on a non-zero return value, which indicates that the
- * difference has underflowed or been truncated.
+ * *@d holds the results of the attempted subtraction, which may wrap-around.
*/
#define check_sub_overflow(a, b, d) \
__must_check_overflow(__builtin_sub_overflow(a, b, d))
@@ -87,11 +83,9 @@ static inline bool __must_check __must_check_overflow(bool overflow)
* @b: second factor
* @d: pointer to store product
*
- * Returns 0 on success.
+ * Returns 0 on success, 1 on wrap-around.
*
- * *@d holds the results of the attempted multiplication, but is not
- * considered "safe for use" on a non-zero return value, which indicates
- * that the product has overflowed or been truncated.
+ * *@d holds the results of the attempted multiplication, which may wrap-around.
*/
#define check_mul_overflow(a, b, d) \
__must_check_overflow(__builtin_mul_overflow(a, b, d))
--
2.34.1
This allows replacements of the idioms "var += offset" and "var -= offset"
with the inc_wrap() and dec_wrap() helpers respectively. They will avoid
wrap-around sanitizer instrumentation.
Add to the selftests to validate behavior and lack of side-effects.
Cc: Rasmus Villemoes <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: "Gustavo A. R. Silva" <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
include/linux/overflow.h | 28 +++++++++++++++++++++++++++
lib/overflow_kunit.c | 42 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 70 insertions(+)
diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index 9b8c05bdb788..6a4c18544ab1 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -82,6 +82,20 @@ static inline bool __must_check __must_check_overflow(bool overflow)
__val; \
})
+/**
+ * inc_wrap() - Intentionally perform a wrapping increment
+ * @var: variable to be incremented
+ * @offset: amount to add
+ *
+ * Increments @var by @offset with wrap-around. Returns the resulting
+ * value of @var. Will not trip any wrap-around sanitizers.
+ */
+#define inc_wrap(var, offset) \
+ ({ \
+ typeof(var) *__ptr = &(var); \
+ *__ptr = add_wrap(typeof(var), *__ptr, offset); \
+ })
+
/**
* check_sub_overflow() - Calculate subtraction with overflow checking
* @a: minuend; value to subtract from
@@ -113,6 +127,20 @@ static inline bool __must_check __must_check_overflow(bool overflow)
__val; \
})
+/**
+ * dec_wrap() - Intentionally perform a wrapping decrement
+ * @var: variable to be decremented
+ * @offset: amount to subtract
+ *
+ * Decrements @var by @offset with wrap-around. Returns the resulting
+ * value of @var. Will not trip any wrap-around sanitizers.
+ */
+#define dec_wrap(var, offset) \
+ ({ \
+ typeof(var) *__ptr = &(var); \
+ *__ptr = sub_wrap(typeof(var), *__ptr, offset); \
+ })
+
/**
* check_mul_overflow() - Calculate multiplication with overflow checking
* @a: first factor
diff --git a/lib/overflow_kunit.c b/lib/overflow_kunit.c
index 064dccd973ad..23e37bce0022 100644
--- a/lib/overflow_kunit.c
+++ b/lib/overflow_kunit.c
@@ -284,14 +284,56 @@ DEFINE_TEST_ARRAY(s64) = {
"Unexpected wrap " #op " macro side-effect!\n"); \
} while (0)
+static int global_counter;
+static void bump_counter(void) {
+ global_counter ++;
+}
+
+static int get_index(void) {
+ volatile int index = 0;
+ bump_counter();
+ return index;
+}
+
+#define check_self_op(fmt, op, sym, a, b) do { \
+ typeof(a + 0) _a = a; \
+ typeof(b + 0) _b = b; \
+ typeof(a + 0) _a_sym = a; \
+ typeof(a + 0) _a_orig[1] = { a }; \
+ typeof(b + 0) _b_orig = b; \
+ typeof(b + 0) _b_bump = b + 1; \
+ typeof(a + 0) _r; \
+ \
+ _a_sym sym _b; \
+ _r = op ## _wrap(_a, _b); \
+ KUNIT_EXPECT_TRUE_MSG(test, _r == _a_sym, \
+ "expected "fmt" "#op" "fmt" == "fmt", got "fmt"\n", \
+ a, b, _a_sym, _r); \
+ KUNIT_EXPECT_TRUE_MSG(test, _a == _a_sym, \
+ "expected "fmt" "#op" "fmt" == "fmt", got "fmt"\n", \
+ a, b, _a_sym, _a); \
+ /* Check for internal macro side-effects. */ \
+ global_counter = 0; \
+ op ## _wrap(_a_orig[get_index()], _b_orig++); \
+ KUNIT_EXPECT_EQ_MSG(test, global_counter, 1, \
+ "Unexpected " #op "_wrap() macro side-effect on arg1!\n"); \
+ KUNIT_EXPECT_EQ_MSG(test, _b_orig, _b_bump, \
+ "Unexpected " #op "_wrap() macro side-effect on arg2!\n"); \
+} while (0)
+
#define DEFINE_TEST_FUNC_TYPED(n, t, fmt) \
static void do_test_ ## n(struct kunit *test, const struct test_ ## n *p) \
{ \
+ /* check_{add,sub,mul}_overflow() and {add,sub,mul}_wrap() */ \
check_one_op(t, fmt, add, "+", p->a, p->b, p->sum, p->s_of); \
check_one_op(t, fmt, add, "+", p->b, p->a, p->sum, p->s_of); \
check_one_op(t, fmt, sub, "-", p->a, p->b, p->diff, p->d_of); \
check_one_op(t, fmt, mul, "*", p->a, p->b, p->prod, p->p_of); \
check_one_op(t, fmt, mul, "*", p->b, p->a, p->prod, p->p_of); \
+ /* {inc,dec}_wrap() */ \
+ check_self_op(fmt, inc, +=, p->a, p->b); \
+ check_self_op(fmt, inc, +=, p->b, p->a); \
+ check_self_op(fmt, dec, -=, p->a, p->b); \
} \
\
static void n ## _overflow_test(struct kunit *test) { \
--
2.34.1
On Mon, 5 Feb 2024 at 10:12, Kees Cook <[email protected]> wrote:
>
> Provide helpers that will perform wrapping addition, subtraction, or
> multiplication without tripping the arithmetic wrap-around sanitizers. The
> first argument is the type under which the wrap-around should happen
> with. In other words, these two calls will get very different results:
>
> mul_wrap(int, 50, 50) == 2500
> mul_wrap(u8, 50, 50) == 196
>
> Add to the selftests to validate behavior and lack of side-effects.
>
> Cc: Rasmus Villemoes <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
> ---
> include/linux/overflow.h | 54 ++++++++++++++++++++++++++++++++++++++++
> lib/overflow_kunit.c | 23 ++++++++++++++---
> 2 files changed, 73 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> index 4e741ebb8005..9b8c05bdb788 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -64,6 +64,24 @@ static inline bool __must_check __must_check_overflow(bool overflow)
> #define check_add_overflow(a, b, d) \
> __must_check_overflow(__builtin_add_overflow(a, b, d))
>
> +/**
> + * add_wrap() - Intentionally perform a wrapping addition
> + * @type: type for result of calculation
> + * @a: first addend
> + * @b: second addend
> + *
> + * Return the potentially wrapped-around addition without
> + * tripping any wrap-around sanitizers that may be enabled.
> + */
> +#define add_wrap(type, a, b) \
> + ({ \
> + type __val; \
> + if (check_add_overflow(a, b, &__val)) { \
> + /* do nothing */ \
The whole reason check_*_overflow() exists is to wrap the builtin in a
function with __must_check. Here the result is explicitly ignored, so
do we have to go through the check_add_overflow indirection? Why not
just use the builtin directly? It might make sense to make the
compiler's job a little easier, because I predict that
__must_check_overflow will be outlined with enough instrumentation
(maybe it should have been __always_inline).
On 2/5/24 07:31, Marco Elver wrote:
> On Mon, 5 Feb 2024 at 10:12, Kees Cook <[email protected]> wrote:
>>
>> Provide helpers that will perform wrapping addition, subtraction, or
>> multiplication without tripping the arithmetic wrap-around sanitizers. The
>> first argument is the type under which the wrap-around should happen
>> with. In other words, these two calls will get very different results:
>>
>> mul_wrap(int, 50, 50) == 2500
>> mul_wrap(u8, 50, 50) == 196
>>
>> Add to the selftests to validate behavior and lack of side-effects.
>>
>> Cc: Rasmus Villemoes <[email protected]>
>> Cc: Mark Rutland <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: Kees Cook <[email protected]>
>> ---
>> include/linux/overflow.h | 54 ++++++++++++++++++++++++++++++++++++++++
>> lib/overflow_kunit.c | 23 ++++++++++++++---
>> 2 files changed, 73 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
>> index 4e741ebb8005..9b8c05bdb788 100644
>> --- a/include/linux/overflow.h
>> +++ b/include/linux/overflow.h
>> @@ -64,6 +64,24 @@ static inline bool __must_check __must_check_overflow(bool overflow)
>> #define check_add_overflow(a, b, d) \
>> __must_check_overflow(__builtin_add_overflow(a, b, d))
>>
>> +/**
>> + * add_wrap() - Intentionally perform a wrapping addition
>> + * @type: type for result of calculation
>> + * @a: first addend
>> + * @b: second addend
>> + *
>> + * Return the potentially wrapped-around addition without
>> + * tripping any wrap-around sanitizers that may be enabled.
>> + */
>> +#define add_wrap(type, a, b) \
>> + ({ \
>> + type __val; \
>> + if (check_add_overflow(a, b, &__val)) { \
>> + /* do nothing */ \
>
> The whole reason check_*_overflow() exists is to wrap the builtin in a
> function with __must_check. Here the result is explicitly ignored, so
> do we have to go through the check_add_overflow indirection? Why not
> just use the builtin directly? It might make sense to make the
> compiler's job a little easier, because I predict that
> __must_check_overflow will be outlined with enough instrumentation
> (maybe it should have been __always_inline).
Yeah; I think that directly calling __builtin_*_overflow() is a bit
cleaner.
I wonder if there is any particular reason for not doing that.
In any case, this version of the add_wrap() helper with the `type` as
parameter looks much better than the v1 that relied on `typeof(a)`. :)
So,
Reviewed-by: Gustavo A. R. Silva <[email protected]>
Thanks!
--
Gustavo
On 2/5/24 03:12, Kees Cook wrote:
> The check_*_overflow() helpers will return results with potentially
> wrapped-around values. These values have always been checked by the
> selftests, so avoid the confusing language in the kern-doc. The idea of
> "safe for use" was relative to the expectation of whether or not the
> caller wants a wrapped value -- the calculation itself will always follow
> arithmetic wrapping rules.
>
> Cc: "Gustavo A. R. Silva" <[email protected]>
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
Better to be concise and direct. :)
Reviewed-by: Gustavo A. R. Silva <[email protected]>
Thanks!
--
Gustavo
> ---
> include/linux/overflow.h | 18 ++++++------------
> 1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> index 7b5cf4a5cd19..4e741ebb8005 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -57,11 +57,9 @@ static inline bool __must_check __must_check_overflow(bool overflow)
> * @b: second addend
> * @d: pointer to store sum
> *
> - * Returns 0 on success.
> + * Returns 0 on success, 1 on wrap-around.
> *
> - * *@d holds the results of the attempted addition, but is not considered
> - * "safe for use" on a non-zero return value, which indicates that the
> - * sum has overflowed or been truncated.
> + * *@d holds the results of the attempted addition, which may wrap-around.
> */
> #define check_add_overflow(a, b, d) \
> __must_check_overflow(__builtin_add_overflow(a, b, d))
> @@ -72,11 +70,9 @@ static inline bool __must_check __must_check_overflow(bool overflow)
> * @b: subtrahend; value to subtract from @a
> * @d: pointer to store difference
> *
> - * Returns 0 on success.
> + * Returns 0 on success, 1 on wrap-around.
> *
> - * *@d holds the results of the attempted subtraction, but is not considered
> - * "safe for use" on a non-zero return value, which indicates that the
> - * difference has underflowed or been truncated.
> + * *@d holds the results of the attempted subtraction, which may wrap-around.
> */
> #define check_sub_overflow(a, b, d) \
> __must_check_overflow(__builtin_sub_overflow(a, b, d))
> @@ -87,11 +83,9 @@ static inline bool __must_check __must_check_overflow(bool overflow)
> * @b: second factor
> * @d: pointer to store product
> *
> - * Returns 0 on success.
> + * Returns 0 on success, 1 on wrap-around.
> *
> - * *@d holds the results of the attempted multiplication, but is not
> - * considered "safe for use" on a non-zero return value, which indicates
> - * that the product has overflowed or been truncated.
> + * *@d holds the results of the attempted multiplication, which may wrap-around.
> */
> #define check_mul_overflow(a, b, d) \
> __must_check_overflow(__builtin_mul_overflow(a, b, d))
On Mon, Feb 05, 2024 at 01:12:30AM -0800, Kees Cook wrote:
> Subject: Re: [PATCH v3 2/3] overflow: Introduce add_wrap(), sub_wrap(), and mul_wrap()
Maybe these should be called wrapping_add, wrapping_sub, and wrapping_mul?
Those names are more grammatically correct, and Rust chose those names too.
- Eric
On Mon, Feb 05, 2024 at 12:21:45PM -0800, Eric Biggers wrote:
> On Mon, Feb 05, 2024 at 01:12:30AM -0800, Kees Cook wrote:
> > Subject: Re: [PATCH v3 2/3] overflow: Introduce add_wrap(), sub_wrap(), and mul_wrap()
>
> Maybe these should be called wrapping_add, wrapping_sub, and wrapping_mul?
> Those names are more grammatically correct, and Rust chose those names too.
Sure, that works for me. What bout the inc_wrap() and dec_wrap() names?
I assume wrapping_inc() and wrapping_dec() ?
--
Kees Cook
On Mon, Feb 05, 2024 at 02:44:14PM -0800, Kees Cook wrote:
> On Mon, Feb 05, 2024 at 12:21:45PM -0800, Eric Biggers wrote:
> > On Mon, Feb 05, 2024 at 01:12:30AM -0800, Kees Cook wrote:
> > > Subject: Re: [PATCH v3 2/3] overflow: Introduce add_wrap(), sub_wrap(), and mul_wrap()
> >
> > Maybe these should be called wrapping_add, wrapping_sub, and wrapping_mul?
> > Those names are more grammatically correct, and Rust chose those names too.
>
> Sure, that works for me. What bout the inc_wrap() and dec_wrap() names?
> I assume wrapping_inc() and wrapping_dec() ?
>
Yes, though I'm not sure those should exist at all. Maybe a += b should just
become a = wrapping_add(a, b), instead of wrapping_inc(a, b)?
wrapping_inc(a, b) isn't as self-explanatory. Likewise for wrapping_dec.
- Eric
On February 5, 2024 11:17:12 PM GMT, Eric Biggers <[email protected]> wrote:
>On Mon, Feb 05, 2024 at 02:44:14PM -0800, Kees Cook wrote:
>> On Mon, Feb 05, 2024 at 12:21:45PM -0800, Eric Biggers wrote:
>> > On Mon, Feb 05, 2024 at 01:12:30AM -0800, Kees Cook wrote:
>> > > Subject: Re: [PATCH v3 2/3] overflow: Introduce add_wrap(), sub_wrap(), and mul_wrap()
>> >
>> > Maybe these should be called wrapping_add, wrapping_sub, and wrapping_mul?
>> > Those names are more grammatically correct, and Rust chose those names too.
>>
>> Sure, that works for me. What bout the inc_wrap() and dec_wrap() names?
>> I assume wrapping_inc() and wrapping_dec() ?
>>
>
>Yes, though I'm not sure those should exist at all. Maybe a += b should just
>become a = wrapping_add(a, b), instead of wrapping_inc(a, b)?
>wrapping_inc(a, b) isn't as self-explanatory. Likewise for wrapping_dec.
It was to avoid repeating type information, as it would go from:
var_a += var_b;
to:
var_a = wrapping_add(typeof(var_a), var_a, var_b);
Which repeats "var_a" 3 times. :|
--
Kees Cook
On 06/02/2024 00.21, Kees Cook wrote:
>
>
> On February 5, 2024 11:17:12 PM GMT, Eric Biggers <[email protected]> wrote:
>> On Mon, Feb 05, 2024 at 02:44:14PM -0800, Kees Cook wrote:
>>> On Mon, Feb 05, 2024 at 12:21:45PM -0800, Eric Biggers wrote:
>>>> On Mon, Feb 05, 2024 at 01:12:30AM -0800, Kees Cook wrote:
>>>>> Subject: Re: [PATCH v3 2/3] overflow: Introduce add_wrap(), sub_wrap(), and mul_wrap()
>>>>
>>>> Maybe these should be called wrapping_add, wrapping_sub, and wrapping_mul?
>>>> Those names are more grammatically correct, and Rust chose those names too.
>>>
>>> Sure, that works for me. What bout the inc_wrap() and dec_wrap() names?
>>> I assume wrapping_inc() and wrapping_dec() ?
>>>
>>
>> Yes, though I'm not sure those should exist at all. Maybe a += b should just
>> become a = wrapping_add(a, b), instead of wrapping_inc(a, b)?
>> wrapping_inc(a, b) isn't as self-explanatory. Likewise for wrapping_dec.
>
> It was to avoid repeating type information, as it would go from:
>
> var_a += var_b;
>
> to:
>
> var_a = wrapping_add(typeof(var_a), var_a, var_b);
>
> Which repeats "var_a" 3 times. :|
Yeah, I think that's a reasonable rationale. I'm fine with the
wrapping_* naming, and then the _inc and _dec helpers should follow.
However, I now wonder if those should really also return the new value.
Yes, that corresponds to the value of the expression (a += b), but
nobody would ever write c = (a += b) or otherwise make use of that
value, and the naming doesn't immediately imply whether one should think
of ++a or a++.
Rasmus
On Tue, Feb 06, 2024 at 09:42:26AM +0100, Rasmus Villemoes wrote:
> On 06/02/2024 00.21, Kees Cook wrote:
> >
> >
> > On February 5, 2024 11:17:12 PM GMT, Eric Biggers <[email protected]> wrote:
> >> On Mon, Feb 05, 2024 at 02:44:14PM -0800, Kees Cook wrote:
> >>> On Mon, Feb 05, 2024 at 12:21:45PM -0800, Eric Biggers wrote:
> >>>> On Mon, Feb 05, 2024 at 01:12:30AM -0800, Kees Cook wrote:
> >>>>> Subject: Re: [PATCH v3 2/3] overflow: Introduce add_wrap(), sub_wrap(), and mul_wrap()
> >>>>
> >>>> Maybe these should be called wrapping_add, wrapping_sub, and wrapping_mul?
> >>>> Those names are more grammatically correct, and Rust chose those names too.
> >>>
> >>> Sure, that works for me. What bout the inc_wrap() and dec_wrap() names?
> >>> I assume wrapping_inc() and wrapping_dec() ?
> >>>
> >>
> >> Yes, though I'm not sure those should exist at all. Maybe a += b should just
> >> become a = wrapping_add(a, b), instead of wrapping_inc(a, b)?
> >> wrapping_inc(a, b) isn't as self-explanatory. Likewise for wrapping_dec.
> >
> > It was to avoid repeating type information, as it would go from:
> >
> > var_a += var_b;
> >
> > to:
> >
> > var_a = wrapping_add(typeof(var_a), var_a, var_b);
> >
> > Which repeats "var_a" 3 times. :|
>
> Yeah, I think that's a reasonable rationale. I'm fine with the
> wrapping_* naming, and then the _inc and _dec helpers should follow.
Sounds good.
> However, I now wonder if those should really also return the new value.
> Yes, that corresponds to the value of the expression (a += b), but
> nobody would ever write c = (a += b) or otherwise make use of that
> value, and the naming doesn't immediately imply whether one should think
> of ++a or a++.
They were designed to return the new value, and the selftests validate
that. I've updated the kern-doc to reflect this.
--
Kees Cook
On Mon, Feb 05, 2024 at 02:31:04PM +0100, Marco Elver wrote:
> On Mon, 5 Feb 2024 at 10:12, Kees Cook <[email protected]> wrote:
> >
> > Provide helpers that will perform wrapping addition, subtraction, or
> > multiplication without tripping the arithmetic wrap-around sanitizers. The
> > first argument is the type under which the wrap-around should happen
> > with. In other words, these two calls will get very different results:
> >
> > mul_wrap(int, 50, 50) == 2500
> > mul_wrap(u8, 50, 50) == 196
> >
> > Add to the selftests to validate behavior and lack of side-effects.
> >
> > Cc: Rasmus Villemoes <[email protected]>
> > Cc: Mark Rutland <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Kees Cook <[email protected]>
> > ---
> > include/linux/overflow.h | 54 ++++++++++++++++++++++++++++++++++++++++
> > lib/overflow_kunit.c | 23 ++++++++++++++---
> > 2 files changed, 73 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> > index 4e741ebb8005..9b8c05bdb788 100644
> > --- a/include/linux/overflow.h
> > +++ b/include/linux/overflow.h
> > @@ -64,6 +64,24 @@ static inline bool __must_check __must_check_overflow(bool overflow)
> > #define check_add_overflow(a, b, d) \
> > __must_check_overflow(__builtin_add_overflow(a, b, d))
> >
> > +/**
> > + * add_wrap() - Intentionally perform a wrapping addition
> > + * @type: type for result of calculation
> > + * @a: first addend
> > + * @b: second addend
> > + *
> > + * Return the potentially wrapped-around addition without
> > + * tripping any wrap-around sanitizers that may be enabled.
> > + */
> > +#define add_wrap(type, a, b) \
> > + ({ \
> > + type __val; \
> > + if (check_add_overflow(a, b, &__val)) { \
> > + /* do nothing */ \
>
> The whole reason check_*_overflow() exists is to wrap the builtin in a
> function with __must_check. Here the result is explicitly ignored, so
> do we have to go through the check_add_overflow indirection? Why not
> just use the builtin directly? It might make sense to make the
Yes, this follows now. This is a leftover from extending the helpers to
work with pointers, which I don't have any current use for now. I'll fix
this.
> compiler's job a little easier, because I predict that
> __must_check_overflow will be outlined with enough instrumentation
> (maybe it should have been __always_inline).
I could change that separately, yeah.
--
Kees Cook