Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 30 Aug 2022 11:42:19 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Andrzej Hajda <andrzej.hajda@intel.com>
Cc:     Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        linux-hardening@vger.kernel.org,
        Daniel Latypov <dlatypov@google.com>,
        Vitor Massaru Iha <vitor@massaru.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] overflow: Allow mixed type arguments
Message-ID: <202208301141.8CA10CA@keescook>
References: <20220829214550.3443472-1-keescook@chromium.org>
 <8d880425-72f5-6ba6-933f-b8b016da88cd@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8d880425-72f5-6ba6-933f-b8b016da88cd@intel.com>
Precedence: bulk

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@rasmusvillemoes.dk>
> > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> > Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Cc: linux-hardening@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.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