Re: [PATCH v3 3/4] uaccess: Check no rescheduling function is called in unsafe region

From: Andy Lutomirski <luto@amacapital.net>
To: Peter Zijlstra <peterz@infradead.org>
Cc: dvlasenk@redhat.com, brgerst@gmail.com,
	Julien Thierry <julien.thierry@arm.com>,
	catalin.marinas@arm.com, jpoimboe@redhat.com,
	Will Deacon <will.deacon@arm.com>,
	linux-kernel@vger.kernel.org, valentin.schneider@arm.com,
	mingo@redhat.com, james.morse@arm.com, luto@kernel.org,
	hpa@zytor.com, bp@alien8.de, tglx@linutronix.de,
	torvalds@linux-foundation.org, Ingo Molnar <mingo@kernel.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 3/4] uaccess: Check no rescheduling function is called in unsafe region
Date: Wed, 13 Feb 2019 14:49:47 -0800	[thread overview]
Message-ID: <D3E1B64D-3E91-4A82-9C3B-92D9EB7326CE@amacapital.net> (raw)
In-Reply-To: <20190213222146.GC32494@hirez.programming.kicks-ass.net>

> On Feb 13, 2019, at 2:21 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Wed, Feb 13, 2019 at 10:51:24AM -0800, Andy Lutomirski wrote:
>>> On Feb 13, 2019, at 7:45 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
>>> Which I suppose means that GCC generates the PUSHF/POPF to preserve the
>>> EFLAGS, since we mark those explicitly clobbered.
>>> 
>> 
>> Not quite.  A flags clobber doesn’t save the control bits like AC
>> except on certain rather buggy llvm compilers. The change you’re
>> looking for is:
>> 
>> http://git.kernel.org/tip/2c7577a7583747c9b71f26dced7f696b739da745
> 
> Indeed, failed to find that.
> 
>>> For a little bit of context; it turns out that user_access_begin() /
>>> user_access_end() sets EFLAGS.AC and scheduling in between there wrecks
>>> that because we're apparently not saving that anymore.
>> 
>> But only explicit scheduling — preemption and sleepy page faults are
>> fine because the interrupt frame saves flags.
> 
> No, like pointed out elsewhere in this thread, anything that does
> preempt_disable() is utterly broken with this.
> 
> Because at that point the IRQ return path doesn't reschedule but
> preempt_enable() will, and that doesn't preserve EFLAGS again.
> 
>>> Now, I'm tempted to add the PUSHF / POPF right back because of this, but
>>> first I suppose we need to figure out if that change was on purpose and
>>> why that went missing from the Changelog.
>> 
>> That’s certainly the easy solution. Or we could teach the might_sleep
>> checks about this, but that could be a mess.
> 
> That's not enough, we'd have to teach preempt_disable(), but worse,
> preempt_disable_notrace().
> 
> Anything that lands in ftrace, which _will_ use
> preempt_disable_notrace(), will screw this thing up.

Ugh.  Consider your patch acked.  Do we need to backport this thing?  The problem can’t be too widespread or we would have heard of it before.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel