| Message ID | 20220203201754.1145358-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | dcb85f85fa6f142aae1fe86f399d4503d49f2b60 |
| Headers | show |
| Series | [v2] gcc-plugins/stackleak: Use noinstr in favor of notrace | expand |
On Thu, Feb 3, 2022 at 12:18 PM Kees Cook <keescook@chromium.org> wrote: > > While the stackleak plugin was already using notrace, objtool is now a > bit more picky. Update the notrace uses to noinstr. Silences the > following objtool warnings when building with: Thanks, applied. There are still a few objtool warnings about other issues, all look somehow related to mce: mce_start()+0x5c: call to __kasan_check_write() leaves .noinstr.text section mce_gather_info()+0x5f: call to v8086_mode.constprop.0() leaves .noinstr.text section mce_read_aux()+0x8a: call to mca_msr_reg() leaves .noinstr.text section do_machine_check()+0x197: call to mce_no_way_out() leaves .noinstr.text section mce_severity_amd.constprop.0()+0xca: call to mce_severity_amd_smca() leaves .noinstr.textp section and from a quick look at least some of them look like real bugs. For example, mce_read_aux() is marked 'noinstr', but it calls mca_msr_reg() which is not. That's iffy. The others might be compiler-generated (the 'constprop' thing has caused section issues before so I didn't bother looking closer). Or related to kasan. But at least one of them seems to be a valid warning about bad behavior. Linus
On Thu, Feb 03, 2022 at 05:14:11PM -0800, Linus Torvalds wrote: > There are still a few objtool warnings about other issues, all look > somehow related to mce: I have a small patchset addressing that, ofc. It is on its way to be sent out but there's always something else preempting me... :-\
On Thu, Feb 03, 2022 at 12:17:54PM -0800, Kees Cook wrote: > While the stackleak plugin was already using notrace, objtool is now a > bit more picky. Update the notrace uses to noinstr. Silences the > following objtool warnings when building with: > > CONFIG_DEBUG_ENTRY=y > CONFIG_STACK_VALIDATION=y > CONFIG_VMLINUX_VALIDATION=y > CONFIG_GCC_PLUGIN_STACKLEAK=y > > vmlinux.o: warning: objtool: do_syscall_64()+0x9: call to stackleak_track_stack() leaves .noinstr.text section > vmlinux.o: warning: objtool: do_int80_syscall_32()+0x9: call to stackleak_track_stack() leaves .noinstr.text section > vmlinux.o: warning: objtool: exc_general_protection()+0x22: call to stackleak_track_stack() leaves .noinstr.text section > vmlinux.o: warning: objtool: fixup_bad_iret()+0x20: call to stackleak_track_stack() leaves .noinstr.text section > vmlinux.o: warning: objtool: do_machine_check()+0x27: call to stackleak_track_stack() leaves .noinstr.text section > vmlinux.o: warning: objtool: .text+0x5346e: call to stackleak_erase() leaves .noinstr.text section > vmlinux.o: warning: objtool: .entry.text+0x143: call to stackleak_erase() leaves .noinstr.text section > vmlinux.o: warning: objtool: .entry.text+0x10eb: call to stackleak_erase() leaves .noinstr.text section > vmlinux.o: warning: objtool: .entry.text+0x17f9: call to stackleak_erase() leaves .noinstr.text section > > Note that the plugin's addition of calls to stackleak_track_stack() > from noinstr functions is expected to be safe, as it isn't runtime > instrumentation and is self-contained. > > Cc: Alexander Popov <alex.popov@linux.com> > Suggested-by: Peter Zijlstra <peterz@infradead.org> > Signed-off-by: Kees Cook <keescook@chromium.org> No, I didn't suggest this and it is actively wrong. noinstr *really* should mean no instrumentation, nothing, nada, zip.
diff --git a/kernel/stackleak.c b/kernel/stackleak.c index 66b8af394e58..ddb5a7f48d69 100644 --- a/kernel/stackleak.c +++ b/kernel/stackleak.c @@ -70,7 +70,7 @@ late_initcall(stackleak_sysctls_init); #define skip_erasing() false #endif /* CONFIG_STACKLEAK_RUNTIME_DISABLE */ -asmlinkage void notrace stackleak_erase(void) +asmlinkage void noinstr stackleak_erase(void) { /* It would be nice not to have 'kstack_ptr' and 'boundary' on stack */ unsigned long kstack_ptr = current->lowest_stack; @@ -124,9 +124,8 @@ asmlinkage void notrace stackleak_erase(void) /* Reset the 'lowest_stack' value for the next syscall */ current->lowest_stack = current_top_of_stack() - THREAD_SIZE/64; } -NOKPROBE_SYMBOL(stackleak_erase); -void __used __no_caller_saved_registers notrace stackleak_track_stack(void) +void __used __no_caller_saved_registers noinstr stackleak_track_stack(void) { unsigned long sp = current_stack_pointer;
While the stackleak plugin was already using notrace, objtool is now a bit more picky. Update the notrace uses to noinstr. Silences the following objtool warnings when building with: CONFIG_DEBUG_ENTRY=y CONFIG_STACK_VALIDATION=y CONFIG_VMLINUX_VALIDATION=y CONFIG_GCC_PLUGIN_STACKLEAK=y vmlinux.o: warning: objtool: do_syscall_64()+0x9: call to stackleak_track_stack() leaves .noinstr.text section vmlinux.o: warning: objtool: do_int80_syscall_32()+0x9: call to stackleak_track_stack() leaves .noinstr.text section vmlinux.o: warning: objtool: exc_general_protection()+0x22: call to stackleak_track_stack() leaves .noinstr.text section vmlinux.o: warning: objtool: fixup_bad_iret()+0x20: call to stackleak_track_stack() leaves .noinstr.text section vmlinux.o: warning: objtool: do_machine_check()+0x27: call to stackleak_track_stack() leaves .noinstr.text section vmlinux.o: warning: objtool: .text+0x5346e: call to stackleak_erase() leaves .noinstr.text section vmlinux.o: warning: objtool: .entry.text+0x143: call to stackleak_erase() leaves .noinstr.text section vmlinux.o: warning: objtool: .entry.text+0x10eb: call to stackleak_erase() leaves .noinstr.text section vmlinux.o: warning: objtool: .entry.text+0x17f9: call to stackleak_erase() leaves .noinstr.text section Note that the plugin's addition of calls to stackleak_track_stack() from noinstr functions is expected to be safe, as it isn't runtime instrumentation and is self-contained. Cc: Alexander Popov <alex.popov@linux.com> Suggested-by: Peter Zijlstra <peterz@infradead.org> Signed-off-by: Kees Cook <keescook@chromium.org> --- kernel/stackleak.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)