linux-hardening - Re: [PATCH v2] overflow: Allow mixed type arguments

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 30 Aug 2022 11:42:19 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Andrzej Hajda <andrzej.hajda@...el.com>
Cc:     Rasmus Villemoes <linux@...musvillemoes.dk>,
        Gwan-gyeong Mun <gwan-gyeong.mun@...el.com>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        linux-hardening@...r.kernel.org,
        Daniel Latypov <dlatypov@...gle.com>,
        Vitor Massaru Iha <vitor@...saru.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] overflow: Allow mixed type arguments

On Tue, Aug 30, 2022 at 08:40:25AM +0200, Andrzej Hajda wrote:
> 
> 
> On 29.08.2022 23:45, Kees Cook wrote:
> > When the check_[op]_overflow() helpers were introduced, all arguments were
> > required to be the same type to make the fallback macros simpler. However,
> > once the fallback macros were removed[1], it is fine to allow mixed
> > types, which makes using the helpers much more useful, as they can be
> > used to test for type-based overflows (e.g. adding two large ints but
> > storing into a u8), as would be handy in the drm core[2].
> > 
> > Remove the restriction, and add additional self-tests that exercise some
> > of the mixed-type overflow cases.
> > 
> > [1] https://git.kernel.org/linus/4eb6bd55cfb22ffc20652732340c4962f3ac9a91
> > [2] https://lore.kernel.org/lkml/20220824084514.2261614-2-gwan-gyeong.mun@intel.com
> > 
> > Cc: Rasmus Villemoes <linux@...musvillemoes.dk>
> > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@...el.com>
> > Cc: Andrzej Hajda <andrzej.hajda@...el.com>
> > Cc: "Gustavo A. R. Silva" <gustavoars@...nel.org>
> > Cc: Nick Desaulniers <ndesaulniers@...gle.com>
> > Cc: linux-hardening@...r.kernel.org
> > Signed-off-by: Kees Cook <keescook@...omium.org>
> > ---
> > v2: change names to "type1_type2__output-type" for better readability (Rasmus)
> > v1: https://lore.kernel.org/lkml/20220829204729.3409270-1-keescook@chromium.org
> > ---
> >   include/linux/overflow.h |  6 ----
> >   lib/overflow_kunit.c     | 77 +++++++++++++++++++++++++++++-----------
> >   2 files changed, 57 insertions(+), 26 deletions(-)
> > 
> > diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> > index 0eb3b192f07a..ad692fb11bf3 100644
> > --- a/include/linux/overflow.h
> > +++ b/include/linux/overflow.h
> > @@ -63,8 +63,6 @@ static inline bool __must_check __must_check_overflow(bool overflow)
> >   	typeof(a) __a = (a);			\
> >   	typeof(b) __b = (b);			\
> >   	typeof(d) __d = (d);			\
> > -	(void) (&__a == &__b);			\
> > -	(void) (&__a == __d);			\
> >   	__builtin_add_overflow(__a, __b, __d);	\
> >   }))
> 
> Is there a reason to keep all local vars?
> Why not:
> #define check_add_overflow(a, b, d)
> __must_check_overflow(__builtin_add_overflow((a), (b), (d)))

Excellent point! Those were there to avoid side-effects when doing the
type comparisons in the macro, which are being removed too. :P

-- 
Kees Cook