Messages in this thread | | | Date | Fri, 2 Feb 2024 01:26:09 -0800 | From | Kees Cook <> | Subject | Re: [PATCH v2 2/5] overflow: Expand check_add_overflow() for pointer addition |
| |
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
| |