Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[Mark Rutland]

On Wed, Sep 29, 2021 at 11:59:51AM +0200, Peter Zijlstra wrote:
> On Wed, Sep 29, 2021 at 09:50:45AM +0100, Mark Rutland wrote:
> > On Wed, Sep 29, 2021 at 09:39:47AM +0200, Peter Zijlstra wrote:
> > > On Tue, Sep 28, 2021 at 06:36:37PM -0700, Josh Poimboeuf wrote:
> 
> > > > +	asm volatile("417: rdmsr\n"
> > > > +		     : EAX_EDX_RET(val, low, high)
> > > > +		     : "c" (msr));
> > > > +	asm_volatile_goto(_ASM_EXTABLE(417b, %l[Efault]) :::: Efault);
> > > 
> > > That's terrible :-) Could probably do with a comment, but might just
> > > work..
> > 
> > The compiler is well within its rights to spill/restore/copy/shuffle
> > registers or modify memory between the two asm blocks (which it's liable
> > to do that when optimizing this after a few layers of inlining), and
> > skipping that would cause all sorts of undefined behaviour.
> 
> Ah, but in this case it'll work irrespective of that (which is why we
> needs a comment!).
> 
> This is because _ASM_EXTABLE only generates data for another section.
> There doesn't need to be code continuity between these two asm
> statements.

I think you've missed my point. It doesn't matter that the
asm_volatile_goto() doesn't contain code, and this is solely about the
*state* expected at entry/exit from each asm block being different.

The problem is that when the compiler encounters the
asm_volatile_goto(), it will generate a target for `Efault` expecting
the state of registers/stack/etc to be consistent with the state at
entry to the asm_volatile_goto() block. So if the compiler places any
register/memory manipulation between the asm volatile and the
asm_volatile_goto block, that expectation will be violated, since we
effectively branch from the first asm volatile block directly to the
label handed to the asm_volatile_goto block.

Consider the following pseudo asm example:

inline unsigned long read_magic_asm_thing(void)
{
	// asm constraints allocates this into x3 for now
	unsigned long ret = 3;

	asm volatile(
	"magic_insn_that_can_only_read_into x3\n"
	"fault_insn: some_faulting_insn x3\n"
	: [x3] "x3" (ret)
	);

	// compiler moves x3 into x0 because that's simpler for later
	// code (in both the fall-through and branch case of the
	// asm_volatile_goto()).
	// Maybe it shuffles other things too, e.g. moving another
	// variable into x3.

	// This is generated expecting the register allocation at this
	// instant in the code
	asm_volatile_goto(extable_from_to(fault_isn, Efault));

	// When not faulting, x0 is used here; this works correctly.
	return ret;

Efault:
	// When we take a fault from the first asm, the `ret` value is
	// in x3, and we skipped the moves between the two asm blocks.
	// This code was generated assuming those had happened (since
	// that was the case at the start of the asm_volatile_goto(),
	// and consumes x0 here, which contains garbage.
	do_something_with(ret);

	// Maybe this uses something that was moved into x3, but we have
	// `ret` there instead.
	something_else();
	
	// Who knows if we even got here safely.
	return whatever;
}

Thanks,
Mark.