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 <rasmus.villemoes@prevas.dk>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
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.1On 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 <ebiggers@kernel.org> 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 <ebiggers@kernel.org> 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 <ebiggers@kernel.org> 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, 5 Feb 2024 at 10:12, Kees Cook <keescook@chromium.org> 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 <rasmus.villemoes@prevas.dk>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> 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 Mon, Feb 05, 2024 at 02:31:04PM +0100, Marco Elver wrote:
> On Mon, 5 Feb 2024 at 10:12, Kees Cook <keescook@chromium.org> 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 <rasmus.villemoes@prevas.dk>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: linux-hardening@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > 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
On 2/5/24 07:31, Marco Elver wrote:
> On Mon, 5 Feb 2024 at 10:12, Kees Cook <keescook@chromium.org> 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 <rasmus.villemoes@prevas.dk>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: linux-hardening@vger.kernel.org
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> 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 <gustavoars@kernel.org>
Thanks!
--
Gustavo
© 2016 - 2026 Red Hat, Inc.