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

[Mostafa Saleh <smostafa@google.com>]

On Wed, Apr 16, 2025 at 12:54:21PM -0700, Kees Cook wrote:
> 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 "."

I will fix it in v2.

> 
> > +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).
I will fix it also in v2.

> 
> >  
> >  export CFLAGS_UBSAN := $(ubsan-cflags-y)
> 
> Otherwise, yes, looks good.
> 
> -- 
> Kees Cook

Thanks,
Mostafa