Received-SPF: pass (google.com: domain of linux-kernel+bounces-49576-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 2 Feb 2024 01:26:09 -0800
From: Kees Cook <keescook@chromium.org>
To: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Nathan Chancellor <nathan@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Bill Wendling <morbo@google.com>,
	Justin Stitt <justinstitt@google.com>, llvm@lists.linux.dev,
	linux-hardening@vger.kernel.org,
	Mark Rutland <mark.rutland@arm.com>,
	Miguel Ojeda <ojeda@kernel.org>, Marco Elver <elver@google.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Przemek Kitszel <przemyslaw.kitszel@intel.com>,
	Masahiro Yamada <masahiroy@kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/5] overflow: Expand check_add_overflow() for pointer
 addition
Message-ID: <202402020105.0759D4CD@keescook>
References: <20240130220218.it.154-kees@kernel.org>
 <20240130220614.1154497-2-keescook@chromium.org>
 <6d66deda-e09d-4899-b3a3-5137eeee449c@prevas.dk>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6d66deda-e09d-4899-b3a3-5137eeee449c@prevas.dk>

On Wed, Jan 31, 2024 at 09:35:35AM +0100, Rasmus Villemoes wrote:
> On 30/01/2024 23.06, Kees Cook wrote:
> > [...]
> > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> > index 6f1ca49306d2..d27b58fddfaa 100644
> > --- a/include/linux/compiler_types.h
> > +++ b/include/linux/compiler_types.h
> > @@ -375,6 +375,16 @@ struct ftrace_likely_data {
> >  /* Are two types/vars the same type (ignoring qualifiers)? */
> >  #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
> >  
> > +/* Is variable addressable? */
> > +#define __is_ptr_or_array(p)	(__builtin_classify_type(p) == 5)
> 
> That magic constant is a bit ugly, but I don't think there's a better
> way. However, a comment saying "pointer_type_class in gcc/typeclass.h in
> gcc source code" or something like that might help. Do we know for sure
> that clang uses the same value? I can't find it documented anywhere.

Very true. Offlist, Keith Packard suggested I switch to this, so we can
avoid the constant:

+#define __is_ptr_or_array(p)	(__builtin_classify_type(p) == \
				 __builtin_classify_type(NULL))

> 
> __check_ptr_add_overflow() - Calculate pointer addition with overflow
> checking
> > + * @a: pointer addend
> > + * @b: numeric addend
> > + * @d: pointer to store sum
> > + *
> > + * Returns 0 on success, 1 on wrap-around.
> > + *
> > + * Do not use this function directly, use check_add_overflow() instead.
> > + *
> > + * *@d holds the results of the attempted addition, which may wrap-around.
> > + */
> > +#define __check_ptr_add_overflow(a, b, d)		\
> > +	({						\
> > +		typeof(a) __a = (a);			\
> > +		typeof(b) __b = (b);			\
> > +		size_t __bytes;				\
> > +		bool __overflow;			\
> > +							\
> > +		/* we want to perform the wrap-around, but retain the result */ \
> > +		__overflow = __builtin_mul_overflow(sizeof(*(__a)), __b, &__bytes); \
> > +		__builtin_add_overflow((unsigned long)(__a), __bytes, (unsigned long *)(d)) || \
> > +		__overflow;				\
> > +	})
> 
> So I've tried to wrap my head around all these layers of macros, and it
> seems ok. However, here I'm a bit worried that there's no type checking
> of the destination. In particular, the user could perhaps end up doing
> 
>   check_add_overflow(p, x, p)

I tried to make sure the top-level filtering would require a pointer to
an integral type. I'm sure there is a way to foot-gun it, if one tries
hard enough. :)

> 
> which will go horribly wrong. Do we have any infrastructure for testing
> "this should fail to compile"? It would be good to have, not just for
> this, but in general for checking our sanity checks.
> 
> Another thing is that this will always fail with negative offsets (if b
> has signed type and a negative value, the multiplication won't fit in an
> unsigned type). I think __bytes should be ptrdiff_t.

Ew. A negative "add"... yes. I'll take a closer look.

Thanks for the review!

As it turns out, I may not need this patch at all yet, so I may hold off
on it until I can prove that we really will need it.

-Kees

-- 
Kees Cook