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 <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
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
+ * 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.
--
2.25.1
--
Kees Cook
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 <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> 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 <[email protected]>
> > Signed-off-by: Kees Cook <[email protected]>
> > ---
> > 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).
--
Kees Cook