linux-kernel - Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text

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]

Message-ID: <20210929103730.GC33284@C02TD0UTHF1T.local>
Date:   Wed, 29 Sep 2021 11:37:30 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Josh Poimboeuf <jpoimboe@...hat.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        syzbot <syzbot+488ddf8087564d6de6e2@...kaller.appspotmail.com>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        syzkaller-bugs@...glegroups.com, viro@...iv.linux.org.uk,
        will@...nel.org, x86@...nel.org
Subject: Re: [syzbot] upstream test error: KASAN: invalid-access Read in
 __entry_tramp_text_end

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.