[PATCH] gcc-plugins/stackleak: Use noinstr in favor of notrace

[PATCH] gcc-plugins/stackleak: Use noinstr in favor of notrace

[Kees Cook]

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(-)

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

Re: [PATCH] gcc-plugins/stackleak: Use noinstr in favor of notrace

[Mark Rutland]

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
> 

Re: [PATCH] gcc-plugins/stackleak: Use noinstr in favor of notrace

[Linus Torvalds]

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

Re: [PATCH] gcc-plugins/stackleak: Use noinstr in favor of notrace

[Peter Zijlstra]

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;

Re: [PATCH] gcc-plugins/stackleak: Use noinstr in favor of notrace

[Kees Cook]

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...

-- 
Kees Cook

Re: [PATCH] gcc-plugins/stackleak: Use noinstr in favor of notrace

[Peter Zijlstra]

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.

Re: [PATCH] gcc-plugins/stackleak: Use noinstr in favor of notrace

[Kees Cook]

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

-- 
Kees Cook