| Message ID | 20240223165942.work.950-kees@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | sparc: vdso: Disable UBSAN instrumentation | expand |
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@chromium.org> > --- > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Andreas Larsson <andreas@gaisler.com> > Cc: Masahiro Yamada <masahiroy@kernel.org> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Helge Deller <deller@gmx.de> > Cc: Guo Ren <guoren@kernel.org> > Cc: sparclinux@vger.kernel.org > --- > 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. 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? Sam
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@chromium.org> > > --- > > Cc: "David S. Miller" <davem@davemloft.net> > > Cc: Andreas Larsson <andreas@gaisler.com> > > Cc: Masahiro Yamada <masahiroy@kernel.org> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: Helge Deller <deller@gmx.de> > > Cc: Guo Ren <guoren@kernel.org> > > Cc: sparclinux@vger.kernel.org > > --- > > 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? -Kees [1] https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/kspp&id=918327e9b7ffb45321cbb4b9b86b58ec555fe6b3 [2] https://lore.kernel.org/lkml/20240130232717.work.088-kees@kernel.org/ [3] https://lore.kernel.org/lkml/20240130233140.work.887-kees@kernel.org/
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@chromium.org> > > > --- > > > Cc: "David S. Miller" <davem@davemloft.net> > > > Cc: Andreas Larsson <andreas@gaisler.com> > > > Cc: Masahiro Yamada <masahiroy@kernel.org> > > > Cc: Sam Ravnborg <sam@ravnborg.org> > > > Cc: Helge Deller <deller@gmx.de> > > > Cc: Guo Ren <guoren@kernel.org> > > > Cc: sparclinux@vger.kernel.org > > > --- > > > 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@ravnborg.org> Sam
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@chromium.org> > --- > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Andreas Larsson <andreas@gaisler.com> > Cc: Masahiro Yamada <masahiroy@kernel.org> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Helge Deller <deller@gmx.de> > Cc: Guo Ren <guoren@kernel.org> > Cc: sparclinux@vger.kernel.org I dunno how you applied patches, but these Cc seems to appear in a few commits in your hardening branch. I formatted patch from 9fd54b08040669, checked out the new branch just before this commit and run `git am 0001-...`. I don't see them.
On Thu, Feb 29, 2024 at 10:00:38PM +0200, Andy Shevchenko wrote: > 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@chromium.org> > > --- > > Cc: "David S. Miller" <davem@davemloft.net> > > Cc: Andreas Larsson <andreas@gaisler.com> > > Cc: Masahiro Yamada <masahiroy@kernel.org> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: Helge Deller <deller@gmx.de> > > Cc: Guo Ren <guoren@kernel.org> > > Cc: sparclinux@vger.kernel.org > > I dunno how you applied patches, but these Cc seems to appear in a few commits > in your hardening branch. > > I formatted patch from 9fd54b08040669, checked out the new branch just before > this commit and run `git am 0001-...`. I don't see them. Ah, hm, yes, I'll need to split up my trees a bit to get the right results. Thanks for pointing that out!
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 # files to link into the vdso vobjs-y := vdso-note.o vclock_gettime.o
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@chromium.org> --- Cc: "David S. Miller" <davem@davemloft.net> Cc: Andreas Larsson <andreas@gaisler.com> Cc: Masahiro Yamada <masahiroy@kernel.org> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Helge Deller <deller@gmx.de> Cc: Guo Ren <guoren@kernel.org> Cc: sparclinux@vger.kernel.org --- arch/sparc/vdso/Makefile | 1 + 1 file changed, 1 insertion(+)