| Message ID | 20240123002814.1396804-2-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | overflow: Refactor open-coded arithmetic wrap-around | expand |
On 23/01/2024 01.26, Kees Cook wrote: > For instances where only the overflow needs to be checked (and the sum > isn't used), provide the new helper add_would_overflow(), which is > a wrapper for check_add_overflow(). > > Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org> > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > include/linux/overflow.h | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > index 099f2e559aa8..ac088f73e0fd 100644 > --- a/include/linux/overflow.h > +++ b/include/linux/overflow.h > @@ -108,6 +108,22 @@ static inline bool __must_check __must_check_overflow(bool overflow) > __builtin_add_overflow(__filter_integral(a), b, \ > __filter_ptrint(d)))) > > +/** > + * add_would_overflow() - Check if an addition would overflow > + * @a: first addend > + * @b: second addend > + * > + * Returns true if the sum would overflow. > + * > + * To keep a copy of the sum when the addition doesn't overflow, use > + * check_add_overflow() instead. > + */ > +#define add_would_overflow(a, b) \ > + __must_check_overflow(({ \ > + size_t __result; \ > + check_add_overflow(a, b, &__result);\ > + })) Hm, I think this is a bit too ill-defined. Why is the target type hard-coded as size_t? What if a and b are u64, and we're on a 32 bit target? Then a+b might not overflow but this helper would claim it did. But we also cannot just use typeof(a+b) instead of size_t, since that breaks when a and b are narrower than int (adding two u16 never overflows since they get promoted to int, but then if assigning the result to a u16 one truncates...). Perhaps the target type must be explicit? sum_fits_in_type(T, a, b) ? IDK, I just don't think size_t is the right thing to use in something that is otherwise supposed to be type-generic. Rasmus
On Tue, Jan 23, 2024 at 09:03:10AM +0100, Rasmus Villemoes wrote: > On 23/01/2024 01.26, Kees Cook wrote: > > For instances where only the overflow needs to be checked (and the sum > > isn't used), provide the new helper add_would_overflow(), which is > > a wrapper for check_add_overflow(). > > > > Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org> > > Cc: linux-hardening@vger.kernel.org > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > include/linux/overflow.h | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > > index 099f2e559aa8..ac088f73e0fd 100644 > > --- a/include/linux/overflow.h > > +++ b/include/linux/overflow.h > > @@ -108,6 +108,22 @@ static inline bool __must_check __must_check_overflow(bool overflow) > > __builtin_add_overflow(__filter_integral(a), b, \ > > __filter_ptrint(d)))) > > > > +/** > > + * add_would_overflow() - Check if an addition would overflow > > + * @a: first addend > > + * @b: second addend > > + * > > + * Returns true if the sum would overflow. > > + * > > + * To keep a copy of the sum when the addition doesn't overflow, use > > + * check_add_overflow() instead. > > + */ > > +#define add_would_overflow(a, b) \ > > + __must_check_overflow(({ \ > > + size_t __result; \ > > + check_add_overflow(a, b, &__result);\ > > + })) > > Hm, I think this is a bit too ill-defined. Why is the target type > hard-coded as size_t? What if a and b are u64, and we're on a 32 bit > target? Then a+b might not overflow but this helper would claim it did. Oooh, yes. That's no good. Thanks. > But we also cannot just use typeof(a+b) instead of size_t, since that > breaks when a and b are narrower than int (adding two u16 never > overflows since they get promoted to int, but then if assigning the > result to a u16 one truncates...). The add_would_overflow() is aimed at replacing the "v + o < v" pattern, so perhaps use typeof(a) ? > Perhaps the target type must be explicit? sum_fits_in_type(T, a, b) ? > IDK, I just don't think size_t is the right thing to use in something > that is otherwise supposed to be type-generic. I will use typeof(a) and check binary differences to see if there are any places doing something unexpected... -Kees
diff --git a/include/linux/overflow.h b/include/linux/overflow.h index 099f2e559aa8..ac088f73e0fd 100644 --- a/include/linux/overflow.h +++ b/include/linux/overflow.h @@ -108,6 +108,22 @@ static inline bool __must_check __must_check_overflow(bool overflow) __builtin_add_overflow(__filter_integral(a), b, \ __filter_ptrint(d)))) +/** + * add_would_overflow() - Check if an addition would overflow + * @a: first addend + * @b: second addend + * + * Returns true if the sum would overflow. + * + * To keep a copy of the sum when the addition doesn't overflow, use + * check_add_overflow() instead. + */ +#define add_would_overflow(a, b) \ + __must_check_overflow(({ \ + size_t __result; \ + check_add_overflow(a, b, &__result);\ + })) + /** * check_sub_overflow() - Calculate subtraction with overflow checking * @a: minuend; value to subtract from
For instances where only the overflow needs to be checked (and the sum isn't used), provide the new helper add_would_overflow(), which is a wrapper for check_add_overflow(). Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org> Cc: linux-hardening@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- include/linux/overflow.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)