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

[Rasmus Villemoes <rasmus.villemoes@prevas.dk>]

On 30/01/2024 23.06, Kees Cook wrote:
> The check_add_overflow() helper is mostly a wrapper around
> __builtin_add_overflow(), but GCC and Clang refuse to operate on pointer
> arguments that would normally be allowed if the addition were open-coded.
> 
> For example, we have many places where pointer overflow is tested:
> 
> 	struct foo *ptr;
> 	...
> 	/* Check for overflow */
> 	if (ptr + count < ptr) ...
> 
> And in order to avoid running into the overflow sanitizers in the
> future, we need to rewrite these "intended" overflow checks:
> 
> 	if (check_add_overflow(ptr, count, &result)) ...
> 
> Frustratingly the argument type validation for __builtin_add_overflow()
> is done before evaluating __builtin_choose_expr(), so for arguments to
> be valid simultaneously for sizeof(*p) (when p may not be a pointer),
> and __builtin_add_overflow(a, ...) (when a may be a pointer), we must
> introduce wrappers that always produce a specific type (but they are
> only used in the places where the bogus arguments will be ignored).
> 
> To test whether a variable is a pointer or not, introduce the __is_ptr()
> helper, which uses __builtin_classify_type() to find arrays and pointers
> (via the new __is_ptr_or_array() helper), and then decays arrays into
> pointers (via the new __decay() helper), to distinguish pointers from
> arrays.
> 
> Additionally update the unit tests to cover pointer addition.
> 
> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Bill Wendling <morbo@google.com>
> Cc: Justin Stitt <justinstitt@google.com>
> Cc: llvm@lists.linux.dev
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/compiler_types.h | 10 +++++
>  include/linux/overflow.h       | 44 ++++++++++++++++++-
>  lib/overflow_kunit.c           | 77 ++++++++++++++++++++++++++++++----
>  3 files changed, 120 insertions(+), 11 deletions(-)
> 
> 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.

__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)

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.

Rasmus