linux-kernel - Re: [PATCH v2] kprobes: Disable Kprobe when ftrace arming fails

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] [day] [month] [year] [list]

Message-ID: <20150319125701.4202ee77@gandalf.local.home>
Date:	Thu, 19 Mar 2015 12:57:01 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Petr Mladek <pmladek@...e.cz>
Cc:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	"David S. Miller" <davem@...emloft.net>,
	Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
	Ananth NMavinakayanahalli <ananth@...ibm.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...hat.com>, Jiri Kosina <jkosina@...e.cz>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] kprobes: Disable Kprobe when ftrace arming fails

On Mon, 16 Mar 2015 16:27:47 +0100
Petr Mladek <pmladek@...e.cz> wrote:

> arm_kprobe_ftrace() could fail, especially after introducing ftrace IPMODIFY
> flag and LifePatching. But this situation is not properly handled.
> This patch adds the most important changes.
> 
> First, it does not make sense to register "kprobe_ftrace_ops" if the filter was
> not set.
> 
> Second, we should remove the filter if the registration of "kprobe_ftrace_ops"
> fails. The failure might be caused by conflict between the Kprobe and
> a life patch via the IPMODIFY flag. If we remove the filter, we will allow
> to register "kprobe_ftrace_ops" for another non-conflicting Kprobe later.
> 
> Third, we need to make sure that "kprobe_ftrace_enabled" is incremented only
> when "kprobe_ftrace_ops" is successfully registered. Otherwise, another
> Kprobe will not try to register it again. Note that we could move the
> manipulation with this counter because it is accessed only under "kprobe_mutex".
> 
> Four, we should mark the probe as disabled if the ftrace stuff is not usable.
> It will be the correct status. Also it will prevent the unregistration code
> from producing another failure.
> 
> It looks more safe to disable the Kprobe directly in "kprobe_ftrace_ops". Note
> that we need to disable also all listed Kprobes in case of an aggregated probe.
> It would be enough to disable only the new one but we do not know which one it
> was. They should be in sync anyway.
> 

I can pull this into my tree. But there's one little nit below.

> Signed-off-by: Petr Mladek <pmladek@...e.cz>
> Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
> ---
> Hi,
> 
> I resend this patch separately and have just added the acked by Masami.
> 
> Best Regards,
> Petr
> 
>  kernel/kprobes.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index c90e417bb963..54b22db084f3 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -932,16 +932,33 @@ static int prepare_kprobe(struct kprobe *p)
>  /* Caller must lock kprobe_mutex */
>  static void arm_kprobe_ftrace(struct kprobe *p)
>  {
> +	struct kprobe *kp;
>  	int ret;
>  
>  	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
>  				   (unsigned long)p->addr, 0, 0);
> -	WARN(ret < 0, "Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
> -	kprobe_ftrace_enabled++;
> -	if (kprobe_ftrace_enabled == 1) {
> +	if (WARN(ret < 0,
> +		 "Failed to arm kprobe-ftrace at %p (%d). The kprobe gets disabled.\n",
> +		 p->addr, ret))
> +		goto err_filter;
> +
> +	if (!kprobe_ftrace_enabled) {
>  		ret = register_ftrace_function(&kprobe_ftrace_ops);
> -		WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
> +		if (WARN(ret < 0,
> +			 "Failed to init kprobe-ftrace (%d). The probe at %p gets disabled\n",
> +			 ret, p->addr))
> +			goto err_function;
>  	}
> +	kprobe_ftrace_enabled++;
> +	return;
> +
> +err_function:
> +	ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0);
> +err_filter:
> +	p->flags |= KPROBE_FLAG_DISABLED;
> +	if (kprobe_aggrprobe(p))
> +		list_for_each_entry_rcu(kp, &p->list, list)
> +			kp->flags |= KPROBE_FLAG_DISABLED;

The if statement should have brackets. No bracket if and for statements
should only be used when there's a singe simple non complex line below.
This is not the case.

Thanks,

-- Steve


>  }
>  
>  /* Caller must lock kprobe_mutex */

--
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/