Re: [x86] 1099ce55b0: BUG:kernel_NULL_pointer_dereference,address

[Kees Cook <keescook@chromium.org>]

On Mon, Feb 07, 2022 at 04:23:06PM -0800, Nick Desaulniers wrote:
> On Fri, Feb 4, 2022 at 4:37 AM kernel test robot <oliver.sang@intel.com> wrote:
> > FYI, we noticed the following commit (built with clang-15):
> >
> > commit: 1099ce55b0530ff429312dc37362ad43aee8c5c0 ("x86: don't build CONFIG_X86_32 as -ffreestanding")
> > https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git for-next/memcpy
> >
> > in testcase: boot
> [...]
> I've been having a hard time pinpointing via bisection when this
> stopped working.  I suspect it's actually the change on llvm's side
> that would replace memcmp with bcmp.  With this diff, we can boot
> ARCH=i386 defconfig

Thanks for spending the time on this!

> 
> ```
> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> index 7ef211865239..5e4570495206 100644
> --- a/arch/x86/Makefile
> +++ b/arch/x86/Makefile
> @@ -88,6 +88,8 @@ ifeq ($(CONFIG_X86_32),y)
>          include $(srctree)/arch/x86/Makefile_32.cpu
>          KBUILD_CFLAGS += $(cflags-y)
> 
> +        KBUILD_CFLAGS += -fno-builtin-bcmp
> +
>         ifeq ($(CONFIG_STACKPROTECTOR),y)
>                 ifeq ($(CONFIG_SMP),y)
>                         KBUILD_CFLAGS +=
> -mstack-protector-guard-reg=fs
> -mstack-protector-guard-symbol=__stack_chk_guard
> ```
> 
> It looks like the call argument setup in the _callers_ of memcmp is messed up.
> 
> Before:
> pushl %ecx
> pushl %ebx
> pushl -24(%ebp)
> calll bcmp
> 
> After:
> movl  %ebx, %eax
> movl  %esi, %edx
> movl  %ecx, %ebx
> calll memcmp
> 
> it looks like they're not obeying `-mregparm=3`.
> 
> https://godbolt.org/z/z3fjveP4h
> 
> Diffing the IR between `-mregparm=3` vs not, it looks like there's an
> LLVM IR function argument attribute inreg.
> https://llvm.org/docs/LangRef.html#parameter-attributes
> >> This indicates that this parameter or return value should be treated in a
> >> special target-dependent fashion while emitting code for a function call
> >> or return (usually, by putting it in a register as opposed to memory,
> >> though some targets use it to distinguish between two different kinds of
> >> registers). Use of this attribute is target-specific.
> 
> As is tradition, instcombine is not checking+carrying over the
> function argument attributes when replacing calls to memcmp w/ bcmp.
> 
> Before:
>   %4 = call i32 @memcmp(i8* inreg noundef %3, i8* inreg noundef %0,
> i32 inreg noundef %1) #4, !dbg !22
> 
> After:
>   %bcmp = call i32 @bcmp(i8* %3, i8* %0, i32 %1), !dbg !22
> 
> Filed:
> https://github.com/llvm/llvm-project/issues/53645
> 
> So I think the best course of action is to disable memcmp to bcmp
> BEFORE the removal of -ffreestanding, and only for clang until we have
> a fix in hand.

What do you mean about BEFORE the removal of -ffreestanding? As in, add
two patches, one to add -fno-builtin-bcmp and the next to remove
-ffreestanding? i.e.:


diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index e84cdd409b64..c92f69e916b4 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -90,6 +90,9 @@ ifeq ($(CONFIG_X86_32),y)
 
         # temporary until string.h is fixed
         KBUILD_CFLAGS += -ffreestanding
+        ifdef CONFIG_CC_IS_CLANG
+                KBUILD_CFLAGS += -fno-builtin-bcmp
+        endif
 
 	ifeq ($(CONFIG_STACKPROTECTOR),y)
 		ifeq ($(CONFIG_SMP),y)


then:



diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index c92f69e916b4..f56936aeed9e 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -88,8 +88,6 @@ ifeq ($(CONFIG_X86_32),y)
         include $(srctree)/arch/x86/Makefile_32.cpu
         KBUILD_CFLAGS += $(cflags-y)
 
-        # temporary until string.h is fixed
-        KBUILD_CFLAGS += -ffreestanding
         ifdef CONFIG_CC_IS_CLANG
                 KBUILD_CFLAGS += -fno-builtin-bcmp
         endif

?

-- 
Kees Cook