| Message ID | 20220202001918.4104428-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gcc-plugins/stackleak: Use noinstr in favor of notrace | expand |
On Tue, Feb 01, 2022 at 04:19:18PM -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 these > warnings: > > 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 > > Cc: Alexander Popov <alex.popov@linux.com> > Suggested-by: Peter Zijlstra <peterz@infradead.org> > Link: https://lore.kernel.org/lkml/YYENAKB0igNFnFmK@hirez.programming.kicks-ass.net/ > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > Is it correct to exclude .noinstr.text here? That means any functions called in > there will have their stack utilization untracked. This doesn't seem right to me, > though. Shouldn't stackleak_track_stack() just be marked noinstr instead? Given "noinstr" means "no instrumentation", it seems entirely correct to me that noinstr functions should not be instrumented with stack utilization checks. I am surprised that those *were* instrumented, and arguably this is a fix that should be backported. For stackleak_erase() itself, using noinstr certianly makes sense to me given the context in which it is called. FWIW: Reviewed-by: Mark Rutland <mark.rutland@arm.com> Mark. > --- > kernel/stackleak.c | 3 +-- > scripts/gcc-plugins/stackleak_plugin.c | 3 +++ > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/kernel/stackleak.c b/kernel/stackleak.c > index 66b8af394e58..72d4ebf49480 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,7 +124,6 @@ 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) > { > diff --git a/scripts/gcc-plugins/stackleak_plugin.c b/scripts/gcc-plugins/stackleak_plugin.c > index e9db7dcb3e5f..e7e51f0eb597 100644 > --- a/scripts/gcc-plugins/stackleak_plugin.c > +++ b/scripts/gcc-plugins/stackleak_plugin.c > @@ -429,6 +429,7 @@ static unsigned int stackleak_cleanup_execute(void) > return 0; > } > > +/* Do not instrument anything found in special sections. */ > static bool stackleak_gate(void) > { > tree section; > @@ -446,6 +447,8 @@ static bool stackleak_gate(void) > return false; > if (!strncmp(TREE_STRING_POINTER(section), ".meminit.text", 13)) > return false; > + if (!strncmp(TREE_STRING_POINTER(section), ".noinstr.text", 13)) > + return false; > } > > return track_frame_size >= 0; > -- > 2.30.2 >
I was going to apply your patch, but then I read your note: On Tue, Feb 1, 2022 at 4:19 PM Kees Cook <keescook@chromium.org> wrote: > > Is it correct to exclude .noinstr.text here? That means any functions called in > there will have their stack utilization untracked. This doesn't seem right to me, > though. Shouldn't stackleak_track_stack() just be marked noinstr instead? ... and yes, it seems like stackleak_track_stack() should just be 'noinstr' just like you made stackleak_erase(). So I've dropped the patch to see what happens. If you decide this is the right patch after all, you can just re-send it. Linus
On Tue, Feb 01, 2022 at 04:19:18PM -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 these > warnings: > > 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 > > Cc: Alexander Popov <alex.popov@linux.com> > Suggested-by: Peter Zijlstra <peterz@infradead.org> > Link: https://lore.kernel.org/lkml/YYENAKB0igNFnFmK@hirez.programming.kicks-ass.net/ > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > Is it correct to exclude .noinstr.text here? That means any functions called in > there will have their stack utilization untracked. This doesn't seem right to me, > though. Shouldn't stackleak_track_stack() just be marked noinstr instead? This patch is right. stackleak_track_stack() cannot be marked noinstr becaues it accesses things that might not be there. Consider what happens if we pull the PTI page-table swap into the noinstr C part. > @@ -446,6 +447,8 @@ static bool stackleak_gate(void) > return false; > if (!strncmp(TREE_STRING_POINTER(section), ".meminit.text", 13)) > return false; > + if (!strncmp(TREE_STRING_POINTER(section), ".noinstr.text", 13)) > + return false; For paranoia's sake I'd like .entry.text added there as well. > } > > return track_frame_size >= 0;
On Sun, Feb 06, 2022 at 12:58:16PM +0100, Peter Zijlstra wrote: > On Tue, Feb 01, 2022 at 04:19:18PM -0800, Kees Cook wrote: > > Is it correct to exclude .noinstr.text here? That means any functions called in > > there will have their stack utilization untracked. This doesn't seem right to me, > > though. Shouldn't stackleak_track_stack() just be marked noinstr instead? > > This patch is right. stackleak_track_stack() cannot be marked noinstr > becaues it accesses things that might not be there. Hmm, as in "current()" may not be available/sane? > Consider what happens if we pull the PTI page-table swap into the > noinstr C part. Yeah, I see your point. I suspect the reason this all currently works is because stackleak is supposed to only instrument leaf functions that have sufficiently large (default 100 bytes) stack usage. What sorts of things may end up in .noinstr.text that are 100+ byte stack leaf functions that would be end up deeper in the call stack? (i.e. what could get missed from stack depth tracking?) Interrupt handling comes to mind, but I'd expect that to make further calls (i.e. not a leaf). > > @@ -446,6 +447,8 @@ static bool stackleak_gate(void) > > return false; > > if (!strncmp(TREE_STRING_POINTER(section), ".meminit.text", 13)) > > return false; > > + if (!strncmp(TREE_STRING_POINTER(section), ".noinstr.text", 13)) > > + return false; > > For paranoia's sake I'd like .entry.text added there as well. Yeah, that's reasonable. Again, I think I see why this hasn't been an actual problem before, but given the constraints, I'd agree. It may allow for the paravirt special-case to be removed as well. I'll send a follow-up patch...
On Sun, Feb 06, 2022 at 08:46:47AM -0800, Kees Cook wrote: > On Sun, Feb 06, 2022 at 12:58:16PM +0100, Peter Zijlstra wrote: > > On Tue, Feb 01, 2022 at 04:19:18PM -0800, Kees Cook wrote: > > > Is it correct to exclude .noinstr.text here? That means any functions called in > > > there will have their stack utilization untracked. This doesn't seem right to me, > > > though. Shouldn't stackleak_track_stack() just be marked noinstr instead? > > > > This patch is right. stackleak_track_stack() cannot be marked noinstr > > becaues it accesses things that might not be there. > > Hmm, as in "current()" may not be available/sane? Exactly the case; if we lift the PTI address space swizzle, we start with C without having the kernel mapped or even the per-cpu segment offset set. So things like current will explode. The whole noinstr thing was invented to get back to C as portable Assembler, with the express purpose to lift a bunch of entry code to C. > > Consider what happens if we pull the PTI page-table swap into the > > noinstr C part. > > Yeah, I see your point. I suspect the reason this all currently works > is because stackleak is supposed to only instrument leaf functions that > have sufficiently large (default 100 bytes) stack usage. > > What sorts of things may end up in .noinstr.text that are 100+ byte stack > leaf functions that would be end up deeper in the call stack? (i.e. what > could get missed from stack depth tracking?) Interrupt handling comes > to mind, but I'd expect that to make further calls (i.e. not a leaf). All the syscall/exception/interrupt entry stuff is noinstr; I don't think we have huge stackframes, but with all that in C that's much easier to do than with then in asm. If you worry about this, it should be possible to have objtool warn about excessive stack frames for noinstr code I suppose, it already tracks the stack anyway.
On Sun, Feb 06, 2022 at 09:40:15PM +0100, Peter Zijlstra wrote: > On Sun, Feb 06, 2022 at 08:46:47AM -0800, Kees Cook wrote: > > On Sun, Feb 06, 2022 at 12:58:16PM +0100, Peter Zijlstra wrote: > > > On Tue, Feb 01, 2022 at 04:19:18PM -0800, Kees Cook wrote: > > > > Is it correct to exclude .noinstr.text here? That means any functions called in > > > > there will have their stack utilization untracked. This doesn't seem right to me, > > > > though. Shouldn't stackleak_track_stack() just be marked noinstr instead? > > > > > > This patch is right. stackleak_track_stack() cannot be marked noinstr > > > becaues it accesses things that might not be there. > > > > Hmm, as in "current()" may not be available/sane? > > Exactly the case; if we lift the PTI address space swizzle, we start > with C without having the kernel mapped or even the per-cpu segment > offset set. So things like current will explode. > > The whole noinstr thing was invented to get back to C as portable > Assembler, with the express purpose to lift a bunch of entry code to C. > > > > Consider what happens if we pull the PTI page-table swap into the > > > noinstr C part. > > > > Yeah, I see your point. I suspect the reason this all currently works > > is because stackleak is supposed to only instrument leaf functions that > > have sufficiently large (default 100 bytes) stack usage. > > > > What sorts of things may end up in .noinstr.text that are 100+ byte stack > > leaf functions that would be end up deeper in the call stack? (i.e. what > > could get missed from stack depth tracking?) Interrupt handling comes > > to mind, but I'd expect that to make further calls (i.e. not a leaf). > > All the syscall/exception/interrupt entry stuff is noinstr; I don't > think we have huge stackframes, but with all that in C that's much > easier to do than with then in asm. > > If you worry about this, it should be possible to have objtool warn > about excessive stack frames for noinstr code I suppose, it already > tracks the stack anyway. Yeah, I think we should be okay at least for now. Let me know what you think of https://lore.kernel.org/linux-hardening/20220206174508.2425076-1-keescook@chromium.org/ and if you like it I can send a v2 Linus's way... -Kees
diff --git a/kernel/stackleak.c b/kernel/stackleak.c index 66b8af394e58..72d4ebf49480 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,7 +124,6 @@ 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) { diff --git a/scripts/gcc-plugins/stackleak_plugin.c b/scripts/gcc-plugins/stackleak_plugin.c index e9db7dcb3e5f..e7e51f0eb597 100644 --- a/scripts/gcc-plugins/stackleak_plugin.c +++ b/scripts/gcc-plugins/stackleak_plugin.c @@ -429,6 +429,7 @@ static unsigned int stackleak_cleanup_execute(void) return 0; } +/* Do not instrument anything found in special sections. */ static bool stackleak_gate(void) { tree section; @@ -446,6 +447,8 @@ static bool stackleak_gate(void) return false; if (!strncmp(TREE_STRING_POINTER(section), ".meminit.text", 13)) return false; + if (!strncmp(TREE_STRING_POINTER(section), ".noinstr.text", 13)) + return false; } return track_frame_size >= 0;
While the stackleak plugin was already using notrace, objtool is now a bit more picky. Update the notrace uses to noinstr. Silences these warnings: 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 Cc: Alexander Popov <alex.popov@linux.com> Suggested-by: Peter Zijlstra <peterz@infradead.org> Link: https://lore.kernel.org/lkml/YYENAKB0igNFnFmK@hirez.programming.kicks-ass.net/ Signed-off-by: Kees Cook <keescook@chromium.org> --- Is it correct to exclude .noinstr.text here? That means any functions called in there will have their stack utilization untracked. This doesn't seem right to me, though. Shouldn't stackleak_track_stack() just be marked noinstr instead? --- kernel/stackleak.c | 3 +-- scripts/gcc-plugins/stackleak_plugin.c | 3 +++ 2 files changed, 4 insertions(+), 2 deletions(-)