linux-kernel - Re: [PATCH 1/2 v3] ftrace: don't allow IPMODIFY without proper compiler support

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: <20150116113539.78c093ad@gandalf.local.home>
Date:	Fri, 16 Jan 2015 11:35:39 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Jiri Kosina <jkosina@...e.cz>
Cc:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Stephen Rothwell <sfr@...b.auug.org.au>,
	Josh Poimboeuf <jpoimboe@...hat.com>,
	Christoph Hellwig <hch@...radead.org>,
	linux-kernel@...r.kernel.org, live-patching@...r.kernel.org,
	linux-next@...r.kernel.org
Subject: Re: [PATCH 1/2 v3] ftrace: don't allow IPMODIFY without proper
 compiler support

On Thu, 15 Jan 2015 10:50:07 +0100 (CET)
Jiri Kosina <jkosina@...e.cz> wrote:

> Using IPMODIFY needs to be allowed only with compilers which are 
> guaranteed to generate function prologues compatible with function 
> redirection through changing instruction pointer in saved regs.

That's actually not true.

Sorry, I started thinking about this more, and I disagree with this
change.

> 
> For example changing regs->ip on x86_64 in cases when gcc is using mcount 
> (and not fentry) is not allowed, because at the time mcount call is 
> issued, the original function's prologue has already been executed, which 
> leads to all kinds of inconsistent havoc.

No, it only causes havoc if whoever modifies the ip doesn't know it's
not at the start of the function.

Hence, it's only the user of ftrace that wants to replace functions
that needs to worry about this.

In fact, there's nothing wrong if kprobes to use ftrace to change ip
even if fentry isn't supported. That's because if fentry isn't
supported, kprobes will notice that there's no ftrace call at the start
of a function and will use a breakpoint instead. If a kprobe user still
wants to change the ip after the stack frame was set up, they still can
do that, they just need to find where the mcount call is. kprobe does
its work based on the kernel address to probe, not where ftrace places
its hooks.

Now you could argue that there's no reason to change ip if ftrace isn't
using fentry, but there's nothing to prevent a user from doing so.

It also bothers me that you just prevented all users of kprobes from
taking advantage of hooking to a ftrace caller if fentry isn't
supported. All kprobes really needs is the REGS version supported. 99%
of all kprobes do not modify ip.

I have to say NAK on this change.

Instead, make live kernel patching fail to load if fentry isn't
supported. IOW, instead of ftrace_ipmodify_supported, have a
live_kernel_patching_supported that could be based on fentry being used
or not.

-- Steve

> 
> There is currently no way to express dependency on gcc features in 
> Kconfig, (CC_USING_FENTRY is defined only during build, so it's not 
> possible for Kconfig symbol to depend on it) so this needs to be checked 
> in runtime.
> 
> Mark x86_64 with fentry supported for now. Other archs can be added
> gradually.
> 
> Signed-off-by: Jiri Kosina <jkosina@...e.cz>
--
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/