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

Message

Kees Cook Sept. 2, 2022, 8:43 p.m. UTC

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 is the actual fix, using a 0-index __builtin_constant_p() check
to short-circuit the runtime check.
Patch 2 is a KUnit test to validate this behavior going forward.
Patch 3 is is a cosmetic cleanup to use SIZE_MAX instead of (size_t)-1

-Kees

v2:
 - different solution
 - add KUnit test
 - expand scope of cosmetic cleanup
v1: https://lore.kernel.org/lkml/[e-mail]

Kees Cook (3):
  fortify: Fix __compiletime_strlen() under UBSAN_BOUNDS_LOCAL
  fortify: Add KUnit test for FORTIFY_SOURCE internals
  fortify: Use SIZE_MAX instead of (size_t)-1

 MAINTAINERS                    |  1 +
 include/linux/fortify-string.h | 29 ++++++-------
 lib/Kconfig.debug              |  9 ++++
 lib/Makefile                   |  1 +
 lib/fortify_kunit.c            | 77 ++++++++++++++++++++++++++++++++++
 5 files changed, 103 insertions(+), 14 deletions(-)
 create mode 100644 lib/fortify_kunit.c

Comments

Nick Desaulniers Sept. 6, 2022, 4:37 p.m. UTC | #1

On Fri, Sep 2, 2022 at 1:43 PM Kees Cook <[e-mail]> 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().
>
> __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 is the actual fix, using a 0-index __builtin_constant_p() check
> to short-circuit the runtime check.
> Patch 2 is a KUnit test to validate this behavior going forward.
> Patch 3 is is a cosmetic cleanup to use SIZE_MAX instead of (size_t)-1

Thanks,
Testing out patch 1/3 against Android's CTS:
https://android-review.googlesource.com/c/kernel/common/+/2206839,
will give formal signoffs/review after a completed test run.

>
> -Kees
>
> v2:
>  - different solution
>  - add KUnit test
>  - expand scope of cosmetic cleanup
> v1: https://lore.kernel.org/lkml/[e-mail]
>
> Kees Cook (3):
>   fortify: Fix __compiletime_strlen() under UBSAN_BOUNDS_LOCAL
>   fortify: Add KUnit test for FORTIFY_SOURCE internals
>   fortify: Use SIZE_MAX instead of (size_t)-1
>
>  MAINTAINERS                    |  1 +
>  include/linux/fortify-string.h | 29 ++++++-------
>  lib/Kconfig.debug              |  9 ++++
>  lib/Makefile                   |  1 +
>  lib/fortify_kunit.c            | 77 ++++++++++++++++++++++++++++++++++
>  5 files changed, 103 insertions(+), 14 deletions(-)
>  create mode 100644 lib/fortify_kunit.c
>
> --
> 2.34.1
>