Re: [PATCH 18/20] objtool: Add UACCESS validation

From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: torvalds@linux-foundation.org, tglx@linutronix.de, hpa@zytor.com,
	julien.thierry@arm.com, will.deacon@arm.com, luto@amacapital.net,
	mingo@kernel.org, catalin.marinas@arm.com, james.morse@arm.com,
	valentin.schneider@arm.com, brgerst@gmail.com, luto@kernel.org,
	bp@alien8.de, dvlasenk@redhat.com, linux-kernel@vger.kernel.org,
	dvyukov@google.com, rostedt@goodmis.org
Subject: Re: [PATCH 18/20] objtool: Add UACCESS validation
Date: Fri, 8 Mar 2019 15:54:49 -0600	[thread overview]
Message-ID: <20190308215449.47xk64pt7svk4pef@treble> (raw)
In-Reply-To: <20190308213155.GD2482@worktop.programming.kicks-ass.net>

On Fri, Mar 08, 2019 at 10:31:56PM +0100, Peter Zijlstra wrote:
> On Fri, Mar 08, 2019 at 03:02:09PM -0600, Josh Poimboeuf wrote:
> > > +static const char *uaccess_safe_builtin[] = {
> > > +	/* KASAN */
> > 
> > A short comment would be good here, something describing why a function
> > might be added to the list.
> 
> There is; but I'm thinking it might be too short?

I actually just meant a comment above uaccess_safe_builtin describing
what the purpose of the list is and what the expectations are for the
listed functions.  i.e. these are functions which are allowed to be
called with the AC flag on, and they should not clear it unless they're
saving/restoring.

> > > +++ b/tools/objtool/special.c
> > > @@ -42,6 +42,7 @@
> > >  #define ALT_NEW_LEN_OFFSET	11
> > >  
> > >  #define X86_FEATURE_POPCNT (4*32+23)
> > > +#define X86_FEATURE_SMAP   (9*32+20)
> > >  
> > >  struct special_entry {
> > >  	const char *sec;
> > > @@ -107,8 +108,15 @@ static int get_alt_entry(struct elf *elf
> > >  		 * It has been requested that we don't validate the !POPCNT
> > >  		 * feature path which is a "very very small percentage of
> > >  		 * machines".
> > > +		 *
> > > +		 * Also, unconditionally enable SMAP; this avoids seeing paths
> > > +		 * that pass through the STAC alternative and through the CLAC
> > > +		 * NOPs.
> > 
> > Why is this a problem?
> 
> 'obvious' violation?
> 
> STAC; .... RET; # an AC=1 leak
> 
> .... CLAC; # spurious CLAC
> 
> If we do the STAC we must also do the CLAC. If we don't do the STAC we
> must also not do the CLAC.

Makes sense now, can you add that last sentence to the paragraph?

> > > +		 *
> > > +		 * XXX: We could do this for all binary NOP/single-INSN
> > > +		 * alternatives.
> > 
> > Same question here.
> 
> In general, validating NOPs isn't too interesting, so all NOP/INSN
> binary alternatives could be forced on.

Right, but it doesn't sound like there's any real benefit to adding
extra logic.

> > >  		 */
> > > -		if (feature == X86_FEATURE_POPCNT)
> > > +		if (feature == X86_FEATURE_POPCNT || feature == X86_FEATURE_SMAP)
> > >  			alt->skip_orig = true;
> > >  	}
> 
> I've actually changed this to depend on --uaccess, when set we force on
> FEATURE_SMAP, otherwise we force off.

Ok.

-- 
Josh