[CONFIG_STACKPROTECTOR] Wrong instruction was generated when the customized stack protector is enabled for i386

[bysui]

CLANG version:
clang version 17.0.0 (https://github.com/llvm/llvm-project.git e369577cd0585d928cad1edfa7d546f3f6750f39)

Hello.

After llvm/llvm-project#60116 was resolved, the patch for using per-cpu stack protector for X86_64 still couldn't work due to wrong instructions being generated in some files. After diving deep into the issue, I was able to reproduce it.

The compile command is:
clang -S -c -m64 -O0 -mcmodel=kernel -fno-PIE -fstack-protector -mstack-protector-guard-reg=gs -mstack-protector-guard-symbol=__stack_chk_guard test.c -o test.s

I found that if there is a definition of "_stack_chk_guard" in the file:

unsigned long __stack_chk_guard;

int foo(void)
{
	char X[200];

	return 3;
}

The output assembly is:

        .......
	movq	__stack_chk_guard(%rip), %rax
	movq	%rax, -8(%rbp)
	movq	__stack_chk_guard(%rip), %rax
	movq	-8(%rbp), %rcx
       .......

It would generate wrong instruction without %gs.

If there is a declaration of "__stack_chk_guard" in the file:

extern unsigned long __stack_chk_guard;

int foo(void)
{
	char X[200];

	return 3;
}

The output assembly is:

        .......
	movq	%gs:__stack_chk_guard(%rip), %rax
	movq	%rax, -8(%rbp)
       .......

It would generate right instruction.

However, If there is a reference of "__stack_chk_guard" in the file:

extern unsigned long __stack_chk_guard;

void test(void)
{
	__stack_chk_guard = 1;
}

int foo(void)
{
	char X[200];

	return 3;
}

The output assembly is:

        .......
	movq	__stack_chk_guard(%rip), %rax
	movq	%rax, -8(%rbp)
       .......

It would generate wrong instruction without %gs too.

This is why I found the wrong instruction in some files where "__stack_chk_guard" was set up.

[nickdesaulniers]

cc @phoebewang @MaskRay

[nickdesaulniers]

cc @xiangzh1

[MaskRay]

Not a bug. This is invalid input. By specifying -mstack-protector-guard-reg=gs, you assert that __stack_chk_guard is a TLS symbol. If you define unsigned long __stack_chk_guard; or reference it as a non-TLS symbol, there will be a type mismatch. Clang doesn't report an error (it's kinda difficult to use a proper error reporting mechanism to surface an error from LLVMCodeGen to Clang) but the user should avoid such case.

[bysui]

Not a bug. This is invalid input. By specifying -mstack-protector-guard-reg=gs, you assert that __stack_chk_guard is a TLS symbol. If you define unsigned long __stack_chk_guard; or reference it as a non-TLS symbol, there will be a type mismatch. Clang doesn't report an error (it's kinda difficult to use a proper error reporting mechanism to surface an error from LLVMCodeGen to Clang) but the user should avoid such case.

But it could work in GCC. In my patch, it is necessary to define a Per-CPU variable for __stack_chk_guard in one file, which is done by using DEFINE_PER_CPU(__stack_chk_guard). The stack canary is then set up using this_cpu_write() in another file, resulting in incorrect instructions in these two files. If such cases are not avoided in the kernel, how can the kernel utilize these options to implement a customized stack protector in CLANG？

extern unsigned long __stack_chk_guard;

void test(unsigned long canary)
{
	asm volatile("movq %[val], %%gs:%[var]"
		     : [var] "+m" (__stack_chk_guard)
		     : [val] "re" (canary));
}

int foo(int i)
{
	char X[200];

	X[i] = 1;

	return 3;
}

I tested this_cpu_write() too, it still generates wrong instruction.

[nickdesaulniers]

which is done by using DEFINE_PER_CPU(__stack_chk_guard).

