Re: [PATCH v2 2/5] overflow: Expand check_add_overflow() for pointer addition

[Kees Cook <keescook@chromium.org>]

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