Hi Kees, On Fri, Feb 23, 2024 at 03:32:37PM -0800, Kees Cook wrote: > On Fri, Feb 23, 2024 at 07:26:46PM +0100, Sam Ravnborg wrote: > > Hi Kees, > > > > On Fri, Feb 23, 2024 at 08:59:45AM -0800, Kees Cook wrote: > > > The UBSAN instrumentation cannot work in the vDSO since it is executing > > > in userspace, so disable it in the Makefile. Fixes the build failures > > > such as: > > > > > > arch/sparc/vdso/vclock_gettime.c:217: undefined reference to `__ubsan_handle_shift_out_of_bounds' > > > > > > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx> > > > --- > > > Cc: "David S. Miller" <davem@xxxxxxxxxxxxx> > > > Cc: Andreas Larsson <andreas@xxxxxxxxxxx> > > > Cc: Masahiro Yamada <masahiroy@xxxxxxxxxx> > > > Cc: Sam Ravnborg <sam@xxxxxxxxxxxx> > > > Cc: Helge Deller <deller@xxxxxx> > > > Cc: Guo Ren <guoren@xxxxxxxxxx> > > > Cc: sparclinux@xxxxxxxxxxxxxxx > > > --- > > > arch/sparc/vdso/Makefile | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/arch/sparc/vdso/Makefile b/arch/sparc/vdso/Makefile > > > index 7f5eedf1f5e0..e8aef2c8ae99 100644 > > > --- a/arch/sparc/vdso/Makefile > > > +++ b/arch/sparc/vdso/Makefile > > > @@ -2,6 +2,7 @@ > > > # > > > # Building vDSO images for sparc. > > > # > > > +UBSAN_SANITIZE := n > > > > When I read: > > > > config UBSAN_SANITIZE_ALL > > bool "Enable instrumentation for the entire kernel" > > depends on ARCH_HAS_UBSAN_SANITIZE_ALL > > default y > > help > > This option activates instrumentation for the entire kernel. > > If you don't enable this option, you have to explicitly specify > > UBSAN_SANITIZE := y for the files/directories you want to check for UB. > > Enabling this option will get kernel image size increased > > significantly. > > > > > > I am left with the understanding that only arch's that > > selects ARCH_HAS_UBSAN_SANITIZE_ALL would need to turn off > > UBSAN_SANITIZE. > > Ah, right. So, I removed[1] UBSAN_SANITIZE_ALL in -next (it was the only > sanitizer using this logic) and this appears to be one of the impacts. :) > I sent similar fixes for sh[2] and LoongArch[3]. > > > Are this fix papering over some other bug where we enable > > UBSAN_SANITIZE_ALL for arch's that should not have it, > > or something else that enable it? > > It's possible we should implement HAVE_ARCH_UBSAN, but in my testing > everything built fine with it, so I didn't opt to do that (it looked > like just additional configs for no real benefit). What do you think? Coffee has not yet kicked in, but... > [1] https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/kspp&id=918327e9b7ffb45321cbb4b9b86b58ec555fe6b3 OK, I did not have this patch in my tree so it explain the need for the patch in this mail. Looking at the linked patch the ARCH_HAS_UBSAN symbol is selected by some architecture but I see no use of it. Maybe that is a later patch and then all is good. In general I am not fan of naked config symbols (no help / comment) like this: config ARCH_HAS_UBSAN bool The reader is left only with the symbol name trying to understand the purpose of a symbol that is selected by some architectures. But that is a different matter for another day. As you now put the patch in this mail in context it makes sense and it has my: Acked-by: Sam Ravnborg <sam@xxxxxxxxxxxx> Sam
