Date:   Fri, 3 Feb 2023 16:04:55 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     Borislav Petkov <bp@alien8.de>
Cc:     x86@kernel.org, Masami Hiramatsu <mhiramat@kernel.org>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Andrew Cooper <Andrew.Cooper3@citrix.com>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC] x86/alternative: Support relocations in alternatives
Message-ID: <Y90ilxBFMAQZ/dRg@hirez.programming.kicks-ass.net>
References: <Y9py2a5Xw0xbB8ou@hirez.programming.kicks-ass.net>
 <Y90IaYHDbCN9P0OX@zn.tnic>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y90IaYHDbCN9P0OX@zn.tnic>
Precedence: bulk

On Fri, Feb 03, 2023 at 02:13:13PM +0100, Borislav Petkov wrote:
> On Wed, Feb 01, 2023 at 03:10:33PM +0100, Peter Zijlstra wrote:
> > (sorry for the repost, forgot lkml)
> 
> Btw, you need to rediff this against tip/master. There's alt_instr.flags
> change in there which conflicts with yours.
> 
> > +static void __always_inline
> > +apply_reloc(int n, void *ptr, uintptr_t diff)
> > +{
> > +	switch (n) {
> > +	case 1: apply_reloc_n(8, ptr, diff); break;
> > +	case 2: apply_reloc_n(16, ptr, diff); break;
> > +	case 4: apply_reloc_n(32, ptr, diff); break;
> > +	default: BUG();
> > +	}
> > +}
> > +
> > +static void __init_or_module noinline
> > +apply_relocation(u8 *instr, size_t len, u8 *dest, u8 *src, size_t src_len)
> 
> First param is instr, third is dest.
> 
> at the call site you have
> 
> apply_relocation(insn_buff, a->instrlen, instr, replacement, a->replacementlen);
> 
> and instr is third param. Let's call the function params:
> 
> static void __init_or_module noinline
> apply_relocation(u8 *insn_buff, size_t len, u8 *instr, u8 *repl, size_t repl_len)

How about:

  apply_relocation(u8 *buf, size_t len, u8 *dst, u8 *src, size_t src_len)

Because I get horribly confused by the whole instr and repl thing.

> > +{
> > +	struct insn insn;
> > +	int i = 0;
> > +
> > +	for (;;) {
> > +		if (insn_decode_kernel(&insn, &instr[i]))
> 
> I guess say a warning here so that we catch when it goes into the fields early.

Sure..

> > +			return;
> > +
> > +		switch (insn.opcode.bytes[0]) {
> > +		case 0x0f:
> > +			if (insn.opcode.bytes[1] < 0x80 ||
> > +			    insn.opcode.bytes[1] > 0x8f)
> > +				break;
> > +
> > +			fallthrough;	/* Jcc.d32 */
> > +		case 0x70 ... 0x7f:	/* Jcc.d8 */
> > +		case JMP8_INSN_OPCODE:
> > +		case JMP32_INSN_OPCODE:
> > +		case CALL_INSN_OPCODE:
> > +			u8 *target = src + i + insn.length + insn.immediate.value;
> > +			if (target < src || target > src + src_len) {
> > +				apply_reloc(insn.immediate.nbytes,
> > +					    instr + i + insn_offset_immediate(&insn),
> > +					    src - dest);
> > +			}
> 
> Uff, it took me a while to parse this. So this can be simpler. The basic
> math is:
> 
> 	new_offset = next_rip - target_address;
> 
> where
> 	next_rip = instr + insn.length;
> 
> and I admit that my function was a bit clumsy but I think yours can be
> made simpler while keeping it cleaner.

I'm not sure what you're saying here... so let me walk through the whole
thing (specifically the immediate case, since that's what you quote). So
what we need is:

	src_imm = target - src_next_ip			(1)

what we want is:

	dst_imm = target - dst_next_ip			(2)

so what we do is rework (1) as an expression for target like:

	target = src_imm + src_next_ip			(1a)

and substitute in (2) to get:

	dst_imm = (src_imm + src_next_ip) - dst_next_ip	(3)

Now, since the instruction stream is 'identical' at src and dest
we can state that:

	src_next_ip = src + ip_offset
	dst_next_ip = dst + ip_offset			(4)

Substitute (4) in (3) and observe ip_offset being cancelled out to
obtain:

	dst_imm = src_imm + (src + ip_offset) - (dst + ip_offset)
	        = src_imm + src - dst + ip_offset - ip_offset
	        = src_imm + src - dst			(5)

The very thing I did.

IOW, we can correct the displacement without caring about which actual
instruction in the stream we're correcting since the relative offset is
given by 'src - dst'. IOW, we don't give a crap about insn.length in
this case ;-)

> Also, calling this all "reloc" here is kinda confusing because this is not a
> relocation but the offset from the next RIP to the target of the
> JMP/CALL.

Well, we do relocate the instructions... They go from one place to
another.