What happens if __stack_chk_guard is not defined (since it is a symbol that's implied)?

[MaskRay]

But it could work in GCC. In my patch, it is necessary to define a Per-CPU variable for __stack_chk_guard in one file, which is done by using DEFINE_PER_CPU(__stack_chk_guard). The stack canary is then set up using this_cpu_write() in another file, resulting in incorrect instructions in these two files. If such cases are not avoided in the kernel, how can the kernel utilize these options to implement a customized stack protector in CLANG？

-mstack-protector-guard-reg={fs,gs} is not expected to be used with a non-TLS global variable.
You can see that -mstack-protector-guard=global nullifies -mstack-protector-guard-reg={fs,gs}.
See gcc/config/i386/i386.cc:ix86_stack_protect_guard.

Your case (implicit -mstack-protector-guard=tls) gets your desired output, but that is accidental.

I recommend that you define __stack_chk_guard in an assembly file or a C file disabling LTO.

[bysui]

which is done by using DEFINE_PER_CPU(__stack_chk_guard).

What happens if __stack_chk_guard is not defined (since it is a symbol that's implied)?

The link failed due to the undefined symbol "__stack_chk_guard".

ld.lld: error: undefined symbol: __stack_chk_guard
>>> referenced by usercopy_64.c
>>>               vmlinux.o:(trace_event_raw_event_initcall_level)
>>> referenced by usercopy_64.c
>>>               vmlinux.o:(trace_event_raw_event_initcall_level)
>>> referenced by usercopy_64.c
>>>               vmlinux.o:(perf_trace_initcall_level)

[bysui]

But it could work in GCC. In my patch, it is necessary to define a Per-CPU variable for __stack_chk_guard in one file, which is done by using DEFINE_PER_CPU(__stack_chk_guard). The stack canary is then set up using this_cpu_write() in another file, resulting in incorrect instructions in these two files. If such cases are not avoided in the kernel, how can the kernel utilize these options to implement a customized stack protector in CLANG？

-mstack-protector-guard-reg={fs,gs} is not expected to be used with a non-TLS global variable. You can see that -mstack-protector-guard=global nullifies -mstack-protector-guard-reg={fs,gs}. See gcc/config/i386/i386.cc:ix86_stack_protect_guard.

Your case (implicit -mstack-protector-guard=tls) gets your desired output, but that is accidental.

I recommend that you define __stack_chk_guard in an assembly file or a C file disabling LTO.

Actually, x86_32 had already used the same options for the per-cpu stack protector in the mainline (commit: 3fb0fdb). Therefore, I believe it would also generate wrong instructions for the 32-bit kernel. However, it still booted successfully. When I used objdump on arch/x86/kernel/cpu/common.c, where the __stack_chk_guard symbol is defined, the generated instruction is:

000001b0 <load_direct_gdt>:
     1b0:       55                      push   %ebp
     1b1:       89 e5                   mov    %esp,%ebp
     1b3:       83 ec 0c                sub    $0xc,%esp
     1b6:       8b 0d 00 00 00 00       mov    0x0,%ecx
                        1b8: R_386_32   __stack_chk_guard
     1bc:       89 4d fc                mov    %ecx,-0x4(%ebp)

The compile command is:

savedcmd_arch/x86/kernel/cpu/common.o := clang -Wp,-MMD,arch/x86/kernel/cpu/.common.o.d  -nostdinc -I./arch/x86/include -I./arch/x86/include/generated  -I./include -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/compiler-version.h -include ./include/linux/kconfig.h -include ./include/linux/compiler_types.h -D__KERNEL__ -Werror -fmacro-prefix-map=./= -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Werror=return-type -Wno-format-security -funsigned-char -std=gnu11 --target=x86_64-linux-gnu -fintegrated-as -Werror=unknown-warning-option -Werror=ignored-optimization-argument -Werror=option-ignored -Werror=unused-command-line-argument -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -mno-avx -fcf-protection=none -m32 -msoft-float -mregparm=3 -freg-struct-return -fno-pic -mstack-alignment=4 -march=i686 -ffreestanding -mstack-protector-guard-reg=fs -mstack-protector-guard-symbol=__stack_chk_guard -Wno-sign-compare -fno-asynchronous-unwind-tables -mretpoline-external-thunk -mfunction-return=thunk-extern -fno-delete-null-pointer-checks -Wno-frame-address -Wno-address-of-packed-member -O2 -Wframe-larger-than=2048 -fstack-protector-strong -Wno-gnu -Wno-unused-but-set-variable -Wno-unused-const-variable -fno-omit-frame-pointer -fno-optimize-sibling-calls -fno-stack-clash-protection -mfentry -DCC_USING_FENTRY -falign-functions=4 -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wcast-function-type -Wimplicit-fallthrough -fno-strict-overflow -fno-stack-check -Werror=date-time -Werror=incompatible-pointer-types -Wno-initializer-overrides -Wno-format -Wformat-extra-args -Wformat-invalid-specifier -Wformat-zero-length -Wnonnull -Wformat-insufficient-args -Wno-sign-compare -Wno-pointer-to-enum-cast -Wno-tautological-constant-out-of-range-compare -Wno-unaligned-access    -DKBUILD_MODFILE='"arch/x86/kernel/cpu/common"' -DKBUILD_BASENAME='"common"' -DKBUILD_MODNAME='"common"' -D__KBUILD_MODNAME=kmod_common -c -o arch/x86/kernel/cpu/common.o arch/x86/kernel/cpu/common.c

Where GCC generates:

00000024 <load_direct_gdt>:
      24:       55                      push   %ebp
      25:       89 c2                   mov    %eax,%edx
      27:       89 e5                   mov    %esp,%ebp
      29:       83 ec 0c                sub    $0xc,%esp
      2c:       64 a1 00 00 00 00       mov    %fs:0x0,%eax
                        2e: R_386_32    __stack_chk_guard
      32:       89 45 fc                mov    %eax,-0x4(%ebp)

So it seems that GCC and CLANG generate different code for the 32-bit kernel as well. However, CLANG always generates non-TLS references for the 32-bit kernel, which is different from the 64-bit kernel that generates both non-TLS and TLS references. Therefore, the 32-bit kernel may boot by accident for CLANG, but I don't believe it is correct because it functions more like a global stack protector.

[bysui]

But it could work in GCC. In my patch, it is necessary to define a Per-CPU variable for __stack_chk_guard in one file, which is done by using DEFINE_PER_CPU(__stack_chk_guard). The stack canary is then set up using this_cpu_write() in another file, resulting in incorrect instructions in these two files. If such cases are not avoided in the kernel, how can the kernel utilize these options to implement a customized stack protector in CLANG？

-mstack-protector-guard-reg={fs,gs} is not expected to be used with a non-TLS global variable. You can see that -mstack-protector-guard=global nullifies -mstack-protector-guard-reg={fs,gs}. See gcc/config/i386/i386.cc:ix86_stack_protect_guard.
Your case (implicit -mstack-protector-guard=tls) gets your desired output, but that is accidental.
I recommend that you define __stack_chk_guard in an assembly file or a C file disabling LTO.

Actually, x86_32 had already used the same options for the per-cpu stack protector in the mainline (commit: 3fb0fdb). Therefore, I believe it would also generate wrong instructions for the 32-bit kernel. However, it still booted successfully. When I used objdump on arch/x86/kernel/cpu/common.c, where the __stack_chk_guard symbol is defined, the generated instruction is:

000001b0 <load_direct_gdt>:
     1b0:       55                      push   %ebp
     1b1:       89 e5                   mov    %esp,%ebp
     1b3:       83 ec 0c                sub    $0xc,%esp
     1b6:       8b 0d 00 00 00 00       mov    0x0,%ecx
                        1b8: R_386_32   __stack_chk_guard
     1bc:       89 4d fc                mov    %ecx,-0x4(%ebp)

The compile command is:

savedcmd_arch/x86/kernel/cpu/common.o := clang -Wp,-MMD,arch/x86/kernel/cpu/.common.o.d  -nostdinc -I./arch/x86/include -I./arch/x86/include/generated  -I./include -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/compiler-version.h -include ./include/linux/kconfig.h -include ./include/linux/compiler_types.h -D__KERNEL__ -Werror -fmacro-prefix-map=./= -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Werror=return-type -Wno-format-security -funsigned-char -std=gnu11 --target=x86_64-linux-gnu -fintegrated-as -Werror=unknown-warning-option -Werror=ignored-optimization-argument -Werror=option-ignored -Werror=unused-command-line-argument -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -mno-avx -fcf-protection=none -m32 -msoft-float -mregparm=3 -freg-struct-return -fno-pic -mstack-alignment=4 -march=i686 -ffreestanding -mstack-protector-guard-reg=fs -mstack-protector-guard-symbol=__stack_chk_guard -Wno-sign-compare -fno-asynchronous-unwind-tables -mretpoline-external-thunk -mfunction-return=thunk-extern -fno-delete-null-pointer-checks -Wno-frame-address -Wno-address-of-packed-member -O2 -Wframe-larger-than=2048 -fstack-protector-strong -Wno-gnu -Wno-unused-but-set-variable -Wno-unused-const-variable -fno-omit-frame-pointer -fno-optimize-sibling-calls -fno-stack-clash-protection -mfentry -DCC_USING_FENTRY -falign-functions=4 -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wcast-function-type -Wimplicit-fallthrough -fno-strict-overflow -fno-stack-check -Werror=date-time -Werror=incompatible-pointer-types -Wno-initializer-overrides -Wno-format -Wformat-extra-args -Wformat-invalid-specifier -Wformat-zero-length -Wnonnull -Wformat-insufficient-args -Wno-sign-compare -Wno-pointer-to-enum-cast -Wno-tautological-constant-out-of-range-compare -Wno-unaligned-access    -DKBUILD_MODFILE='"arch/x86/kernel/cpu/common"' -DKBUILD_BASENAME='"common"' -DKBUILD_MODNAME='"common"' -D__KBUILD_MODNAME=kmod_common -c -o arch/x86/kernel/cpu/common.o arch/x86/kernel/cpu/common.c

Where GCC generates:

00000024 <load_direct_gdt>:
      24:       55                      push   %ebp
      25:       89 c2                   mov    %eax,%edx
      27:       89 e5                   mov    %esp,%ebp
      29:       83 ec 0c                sub    $0xc,%esp
      2c:       64 a1 00 00 00 00       mov    %fs:0x0,%eax
                        2e: R_386_32    __stack_chk_guard
      32:       89 45 fc                mov    %eax,-0x4(%ebp)

So it seems that GCC and CLANG generate different code for the 32-bit kernel as well. However, CLANG always generates non-TLS references for the 32-bit kernel, which is different from the 64-bit kernel that generates both non-TLS and TLS references. Therefore, the 32-bit kernel may boot by accident for CLANG, but I don't believe it is correct because it functions more like a global stack protector.

Sorry for the confusion, I should clarify that CLANG generates both non-TLS and TLS references for the 32-bit kernel as well, but it still manages to boot successfully. I may need to figure out why this is the case.

[bysui]

But it could work in GCC. In my patch, it is necessary to define a Per-CPU variable for __stack_chk_guard in one file, which is done by using DEFINE_PER_CPU(__stack_chk_guard). The stack canary is then set up using this_cpu_write() in another file, resulting in incorrect instructions in these two files. If such cases are not avoided in the kernel, how can the kernel utilize these options to implement a customized stack protector in CLANG？

-mstack-protector-guard-reg={fs,gs} is not expected to be used with a non-TLS global variable. You can see that -mstack-protector-guard=global nullifies -mstack-protector-guard-reg={fs,gs}. See gcc/config/i386/i386.cc:ix86_stack_protect_guard.
Your case (implicit -mstack-protector-guard=tls) gets your desired output, but that is accidental.
I recommend that you define __stack_chk_guard in an assembly file or a C file disabling LTO.

Actually, x86_32 had already used the same options for the per-cpu stack protector in the mainline (commit: 3fb0fdb). Therefore, I believe it would also generate wrong instructions for the 32-bit kernel. However, it still booted successfully. When I used objdump on arch/x86/kernel/cpu/common.c, where the __stack_chk_guard symbol is defined, the generated instruction is:

000001b0 <load_direct_gdt>:
     1b0:       55                      push   %ebp
     1b1:       89 e5                   mov    %esp,%ebp
     1b3:       83 ec 0c                sub    $0xc,%esp
     1b6:       8b 0d 00 00 00 00       mov    0x0,%ecx
                        1b8: R_386_32   __stack_chk_guard
     1bc:       89 4d fc                mov    %ecx,-0x4(%ebp)

The compile command is:

savedcmd_arch/x86/kernel/cpu/common.o := clang -Wp,-MMD,arch/x86/kernel/cpu/.common.o.d  -nostdinc -I./arch/x86/include -I./arch/x86/include/generated  -I./include -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/compiler-version.h -include ./include/linux/kconfig.h -include ./include/linux/compiler_types.h -D__KERNEL__ -Werror -fmacro-prefix-map=./= -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Werror=return-type -Wno-format-security -funsigned-char -std=gnu11 --target=x86_64-linux-gnu -fintegrated-as -Werror=unknown-warning-option -Werror=ignored-optimization-argument -Werror=option-ignored -Werror=unused-command-line-argument -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -mno-avx -fcf-protection=none -m32 -msoft-float -mregparm=3 -freg-struct-return -fno-pic -mstack-alignment=4 -march=i686 -ffreestanding -mstack-protector-guard-reg=fs -mstack-protector-guard-symbol=__stack_chk_guard -Wno-sign-compare -fno-asynchronous-unwind-tables -mretpoline-external-thunk -mfunction-return=thunk-extern -fno-delete-null-pointer-checks -Wno-frame-address -Wno-address-of-packed-member -O2 -Wframe-larger-than=2048 -fstack-protector-strong -Wno-gnu -Wno-unused-but-set-variable -Wno-unused-const-variable -fno-omit-frame-pointer -fno-optimize-sibling-calls -fno-stack-clash-protection -mfentry -DCC_USING_FENTRY -falign-functions=4 -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wcast-function-type -Wimplicit-fallthrough -fno-strict-overflow -fno-stack-check -Werror=date-time -Werror=incompatible-pointer-types -Wno-initializer-overrides -Wno-format -Wformat-extra-args -Wformat-invalid-specifier -Wformat-zero-length -Wnonnull -Wformat-insufficient-args -Wno-sign-compare -Wno-pointer-to-enum-cast -Wno-tautological-constant-out-of-range-compare -Wno-unaligned-access    -DKBUILD_MODFILE='"arch/x86/kernel/cpu/common"' -DKBUILD_BASENAME='"common"' -DKBUILD_MODNAME='"common"' -D__KBUILD_MODNAME=kmod_common -c -o arch/x86/kernel/cpu/common.o arch/x86/kernel/cpu/common.c

Where GCC generates:

00000024 <load_direct_gdt>:
      24:       55                      push   %ebp
      25:       89 c2                   mov    %eax,%edx
      27:       89 e5                   mov    %esp,%ebp
      29:       83 ec 0c                sub    $0xc,%esp
      2c:       64 a1 00 00 00 00       mov    %fs:0x0,%eax
                        2e: R_386_32    __stack_chk_guard
      32:       89 45 fc                mov    %eax,-0x4(%ebp)

So it seems that GCC and CLANG generate different code for the 32-bit kernel as well. However, CLANG always generates non-TLS references for the 32-bit kernel, which is different from the 64-bit kernel that generates both non-TLS and TLS references. Therefore, the 32-bit kernel may boot by accident for CLANG, but I don't believe it is correct because it functions more like a global stack protector.

Sorry for the confusion, I should clarify that CLANG generates both non-TLS and TLS references for the 32-bit kernel as well, but it still manages to boot successfully. I may need to figure out why this is the case.

After conducting a thorough investigation, I have discovered the reason why the 32-bit kernel is able to boot successfully. It is because the Per-CPU section is located in the kernel high address for 32-bit SMP kernel. However, the Per-CPU section is mapped as 0 for 64-bit SMP kernel. Therefore, no TLS references for __stack_chk_guard fail in the 64-bit kernel due to the low address, but they can still be accessed correctly in the 32-bit kernel. However, once the PER-CPU areas are initialized, no TLS references for __stack_chk_guard would use the wrong canary in the initial PER-CPU section. Instead, TLS references for __stack_chk_guard would use the correct canary in PER-CPU areas.

However, the Per-CPU section in EFL is located in init_mem range, which will be freed in free_kernel_image_pages(). If CONFIG_DEBUG_PAGEALLOC is enabled and load_direct_gdt() is called after free_kernel_image_pages, a kernel page fault would occur.:

[   18.720473] debug: unmapping init [mem 0xc33fe000-0xc34e3fff]
[   18.721417] BUG: unable to handle page fault for address: c34da6d0
[   18.722349] #PF: supervisor read access in kernel mode
[   18.723121] #PF: error_code(0x0000) - not-present page
[   18.723891] *pde = 03585063 *pte = 034da062
[   18.724506] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[   18.725326] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.4.0-rc4-g9a1398f042de-dirty #867
[   18.726487] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
[   18.728115] EIP: load_direct_gdt+0x6/0x40
[   18.728743] Code: a1 00 b8 a2 06 00 00 31 d2 31 c9 6a 00 e8 c2 55 45 00 83 c4 04 5d e9 39 6a a1 00 90 90 90 90 90 90 90 90 90 55 89 e5 83 ec 0c <89
[   18.731529] EAX: 00000000 EBX: c2e47280 ECX: 6e0ce995 EDX: 00110800
[   18.732473] ESI: 00000000 EDI: 00000000 EBP: c111ff9c ESP: c111ff90
[   18.733382] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010286
[   18.734364] CR0: 80050033 CR2: c34da6d0 CR3: 034f1000 CR4: 00150ed0
[   18.735309] Call Trace:
[   18.735699]  ? __die_body+0x5b/0xa0
[   18.736246]  ? __die+0x7d/0x90
[   18.736693]  ? page_fault_oops+0x26e/0x2b0
[   18.737336]  ? kernelmode_fixup_or_oops+0xa5/0xc0
[   18.738046]  ? __bad_area_nosemaphore+0x50/0x190
[   18.738717]  ? bad_area_nosemaphore+0x12/0x20
[   18.739377]  ? do_kern_addr_fault+0x7b/0xc0
[   18.740016]  ? exc_page_fault+0x67/0x128
[   18.740592]  ? pvclock_clocksource_read_nowd+0xe0/0xe0
[   18.741349]  ? rest_init+0xb4/0xb4
[   18.741878]  ? handle_exception+0x133/0x133
[   18.742514]  ? rest_init+0xb4/0xb4
[   18.743042]  ? pvclock_clocksource_read_nowd+0xe0/0xe0
[   18.743774]  ? load_direct_gdt+0x6/0x40
[   18.744342]  ? pvclock_clocksource_read_nowd+0xe0/0xe0
[   18.745107]  ? load_direct_gdt+0x6/0x40
[   18.745653]  ? rest_init+0xb4/0xb4
[   18.746188]  free_initmem+0x28/0x30
[   18.746691]  kernel_init+0x35/0x170

Based on my analysis, it appears that the stack protector may be broken for the 32-bit kernel in CLANG in the mainline.

[nickdesaulniers]

https://godbolt.org/z/9q59r6E4P shows the code difference; it's whether or not __stack_chk_guard is declared. Ah sorry, this was discussed already.

cc @xiangzh1