Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 4 May 2022 09:26:47 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Josh Poimboeuf <jpoimboe@redhat.com>
Cc:     x86@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] objtool: Fix SLS checks
Message-ID: <YnIqt4aOooAvlja4@hirez.programming.kicks-ass.net>
References: <Ym0UWja2L40QbgEc@hirez.programming.kicks-ass.net>
 <20220502181547.fzz2yre2p4akph4s@treble>
 <YnA8Yw19AQV28w54@hirez.programming.kicks-ass.net>
 <20220503211510.ytdv5l4l2zwjdwg7@treble>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220503211510.ytdv5l4l2zwjdwg7@treble>
Precedence: bulk

On Tue, May 03, 2022 at 02:15:10PM -0700, Josh Poimboeuf wrote:
> On Mon, May 02, 2022 at 10:17:39PM +0200, Peter Zijlstra wrote:
> > > +++ b/tools/objtool/check.c
> > > @@ -3842,9 +3842,6 @@ static int validate_sls(struct objtool_file *file)
> > >  	for_each_insn(file, insn) {
> > >  		next_insn = next_insn_same_sec(file, insn);
> > >  
> > > -		if (insn->retpoline_safe)
> > > -			continue;
> > > -
> > >  		switch (insn->type) {
> > >  		case INSN_RETURN:
> > >  			if (!next_insn || next_insn->type != INSN_TRAP) {
> > 
> > Yes, agreed. But perhaps with something like this on top?
> 
> Yup, I missed those...  Looks good.  Just one comment:
> 
> > +++ b/arch/x86/kernel/alternative.c
> > @@ -452,6 +452,17 @@ static int patch_retpoline(void *addr, struct insn *insn, u8 *bytes)
> >  		return ret;
> >  	i += ret;
> >  
> > +#ifdef CONFIG_SLS
> > +	/*
> > +	 * Ideally this would be unconditional, except in case of
> > +	 * RETPOLINE_LFENCE we don't have sufficient space. Additionally,
> > +	 * -mharden-sls=all should be extended to emit INT3 after
> > +	 * direct jumps too, which will then cover that case.
> > +	 */
> 
> I don't quite follow this 2nd sentence and how it's related here, since
> this function doesn't actually deal with direct jumps.

Ah, my bad. Also, this wrong.

I suppose this wants to be something like:

	if (i < insn->length && op == JMP32_INSN_OPCODE)
		bytes[i++] = INT3_INSN_OPCODE;

So this *can* be a jump, but typically won't be I suppose.

> Speaking of, I guess we'll eventually need to hack this SLS mess into
> jump labels :-/

Urgh... can't we reason that the straight line case is actually expected
to run with the given register state anyway and ignore this?