| Message ID | 202008121450.405E4A3@keescook (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | overflow: Add __must_check attribute to check_*() helpers | expand |
On 12/08/2020 23.51, Kees Cook wrote: > Since the destination variable of the check_*_overflow() helpers will > contain a wrapped value on failure, it would be best to make sure callers > really did check the return result of the helper. Adjust the macros to use > a bool-wrapping static inline that is marked with __must_check. This means > the macros can continue to have their type-agnostic behavior while gaining > the function attribute (that cannot be applied directly to macros). > > Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > include/linux/overflow.h | 51 +++++++++++++++++++++++----------------- > 1 file changed, 30 insertions(+), 21 deletions(-) > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > index 93fcef105061..ef7d538c2d08 100644 > --- a/include/linux/overflow.h > +++ b/include/linux/overflow.h > @@ -43,6 +43,16 @@ > #define is_non_negative(a) ((a) > 0 || (a) == 0) > #define is_negative(a) (!(is_non_negative(a))) > > +/* > + * Allows to effectively us apply __must_check to a macro so we can have word ordering? > + * both the type-agnostic benefits of the macros while also being able to > + * enforce that the return value is, in fact, checked. > + */ > +static inline bool __must_check __must_check_bool(bool condition) > +{ > + return unlikely(condition); > +} > + > #ifdef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW > /* > * For simplicity and code hygiene, the fallback code below insists on > @@ -52,32 +62,32 @@ > * alias for __builtin_add_overflow, but add type checks similar to > * below. > */ > -#define check_add_overflow(a, b, d) ({ \ > +#define check_add_overflow(a, b, d) __must_check_bool(({ \ > typeof(a) __a = (a); \ > typeof(b) __b = (b); \ > typeof(d) __d = (d); \ > (void) (&__a == &__b); \ > (void) (&__a == __d); \ > __builtin_add_overflow(__a, __b, __d); \ > -}) > +})) Sorry, I meant to send this before your cooking was done but forgot about it again. Not a big deal, but it occurred to me it might be better to rename the existing check_*_overflow to __check_*_overflow (in both branches of the COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW), and then #define check_*_overflow(a, b, d) __must_check_bool(__check_*_overflow(a, b, d)) Mostly because it gives less whitespace churn, but it might also be handy to have the dunder versions available (if nothing else then perhaps in some test code). But as I said, no biggie, I'm fine either way. Now I'm just curious if 0-day is going to find some warning introduced by this :) Rasmus
On Wed, Aug 12, 2020 at 02:51:52PM -0700, Kees Cook wrote: > +/* > + * Allows to effectively us apply __must_check to a macro so we can have > + * both the type-agnostic benefits of the macros while also being able to > + * enforce that the return value is, in fact, checked. > + */ > +static inline bool __must_check __must_check_bool(bool condition) > +{ > + return unlikely(condition); > +} I'm fine with the concept, but this is a weirdly-generically-named function that has a very specific unlikely() in it. So I'd call this __must_check_overflow() and then it's obvious that overflow is unlikely(), whereas it's not obvious that __must_check_bool() is going to be unlikely().
On 13/08/2020 13.23, Matthew Wilcox wrote: > On Wed, Aug 12, 2020 at 02:51:52PM -0700, Kees Cook wrote: >> +/* >> + * Allows to effectively us apply __must_check to a macro so we can have >> + * both the type-agnostic benefits of the macros while also being able to >> + * enforce that the return value is, in fact, checked. >> + */ >> +static inline bool __must_check __must_check_bool(bool condition) >> +{ >> + return unlikely(condition); >> +} > > I'm fine with the concept, but this is a weirdly-generically-named > function that has a very specific unlikely() in it. So I'd call > this __must_check_overflow() and then it's obvious that overflow is > unlikely(), whereas it's not obvious that __must_check_bool() is going > to be unlikely(). Incidentally, __must_check_overflow was what was actually Suggested-by me - though I didn't think too hard about that name, I certainly agree with your reasoning. I still don't know if (un)likely annotations actually matter when used this way, but at least the same pattern is used in kernel/sched/, so probably. Rasmus
On Thu, Aug 13, 2020 at 08:39:44AM +0200, Rasmus Villemoes wrote: > On 12/08/2020 23.51, Kees Cook wrote: > > Since the destination variable of the check_*_overflow() helpers will > > contain a wrapped value on failure, it would be best to make sure callers > > really did check the return result of the helper. Adjust the macros to use > > a bool-wrapping static inline that is marked with __must_check. This means > > the macros can continue to have their type-agnostic behavior while gaining > > the function attribute (that cannot be applied directly to macros). > > > > Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > include/linux/overflow.h | 51 +++++++++++++++++++++++----------------- > > 1 file changed, 30 insertions(+), 21 deletions(-) > > > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > > index 93fcef105061..ef7d538c2d08 100644 > > --- a/include/linux/overflow.h > > +++ b/include/linux/overflow.h > > @@ -43,6 +43,16 @@ > > #define is_non_negative(a) ((a) > 0 || (a) == 0) > > #define is_negative(a) (!(is_non_negative(a))) > > > > +/* > > + * Allows to effectively us apply __must_check to a macro so we can have > > word ordering? This and the __must_check-bool() renaming now done and sent in v2. Thanks! > Sorry, I meant to send this before your cooking was done but forgot > about it again. Not a big deal, but it occurred to me it might be better > to rename the existing check_*_overflow to __check_*_overflow (in both > branches of the COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW), and then > > #define check_*_overflow(a, b, d) > __must_check_bool(__check_*_overflow(a, b, d)) At the end of the day, I'd rather not have a way to ignore the overflow in this way -- I'd rather have a set of wrap_mul_overflow() helpers instead. Then we've got proper annotation of the expectation (and a place for function attributes to be added to tell sanitizers to ignore overflow).
diff --git a/include/linux/overflow.h b/include/linux/overflow.h index 93fcef105061..ef7d538c2d08 100644 --- a/include/linux/overflow.h +++ b/include/linux/overflow.h @@ -43,6 +43,16 @@ #define is_non_negative(a) ((a) > 0 || (a) == 0) #define is_negative(a) (!(is_non_negative(a))) +/* + * Allows to effectively us apply __must_check to a macro so we can have + * both the type-agnostic benefits of the macros while also being able to + * enforce that the return value is, in fact, checked. + */ +static inline bool __must_check __must_check_bool(bool condition) +{ + return unlikely(condition); +} + #ifdef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW /* * For simplicity and code hygiene, the fallback code below insists on @@ -52,32 +62,32 @@ * alias for __builtin_add_overflow, but add type checks similar to * below. */ -#define check_add_overflow(a, b, d) ({ \ +#define check_add_overflow(a, b, d) __must_check_bool(({ \ typeof(a) __a = (a); \ typeof(b) __b = (b); \ typeof(d) __d = (d); \ (void) (&__a == &__b); \ (void) (&__a == __d); \ __builtin_add_overflow(__a, __b, __d); \ -}) +})) -#define check_sub_overflow(a, b, d) ({ \ +#define check_sub_overflow(a, b, d) __must_check_bool(({ \ typeof(a) __a = (a); \ typeof(b) __b = (b); \ typeof(d) __d = (d); \ (void) (&__a == &__b); \ (void) (&__a == __d); \ __builtin_sub_overflow(__a, __b, __d); \ -}) +})) -#define check_mul_overflow(a, b, d) ({ \ +#define check_mul_overflow(a, b, d) __must_check_bool(({ \ typeof(a) __a = (a); \ typeof(b) __b = (b); \ typeof(d) __d = (d); \ (void) (&__a == &__b); \ (void) (&__a == __d); \ __builtin_mul_overflow(__a, __b, __d); \ -}) +})) #else @@ -190,21 +200,20 @@ }) -#define check_add_overflow(a, b, d) \ - __builtin_choose_expr(is_signed_type(typeof(a)), \ - __signed_add_overflow(a, b, d), \ - __unsigned_add_overflow(a, b, d)) +#define check_add_overflow(a, b, d) \ + __must_check_bool(__builtin_choose_expr(is_signed_type(typeof(a)), \ + __signed_add_overflow(a, b, d), \ + __unsigned_add_overflow(a, b, d))) -#define check_sub_overflow(a, b, d) \ - __builtin_choose_expr(is_signed_type(typeof(a)), \ - __signed_sub_overflow(a, b, d), \ - __unsigned_sub_overflow(a, b, d)) - -#define check_mul_overflow(a, b, d) \ - __builtin_choose_expr(is_signed_type(typeof(a)), \ - __signed_mul_overflow(a, b, d), \ - __unsigned_mul_overflow(a, b, d)) +#define check_sub_overflow(a, b, d) \ + __must_check_bool(__builtin_choose_expr(is_signed_type(typeof(a)), \ + __signed_sub_overflow(a, b, d), \ + __unsigned_sub_overflow(a, b, d))) +#define check_mul_overflow(a, b, d) \ + __must_check_bool(__builtin_choose_expr(is_signed_type(typeof(a)), \ + __signed_mul_overflow(a, b, d), \ + __unsigned_mul_overflow(a, b, d))) #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */ @@ -227,7 +236,7 @@ * '*d' will hold the results of the attempted shift, but is not * considered "safe for use" if false is returned. */ -#define check_shl_overflow(a, s, d) ({ \ +#define check_shl_overflow(a, s, d) __must_check_bool(({ \ typeof(a) _a = a; \ typeof(s) _s = s; \ typeof(d) _d = d; \ @@ -237,7 +246,7 @@ *_d = (_a_full << _to_shift); \ (_to_shift != _s || is_negative(*_d) || is_negative(_a) || \ (*_d >> _to_shift) != _a); \ -}) +})) /** * array_size() - Calculate size of 2-dimensional array.
Since the destination variable of the check_*_overflow() helpers will contain a wrapped value on failure, it would be best to make sure callers really did check the return result of the helper. Adjust the macros to use a bool-wrapping static inline that is marked with __must_check. This means the macros can continue to have their type-agnostic behavior while gaining the function attribute (that cannot be applied directly to macros). Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> Signed-off-by: Kees Cook <keescook@chromium.org> --- include/linux/overflow.h | 51 +++++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 21 deletions(-)