linux-kernel - Re: [RFC 03/16] kgr: initial code

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: <53734176.5000003@suse.cz>
Date:	Wed, 14 May 2014 12:12:06 +0200
From:	Jiri Slaby <jslaby@...e.cz>
To:	Aravinda Prasad <aravinda@...ux.vnet.ibm.com>
CC:	linux-kernel@...r.kernel.org, jirislaby@...il.com,
	Vojtech Pavlik <vojtech@...e.cz>, Michael Matz <matz@...e.de>,
	Jiri Kosina <jkosina@...e.cz>,
	Steven Rostedt <rostedt@...dmis.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...hat.com>
Subject: Re: [RFC 03/16] kgr: initial code

On 05/14/2014 11:28 AM, Aravinda Prasad wrote:
> On Wednesday 30 April 2014 08:00 PM, Jiri Slaby wrote:
>> From: Jiri Kosina <jkosina@...e.cz>
>>
>> Provide initial implementation. We are now able to do ftrace-based
>> runtime patching of the kernel code.
>>
>> In addition to that, we will provide a kgr_patcher module in the next
>> patch to test the functionality.
> 
> Hi Jiri,
> 
> Interesting! I have couple of comments:
> 
> I think with kgraft (also with kpatch, though have not looked into
> it yet), the patched function cannot be dynamically ftraced.
> Though dynamic ftrace can be enabled on the new code, the user is
> required to know the function label of the new code. This could
> potentially break existing scripts. I think this should be documented.

Hi,

of course that the functions can be traced. Look, I turned on tracing
for capable, then patched, then turned on tracing for new_capable (which
is the patched function). So now, trace shows:
  console-kit-dae-535   [001] ...1   181.729698: capable <-vt_ioctl
 console-kit-dae-539   [001] ...1   181.729741: capable <-vt_ioctl
 console-kit-dae-541   [000] .N.1   181.906014: capable <-vt_ioctl
         systemd-1     [001] ...1   181.937328: capable <-SyS_epoll_ctl
            sshd-662   [001] ...1   246.437561: capable <-sock_setsockopt
            sshd-662   [001] ...1   246.437564: new_capable
<-sock_setsockopt
            sshd-662   [001] ...1   246.444790: capable <-sock_setsockopt
            sshd-662   [001] ...1   246.444793: new_capable
<-sock_setsockopt
     dbus-daemon-128   [000] .N.1   246.456307: capable <-SyS_epoll_ctl
     dbus-daemon-128   [000] ...1   246.456611: new_capable <-SyS_epoll_ctl


There is no limitation thanks to the use of the ftrace subsystem. We are
just another user, i.e. another piece of code called in a loop for a
particular fentry location.

>> +/*
>> + * The stub needs to modify the RIP value stored in struct pt_regs
>> + * so that ftrace redirects the execution properly.
>> + */
>> +#define KGR_STUB_ARCH_SLOW(_name, _new_function)			\
>> +static void _new_function ##_stub_slow (unsigned long ip, unsigned long parent_ip,	\
>> +		struct ftrace_ops *ops, struct pt_regs *regs)		\
>> +{									\
>> +	struct kgr_loc_caches *c = ops->private;			\
>> +									\
>> +	if (task_thread_info(current)->kgr_in_progress && current->mm) {\
> 
> Is there a race here? The per task kgr_in_progress is set after
> the slow stub is registered in register_ftrace_function(). If the
> patched function is called in between it will be redirected to new code.

Hmm, that looks strange. I will look into that and the other comments
later (and comment separately). Thanks.

-- 
js
suse labs
--
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/