linux-kernel - Re: [PATCHv3 2/3] kernel: add support for live patching

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: <20141125121606.547031b4@gandalf.local.home>
Date:	Tue, 25 Nov 2014 12:16:06 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Petr Mladek <pmladek@...e.cz>
Cc:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Seth Jennings <sjenning@...hat.com>,
	Josh Poimboeuf <jpoimboe@...hat.com>,
	Jiri Kosina <jkosina@...e.cz>,
	Vojtech Pavlik <vojtech@...e.cz>,
	Miroslav Benes <mbenes@...e.cz>,
	Christoph Hellwig <hch@...radead.org>,
	Greg KH <gregkh@...uxfoundation.org>,
	Andy Lutomirski <luto@...capital.net>,
	live-patching@...r.kernel.org, x86@...nel.org, kpatch@...hat.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCHv3 2/3] kernel: add support for live patching

On Tue, 25 Nov 2014 18:04:31 +0100
Petr Mladek <pmladek@...e.cz> wrote:

> On Tue 2014-11-25 11:52:10, Steven Rostedt wrote:
> > On Tue, 25 Nov 2014 17:39:43 +0100
> > Petr Mladek <pmladek@...e.cz> wrote:
> > 
> > > On Fri 2014-11-21 11:39:24, Masami Hiramatsu wrote:
> > > > (2014/11/21 7:29), Seth Jennings wrote:
> > > > > This commit introduces code for the live patching core.  It implements
> > > > > an ftrace-based mechanism and kernel interface for doing live patching
> > > > > of kernel and kernel module functions.
> > > > > 
> > > > > It represents the greatest common functionality set between kpatch and
> > > > > kgraft and can accept patches built using either method.
> > > > > 
> > > > > This first version does not implement any consistency mechanism that
> > > > > ensures that old and new code do not run together.  In practice, ~90% of
> > > > > CVEs are safe to apply in this way, since they simply add a conditional
> > > > > check.  However, any function change that can not execute safely with
> > > > > the old version of the function can _not_ be safely applied in this
> > > > > version.
> > > > 
> > > > Thanks for updating :)
> > > > 
> > > > BTW, this still have some LPC_XXX macros, those should be KLP_XXX.
> > > > 
> > > > Also, as I sent a series of IPMODIFY patches (just now), could you consider
> > > > to use the flag? :)
> > > 
> > > Hmm, it would cause problems with the current LivePatch, kGraft
> > > implementation, and probably also with kPatch. They register more
> > > than one ftrace handler with IPMODIFY at the same time.
> > 
> > But are they hooked to the same functions? That would be a big problem,
> > and should be avoided. Why would you want too ftrace_ops returning two
> > different IPs for one function? That causes a paradox. Why would you
> > want that?
> 
> We does not mind which one wins. The two functions are registered only
> temporarily. It is guaranteed that they both sets the same regs->ip
> address during this time frame.

It is not guaranteed from ftrace's stand point. What happens if we have
a kprobe handler that modifies it for someplace else? Changing the ip
address may not be a kpatch/kGraft privilege only.

> 
> 
> > > They pass pointer to the func-related structure via the "private" field
> > > in struct ftrace_ops. The structure provides information where the old
> > > and new code is.
> > > 
> > > They need to update the structure when new patch for the same functions
> > > appears. It is done by registering a new ftrace function related to the
> > > new patch and unregistering an old ftrace function from the old patch.
> > > 
> > > We would need to maintain some patch-independent list of ftrace_ops
> > > and the related private fields to avoid the double registration.
> > 
> > Yes, that would make sense.
> > 
> > You could create one ftrace_ops per function. That would be ideal
> > because then you get to take advantage of having your own trampoline
> > per function and no need to worry about what function needs to go with
> > another function.
> 
> I adds some complexity but I think that we will need to go this way.
> The check for IPMODIFY conflicts makes sense. It helps to avoid any
> misuse.

Right.

> 
> My main intention was to point out the problem and that we would need
> to handle it somehow  J

Great! J

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/