[v3,2/3] overflow: Introduce add_wrap(), sub_wrap(), and mul_wrap()
Commit Message
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(-)
Comments
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 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
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() ?
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. :|
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.
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.
@@ -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
@@ -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) \