Re: [PATCH 3/4] KVM: arm64: Introduce CONFIG_UBSAN_KVM_EL2

[Kees Cook <kees@kernel.org>]

On Wed, Apr 16, 2025 at 06:04:33PM +0000, Mostafa Saleh wrote:
> Add a new Kconfig CONFIG_UBSAN_KVM_EL2 for KVM which enables
> UBSAN for EL2 code (in protected/nvhe/hvhe) modes.
> This will re-use the same checks enabled for the kernel for
> the hypervisor. The only difference is that for EL2 it always
> emits a "brk" instead of implementing hooks as the hypervisor
> can't print reports.
> 
> The KVM code will re-use the same code for the kernel
> "report_ubsan_failure()" so #ifdefs are changed to also have this
> code for CONFIG_UBSAN_KVM_EL2
> 
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  arch/arm64/kvm/hyp/nvhe/Makefile | 6 ++++++
>  include/linux/ubsan.h            | 2 +-
>  lib/Kconfig.ubsan                | 9 +++++++++
>  lib/ubsan.c                      | 6 ++++--
>  scripts/Makefile.ubsan           | 5 ++++-
>  5 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
> index b43426a493df..cbe7e12752bc 100644
> --- a/arch/arm64/kvm/hyp/nvhe/Makefile
> +++ b/arch/arm64/kvm/hyp/nvhe/Makefile
> @@ -99,3 +99,9 @@ KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_FTRACE) $(CC_FLAGS_SCS), $(KBUILD_CFLAG
>  # causes a build failure. Remove profile optimization flags.
>  KBUILD_CFLAGS := $(filter-out -fprofile-sample-use=% -fprofile-use=%, $(KBUILD_CFLAGS))
>  KBUILD_CFLAGS += -fno-asynchronous-unwind-tables -fno-unwind-tables
> +
> +ifeq ($(CONFIG_UBSAN_KVM_EL2),y)
> +UBSAN_SANITIZE := y
> +# Always use brk and not hooks
> +ccflags-y += $(CFLAGS_UBSAN_FOR_TRAP)
> +endif
> diff --git a/include/linux/ubsan.h b/include/linux/ubsan.h
> index c843816f5f68..3ab8d38aedb8 100644
> --- a/include/linux/ubsan.h
> +++ b/include/linux/ubsan.h
> @@ -2,7 +2,7 @@
>  #ifndef _LINUX_UBSAN_H
>  #define _LINUX_UBSAN_H
>  
> -#ifdef CONFIG_UBSAN_TRAP
> +#if defined(CONFIG_UBSAN_TRAP) || defined(CONFIG_UBSAN_KVM_EL2)
>  const char *report_ubsan_failure(u32 check_type);
>  #else
>  static inline const char *report_ubsan_failure(u32 check_type)
> diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> index 4216b3a4ff21..3878858eb473 100644
> --- a/lib/Kconfig.ubsan
> +++ b/lib/Kconfig.ubsan
> @@ -166,4 +166,13 @@ config TEST_UBSAN
>  	  This is a test module for UBSAN.
>  	  It triggers various undefined behavior, and detect it.
>  
> +config UBSAN_KVM_EL2
> +	bool "UBSAN for KVM code at EL2"
> +	depends on ARM64
> +	help
> +	  Enable UBSAN when running on ARM64 with KVM in a split mode
> +	  (nvhe/hvhe/protected) for the hypervisor code running in EL2.
> +	  In this mode, any UBSAN violation in EL2 would panic the kernel
> +	  and information similar to UBSAN_TRAP would be printed.
> +
>  endif	# if UBSAN
> diff --git a/lib/ubsan.c b/lib/ubsan.c
> index 17993727fc96..a6ca235dd714 100644
> --- a/lib/ubsan.c
> +++ b/lib/ubsan.c
> @@ -19,7 +19,7 @@
>  
>  #include "ubsan.h"
>  
> -#ifdef CONFIG_UBSAN_TRAP
> +#if defined(CONFIG_UBSAN_TRAP) || defined(CONFIG_UBSAN_KVM_EL2)
>  /*
>   * Only include matches for UBSAN checks that are actually compiled in.
>   * The mappings of struct SanitizerKind (the -fsanitize=xxx args) to
> @@ -97,7 +97,9 @@ const char *report_ubsan_failure(u32 check_type)
>  	}
>  }
>  
> -#else
> +#endif
> +
> +#ifndef CONFIG_UBSAN_TRAP
>  static const char * const type_check_kinds[] = {
>  	"load of",
>  	"store to",
> diff --git a/scripts/Makefile.ubsan b/scripts/Makefile.ubsan
> index 9e35198edbf0..68af6830af0f 100644
> --- a/scripts/Makefile.ubsan
> +++ b/scripts/Makefile.ubsan
> @@ -1,5 +1,8 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> +#Shared with KVM/arm64

Nitpick: Please add a space between "#" and "Shared", and end the line
with "."

> +export CFLAGS_UBSAN_FOR_TRAP := $(call cc-option,-fsanitize-trap=undefined,-fsanitize-undefined-trap-on-error)
> +
>  # Enable available and selected UBSAN features.
>  ubsan-cflags-$(CONFIG_UBSAN_ALIGNMENT)		+= -fsanitize=alignment
>  ubsan-cflags-$(CONFIG_UBSAN_BOUNDS_STRICT)	+= -fsanitize=bounds-strict
> @@ -10,7 +13,7 @@ ubsan-cflags-$(CONFIG_UBSAN_DIV_ZERO)		+= -fsanitize=integer-divide-by-zero
>  ubsan-cflags-$(CONFIG_UBSAN_UNREACHABLE)	+= -fsanitize=unreachable
>  ubsan-cflags-$(CONFIG_UBSAN_BOOL)		+= -fsanitize=bool
>  ubsan-cflags-$(CONFIG_UBSAN_ENUM)		+= -fsanitize=enum
> -ubsan-cflags-$(CONFIG_UBSAN_TRAP)		+= $(call cc-option,-fsanitize-trap=undefined,-fsanitize-undefined-trap-on-error)
> +ubsan-cflags-$(CONFIG_UBSAN_TRAP)		+= $(CFLAGS_UBSAN_FOR_TRAP)

Another minor style request: please name this "CFLAGS_UBSAN_TRAP"
(nothing else in Kconfig uses "FOR" like this, and leaving it off sounds
more declarative).

>  
>  export CFLAGS_UBSAN := $(ubsan-cflags-y)

Otherwise, yes, looks good.

-- 
Kees Cook