diff mbox series

[v3,2/3] overflow: Introduce add_wrap(), sub_wrap(), and mul_wrap()

Message ID 20240205091233.1357377-2-keescook@chromium.org (mailing list archive)
State Superseded
Headers show
Series overflow: Introduce wrapping helpers | expand

Commit Message

Kees Cook Feb. 5, 2024, 9:12 a.m. UTC
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

Marco Elver Feb. 5, 2024, 1:31 p.m. UTC | #1
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).
Gustavo A. R. Silva Feb. 5, 2024, 7:53 p.m. UTC | #2
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
Eric Biggers Feb. 5, 2024, 8:21 p.m. UTC | #3
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
Kees Cook Feb. 5, 2024, 10:44 p.m. UTC | #4
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() ?
Eric Biggers Feb. 5, 2024, 11:17 p.m. UTC | #5
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
Kees Cook Feb. 5, 2024, 11:21 p.m. UTC | #6
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. :|
Rasmus Villemoes Feb. 6, 2024, 8:42 a.m. UTC | #7
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
Kees Cook Feb. 6, 2024, 10:01 a.m. UTC | #8
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 Feb. 6, 2024, 10:05 a.m. UTC | #9
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.
diff mbox series

Patch

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)				\