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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 6 Feb 2022 08:46:47 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Alexander Popov <alex.popov@...ux.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Borislav Petkov <bp@...en8.de>, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH] gcc-plugins/stackleak: Use noinstr in favor of notrace

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