linux-kernel - Re: [PATCH -tip v9 20/26] kprobes: Support blacklist functions in module

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: <20140424085601.GA7768@gmail.com>
Date:	Thu, 24 Apr 2014 10:56:01 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
Cc:	linux-kernel@...r.kernel.org,
	Rusty Russell <rusty@...tcorp.com.au>,
	Andi Kleen <andi@...stfloor.org>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Sandeepa Prabhu <sandeepa.prabhu@...aro.org>,
	Frederic Weisbecker <fweisbec@...il.com>, x86@...nel.org,
	Steven Rostedt <rostedt@...dmis.org>, fche@...hat.com,
	mingo@...hat.com, Rob Landley <rob@...dley.net>,
	"H. Peter Anvin" <hpa@...or.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"David S. Miller" <davem@...emloft.net>, systemtap@...rceware.org
Subject: Re: [PATCH -tip v9 20/26] kprobes: Support blacklist functions in
 module


* Masami Hiramatsu <masami.hiramatsu.pt@...achi.com> wrote:

> To blacklist the functions in a module (e.g. user-defined
> kprobe handler and the functions invoked from it), expand
> blacklist support for modules.
> With this change, users can use NOKPROBE_SYMBOL() macro in
> their own modules.
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
> Cc: Ananth N Mavinakayanahalli <ananth@...ibm.com>
> Cc: "David S. Miller" <davem@...emloft.net>
> Cc: Rob Landley <rob@...dley.net>
> Cc: Rusty Russell <rusty@...tcorp.com.au>
> ---
>  Documentation/kprobes.txt |    8 ++++++
>  include/linux/module.h    |    5 ++++
>  kernel/kprobes.c          |   63 ++++++++++++++++++++++++++++++++++++++-------
>  kernel/module.c           |    6 ++++
>  4 files changed, 72 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
> index 4bbeca8..2845956 100644
> --- a/Documentation/kprobes.txt
> +++ b/Documentation/kprobes.txt
> @@ -512,6 +512,14 @@ int enable_jprobe(struct jprobe *jp);
>  Enables *probe which has been disabled by disable_*probe(). You must specify
>  the probe which has been registered.
>  
> +4.9 NOKPROBE_SYMBOL()
> +
> +#include <linux/kprobes.h>
> +NOKPROBE_SYMBOL(FUNCTION);
> +
> +Protects given FUNCTION from other kprobes. This is useful for handler
> +functions and functions called from the handlers.
> +
>  5. Kprobes Features and Limitations
>  
>  Kprobes allows multiple probes at the same address.  Currently,
> diff --git a/include/linux/module.h b/include/linux/module.h
> index f520a76..2fdb673 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -16,6 +16,7 @@
>  #include <linux/kobject.h>
>  #include <linux/moduleparam.h>
>  #include <linux/jump_label.h>
> +#include <linux/kprobes.h>

This include breaks the x86 build:

  CC      arch/x86/kernel/jump_label.o
  In file included from arch/x86/kernel/jump_label.c:14:0:
  /fast/mingo/tip/arch/x86/include/asm/kprobes.h:35:12: error: conflicting types for ‘kprobe_opcode_t' typedef u8 kprobe_opcode_t;
  [...]

But the #include kprobes.h is unnecessary to begin with, as no kprobe 
specific types are used.

>  #include <linux/export.h>
>  
>  #include <linux/percpu.h>
> @@ -357,6 +358,10 @@ struct module {
>  	unsigned int num_ftrace_callsites;
>  	unsigned long *ftrace_callsites;
>  #endif
> +#ifdef CONFIG_KPROBES
> +	unsigned int num_kprobe_blacklist;
> +	unsigned long  *kprobe_blacklist;
> +#endif

There's a small coding style problem here.

More importantly, I think more should be done to make sure that module 
symbols are marked properly: since the module is going to register the 
kprobes handler, that would be a perfect place to emit a warning, 
right?

In fact, why don't kprobe handlers get added to the exclusion list 
explicitly, when the handler gets registered? With such an approach 
handlers are automatically nokprobe and don't need any annotation - 
which is a far more robust usage model.

So I'm skipping this patch and the next one that makes use of it.

Thanks,

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