* [PATCH] x86/cmdline: Disable instrumentation of cmdline unconditionally
@ 2020-09-06 15:46 Arvind Sankar
2020-09-17 9:40 ` Borislav Petkov
0 siblings, 1 reply; 4+ messages in thread
From: Arvind Sankar @ 2020-09-06 15:46 UTC (permalink / raw)
To: x86; +Cc: linux-kernel
On 32-bit, cmdline_find_option_bool() is used before paging is enabled,
from check_loader_disabled_bsp() in the early microcode loader.
Instrumentation options that insert accesses to global data will likely
crash or corrupt memory at this point.
cmdline_find_option{,_bool}() are only used during boot, so
instrumentation is not that useful anyway.
Disable instrumentation unconditionally, and additionally disable:
- GCOV
- UBSAN
- tracing: change -pg -> CC_FLAGS_FTRACE
- STACKLEAK_PLUGIN
- BRANCH_PROFILING
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
arch/x86/lib/Makefile | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
index aa067859a70b..0ad4ae9def44 100644
--- a/arch/x86/lib/Makefile
+++ b/arch/x86/lib/Makefile
@@ -15,17 +15,19 @@ CFLAGS_REMOVE_delay.o = $(CC_FLAGS_FTRACE)
endif
# Early boot use of cmdline; don't instrument it
-ifdef CONFIG_AMD_MEM_ENCRYPT
+GCOV_PROFILE_cmdline.o := n
KCOV_INSTRUMENT_cmdline.o := n
KASAN_SANITIZE_cmdline.o := n
+UBSAN_SANITIZE_cmdline.o := n
KCSAN_SANITIZE_cmdline.o := n
ifdef CONFIG_FUNCTION_TRACER
-CFLAGS_REMOVE_cmdline.o = -pg
+CFLAGS_REMOVE_cmdline.o = $(CC_FLAGS_FTRACE)
endif
CFLAGS_cmdline.o := -fno-stack-protector -fno-jump-tables
-endif
+CFLAGS_cmdline.o += $(DISABLE_STACKLEAK_PLUGIN)
+CFLAGS_cmdline.o += -DDISABLE_BRANCH_PROFILING
inat_tables_script = $(srctree)/arch/x86/tools/gen-insn-attr-x86.awk
inat_tables_maps = $(srctree)/arch/x86/lib/x86-opcode-map.txt
--
2.26.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] x86/cmdline: Disable instrumentation of cmdline unconditionally 2020-09-06 15:46 [PATCH] x86/cmdline: Disable instrumentation of cmdline unconditionally Arvind Sankar @ 2020-09-17 9:40 ` Borislav Petkov 2020-09-17 16:05 ` Arvind Sankar 0 siblings, 1 reply; 4+ messages in thread From: Borislav Petkov @ 2020-09-17 9:40 UTC (permalink / raw) To: Arvind Sankar; +Cc: x86, linux-kernel On Sun, Sep 06, 2020 at 11:46:37AM -0400, Arvind Sankar wrote: > On 32-bit, cmdline_find_option_bool() is used before paging is enabled, > from check_loader_disabled_bsp() in the early microcode loader. > Instrumentation options that insert accesses to global data will likely > crash or corrupt memory at this point. What is the use case here, can you trigger an actual crash? -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/cmdline: Disable instrumentation of cmdline unconditionally 2020-09-17 9:40 ` Borislav Petkov @ 2020-09-17 16:05 ` Arvind Sankar 2020-09-17 17:28 ` Borislav Petkov 0 siblings, 1 reply; 4+ messages in thread From: Arvind Sankar @ 2020-09-17 16:05 UTC (permalink / raw) To: Borislav Petkov; +Cc: Arvind Sankar, x86, linux-kernel On Thu, Sep 17, 2020 at 11:40:55AM +0200, Borislav Petkov wrote: > On Sun, Sep 06, 2020 at 11:46:37AM -0400, Arvind Sankar wrote: > > On 32-bit, cmdline_find_option_bool() is used before paging is enabled, > > from check_loader_disabled_bsp() in the early microcode loader. > > Instrumentation options that insert accesses to global data will likely > > crash or corrupt memory at this point. > > What is the use case here, can you trigger an actual crash? > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette Hm this is a bit embarassing. I did have a crash and this patch fixed it, but it seems it was on a branch where I was making changes to cmdline.c, which triggered clang to use a jump table for cmdline_find_option_bool(). That was the cause of the crash, and the reason this patch fixed it was because it enabled -fno-jump-tables, rather than because it disabled instrumentation. The instrumentation code does write data to random addresses, but apparently that doesn't necessarily crash the system. This patch would also be insufficient to fix it, since load_ucode_bsp() itself can have instrumentation code in it. Eg with GCOV_PROFILE_ALL enabled, the start of the function is: c2a7706a <load_ucode_bsp>: c2a7706a: 55 push %ebp c2a7706b: 83 05 c0 4d ba c2 01 addl $0x1,0xc2ba4dc0 c2a77072: 83 15 c4 4d ba c2 00 adcl $0x0,0xc2ba4dc4 c2a77079: 89 e5 mov %esp,%ebp but when it's called from arch/x86/kernel/head_32.S, paging is disabled and the code is executing out of physical addresses, so it's going to read/write data from garbage addresses. Anyway, please ignore this patch and sorry for the noise. Thanks. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/cmdline: Disable instrumentation of cmdline unconditionally 2020-09-17 16:05 ` Arvind Sankar @ 2020-09-17 17:28 ` Borislav Petkov 0 siblings, 0 replies; 4+ messages in thread From: Borislav Petkov @ 2020-09-17 17:28 UTC (permalink / raw) To: Arvind Sankar; +Cc: x86, linux-kernel On Thu, Sep 17, 2020 at 12:05:48PM -0400, Arvind Sankar wrote: > I did have a crash and this patch fixed it, but it seems it was on a > branch where I was making changes to cmdline.c, which triggered clang to > use a jump table for cmdline_find_option_bool(). That was the cause of > the crash, and the reason this patch fixed it was because it enabled > -fno-jump-tables, rather than because it disabled instrumentation. I see. > The instrumentation code does write data to random addresses, but > apparently that doesn't necessarily crash the system. This patch would > also be insufficient to fix it, since load_ucode_bsp() itself can have > instrumentation code in it. Yeah, it better not have any. > Eg with GCOV_PROFILE_ALL enabled, the start of the function is: > > c2a7706a <load_ucode_bsp>: > c2a7706a: 55 push %ebp > c2a7706b: 83 05 c0 4d ba c2 01 addl $0x1,0xc2ba4dc0 > c2a77072: 83 15 c4 4d ba c2 00 adcl $0x0,0xc2ba4dc4 > c2a77079: 89 e5 mov %esp,%ebp > > but when it's called from arch/x86/kernel/head_32.S, paging is disabled > and the code is executing out of physical addresses, so it's going to > read/write data from garbage addresses. Right, so I'm sceptical to all this instrumentation nonsense - it is fine and good if you have it but not everywhere. And certainly not when the thing is loading microcode. microcode/core.c contains all the code, early and late so it might make some little sense to have instrumentation there for whatever reasons, say KASANing (yah, I made a verb :)) the thing for leaks or whatnot, but meh, I can't find it in me to care. So at some point, if it starts getting in the way, we might remove all instrumentation from that thing altogether. > Anyway, please ignore this patch and sorry for the noise. No worries. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-09-17 18:07 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-06 15:46 [PATCH] x86/cmdline: Disable instrumentation of cmdline unconditionally Arvind Sankar 2020-09-17 9:40 ` Borislav Petkov 2020-09-17 16:05 ` Arvind Sankar 2020-09-17 17:28 ` Borislav Petkov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).