lkml.org 
[lkml]   [2020]   [Aug]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] overflow: Add __must_check attribute to check_*() helpers
    From
    Date
    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

    \
     
     \ /
      Last update: 2020-08-13 08:40    [W:19.718 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site