linux-kernel - Re: barriers: was: [RFC PATCH v2 17/18] livepatch: change to a per-task consistency model

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]

Date:	Thu, 5 May 2016 13:21:32 +0200
From:	Petr Mladek <pmladek@...e.com>
To:	Josh Poimboeuf <jpoimboe@...hat.com>
Cc:	Jessica Yu <jeyu@...hat.com>, Jiri Kosina <jikos@...nel.org>,
	Miroslav Benes <mbenes@...e.cz>,
	Ingo Molnar <mingo@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Michael Ellerman <mpe@...erman.id.au>,
	Heiko Carstens <heiko.carstens@...ibm.com>,
	live-patching@...r.kernel.org, linux-kernel@...r.kernel.org,
	x86@...nel.org, linuxppc-dev@...ts.ozlabs.org,
	linux-s390@...r.kernel.org, Vojtech Pavlik <vojtech@...e.com>,
	Jiri Slaby <jslaby@...e.cz>,
	Chris J Arges <chris.j.arges@...onical.com>,
	Andy Lutomirski <luto@...nel.org>
Subject: Re: barriers: was: [RFC PATCH v2 17/18] livepatch: change to a
 per-task consistency model

On Wed 2016-05-04 12:25:17, Josh Poimboeuf wrote:
> On Wed, May 04, 2016 at 04:12:05PM +0200, Petr Mladek wrote:
> > On Wed 2016-05-04 14:39:40, Petr Mladek wrote:
> > > 		 *
> > > 		 * Note that the task must never be migrated to the target
> > > 		 * state when being inside this ftrace handler.
> > > 		 */
> > > 
> > > We might want to move the second paragraph on top of the function.
> > > It is a basic and important fact. It actually explains why the first
> > > read barrier is not needed when the patch is being disabled.
> > 
> > I wrote the statement partly intuitively. I think that it is really
> > somehow important. And I am slightly in doubts if we are on the safe side.
> > 
> > First, why is it important that the task->patch_state is not switched
> > when being inside the ftrace handler?
> > 
> > If we are inside the handler, we are kind-of inside the called
> > function. And the basic idea of this consistency model is that
> > we must not switch a task when it is inside a patched function.
> > This is normally decided by the stack.
> > 
> > The handler is a bit special because it is called right before the
> > function. If it was the only patched function on the stack, it would
> > not matter if we choose the new or old code. Both decisions would
> > be safe for the moment.
> > 
> > The fun starts when the function calls another patched function.
> > The other patched function must be called consistently with
> > the first one. If the first function was from the patch,
> > the other must be from the patch as well and vice versa.
> > 
> > This is why we must not switch task->patch_state dangerously
> > when being inside the ftrace handler.
> > 
> > Now I am not sure if this condition is fulfilled. The ftrace handler
> > is called as the very first instruction of the function. Does not
> > it break the stack validity? Could we sleep inside the ftrace
> > handler? Will the patched function be detected on the stack?
> > 
> > Or is my brain already too far in the fantasy world?
> 
> I think this isn't a possibility.
> 
> In today's code base, this can't happen because task patch states are
> only switched when sleeping or when exiting the kernel.  The ftrace
> handler doesn't sleep directly.
> 
> If it were preempted, it couldn't be switched there either because we
> consider preempted stacks to be unreliable.

This was the missing piece.

> In theory, a DWARF stack trace of a preempted task *could* be reliable.
> But then the DWARF unwinder should be smart enough to see that the
> original function called the ftrace handler.  Right?  So the stack would
> be reliable, but then livepatch would see the original function on the
> stack and wouldn't switch the task.
> 
> Does that make sense?

Yup. I think that we are on the safe side. Thanks for explanation.

Best Regards,
Petr