[0/3] Fix FORTIFY=y UBSAN_LOCAL_BOUNDS=y

With CONFIG_FORTIFY=y and CONFIG_UBSAN_LOCAL_BOUNDS=y enabled, we
observe a runtime panic while running Android's Compatibility Test
Suite's (CTS) android.hardware.input.cts.tests.  This is stemming from a
strlen() call in hidinput_allocate().

__builtin_object_size(str, 0 or 1) has interesting behavior for C
strings when str is runtime dependent, and all possible values are known
at compile time; it evaluates to the maximum of those sizes. This causes
UBSAN_LOCAL_BOUNDS to insert faults for the smaller values, which we
trip at runtime.

Patch 1 adds a KCONFIG version check for __builtin_dynamic_object_size,
and uses that in __compiletime_strlen rather than __builtin_object_size.
Patch 2 and 3 are cosmetic cleanups, they're not as important to me as
patch 1 is.

Nick Desaulniers (3):
  fortify: use __builtin_dynamic_object_size in __compiletime_strlen
  fortify: cosmetic cleanups to __compiletime_strlen
  HID: avoid runtime call to strlen

 drivers/hid/hid-input.c        | 13 ++++++++++++-
 include/linux/fortify-string.h | 15 ++++++++++-----
 init/Kconfig                   |  3 +++
 3 files changed, 25 insertions(+), 6 deletions(-)

On Tue, Aug 30, 2022 at 01:53:09PM -0700, Nick Desaulniers wrote:
> While looking into a CONFIG_FORTIFY=y related bug, I noticed that
> hid_allocate calls strlen() on a local C string variable. This variable
> can only have literal string values. There is no benefit to having
> FORTIFY have this be a checked strlen call, because these are literal
> values.  By calling strlen() explicitly in the branches of a switch, the
> compiler can evaluate strlen("literal value") at compile time, rather
> than at runtime.
> 
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  drivers/hid/hid-input.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 48c1c02c69f4..9ad3cc88c26b 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1922,12 +1922,15 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid,
>  		switch (application) {
>  		case HID_GD_KEYBOARD:
>  			suffix = "Keyboard";
> +			suffix_len = strlen(suffix);
>  			break;
>  		case HID_GD_KEYPAD:
>  			suffix = "Keypad";
> +			suffix_len = strlen(suffix);
>  			break;
>  		case HID_GD_MOUSE:
>  			suffix = "Mouse";
> +			suffix_len = strlen(suffix);

<snip>

This seems ripe for someone to come along and go "look at this cleanup
patch where I move all of this duplicated code to below it in one line!"

As this is a compiler bug, why not fix the compiler?  Or at least put a
comment in here saying why this looks so odd to prevent it from being
changed in the future.

thanks,

greg k-h

On Tue, Aug 30, 2022 at 01:53:07PM -0700, Nick Desaulniers wrote:
> With CONFIG_FORTIFY=y and CONFIG_UBSAN_LOCAL_BOUNDS=y enabled, we
> observe a runtime panic while running Android's Compatibility Test
> Suite's (CTS) android.hardware.input.cts.tests.  This is stemming from a
> strlen() call in hidinput_allocate().
> 
> __compiletime_strlen is implemented in terms of __builtin_object_size(),
> then does an array access to check for NUL-termination. A quirk of
> __builtin_object_size() is that for strings whose values are runtime
> dependent, __builtin_object_size(str, 1 or 0) returns the maximum size
> of possible values when those sizes are determinable at compile time.
> Example:
> 
>   static const char *v = "FOO BAR";
>   static const char *y = "FOO BA";
>   unsigned long x (int z) {
>       // Returns 8, which is:
>       // max(__builtin_object_size(v, 1), __builtin_object_size(y, 1))
>       return __builtin_object_size(z ? v : y, 1);
>   }
> 
> So when FORTIFY is enabled, the current implementation of
> __compiletime_strlen will try to access beyond the end of y at runtime
> using the size of v. Mixed with UBSAN_LOCAL_BOUNDS we get a fault.
> 
> hidinput_allocate() has a local C string whose value is control flow
> dependent on a switch statement, so __builtin_object_size(str, 1)
> evaluates to the maximum string length, making all other cases fault on
> the last character check. hidinput_allocate() could be cleaned up to
> avoid runtime calls to strlen() since the local variable can only have
> literal values, so there's no benefit to trying to fortify the strlen
> call site there.
> 
> Add a Kconfig check for __builtin_dynamic_object_size(), then use that
> when available (gcc-12+, all supported versions of clang) which avoids
> this surprising behavior.
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  include/linux/fortify-string.h | 8 +++++++-
>  init/Kconfig                   | 3 +++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index 3b401fa0f374..c5adad596a3f 100644
> --- a/include/linux/fortify-string.h
> +++ b/include/linux/fortify-string.h
> @@ -14,11 +14,17 @@ void __read_overflow2_field(size_t avail, size_t wanted) __compiletime_warning("
>  void __write_overflow(void) __compiletime_error("detected write beyond size of object (1st parameter)");
>  void __write_overflow_field(size_t avail, size_t wanted) __compiletime_warning("detected write beyond size of field (1st parameter); maybe use struct_group()?");
>  
> +#ifdef CONFIG_CC_HAS_BUILTIN_DYNAMIC_OBJECT_SIZE
> +#define __object_size __builtin_dynamic_object_size
> +#else
> +#define __object_size __builtin_object_size
> +#endif

Instead of a Kconfig, how about just:

#if __has_builtin(__builtin_dynamic_object_size)
# define __object_size __builtin_dynamic_object_size
#else
# define __object_size __builtin_object_size
#endif

?

> +
>  #define __compiletime_strlen(p)					\
>  ({								\
>  	unsigned char *__p = (unsigned char *)(p);		\
>  	size_t __ret = (size_t)-1;				\
> -	size_t __p_size = __builtin_object_size(p, 1);		\
> +	size_t __p_size = __object_size(p, 1);			\
>  	if (__p_size != (size_t)-1) {				\
>  		size_t __p_len = __p_size - 1;			\
>  		if (__builtin_constant_p(__p[__p_len]) &&	\

The fact that __builtin_object_size() will actually span control flow,
and produce a size-inclusive result on the possible inputs is ...
surprising and potentially quite problematic. But I'm satisfied that
bdos appears to fix it here (since the "compiletime"ness will still get
filtered by the __builtin_constant_p() check).

-Kees