[RFC] [PATCH] To refuse users from probing preempt_schedule()

[RFC] [PATCH] To refuse users from probing preempt_schedule()

[srinivasa]

From: Srinivasa Ds <srinivasa@in.ibm.com>

This patch prohibits user from probing preempt_schedule(). One way of
prohibiting the user from probing functions is by marking such 
functions with __kprobes. But this method doesn't work for those functions,
which are already marked to different section like preempt_schedule()
(belongs to __sched section). So we use blacklist approach to refuse user
from probing these functions.

In blacklist approach we populate the blacklisted function's starting
address and its size in kprobe_blacklist structure. Then we verify the user
specified address against start and end of the blacklisted function.
So any attempt to register probe on blacklisted functions will be rejected.

please let me know your comments.

Signed-off-by: Srinivasa DS <srinivasa@in.ibm.com> 
Signed-off-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Signed-off-by: Jim Keniston <jkenisto@us.ibm.com> 


---
 include/linux/kprobes.h |    7 ++++++
 kernel/kprobes.c        |   52 
++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

Index: linux-2.6.25-rc3/include/linux/kprobes.h
===================================================================
--- linux-2.6.25-rc3.orig/include/linux/kprobes.h
+++ linux-2.6.25-rc3/include/linux/kprobes.h
@@ -173,6 +173,13 @@ struct kretprobe_blackpoint {
 	const char *name;
 	void *addr;
 };
+
+struct kprobe_blackpoint {
+	const char *name;
+	unsigned long start_addr;
+	unsigned long range;
+};
+
 extern struct kretprobe_blackpoint kretprobe_blacklist[];
 
 static inline void kretprobe_assert(struct kretprobe_instance *ri,
Index: linux-2.6.25-rc3/kernel/kprobes.c
===================================================================
--- linux-2.6.25-rc3.orig/kernel/kprobes.c
+++ linux-2.6.25-rc3/kernel/kprobes.c
@@ -89,6 +89,18 @@ struct kprobe_insn_page {
 	int ngarbage;
 };
 
+/*
+ * Normally, functions that we'd want to prohibit kprobes in, are marked
+ * __kprobes. But, there are cases where such functions already belong to
+ * a different section (__sched for preempt_schedule)
+ *
+ * For such cases, we now have a blacklist
+ */
+struct kprobe_blackpoint kprobe_blacklist[] = {
+        {"preempt_schedule",},
+        {NULL}    /* Terminator */
+};
+
 enum kprobe_slot_state {
 	SLOT_CLEAN = 0,
 	SLOT_DIRTY = 1,
@@ -492,9 +504,22 @@ static int __kprobes register_aggr_kprob
 
 static int __kprobes in_kprobes_functions(unsigned long addr)
 {
+	struct kprobe_blackpoint *kb;
+
 	if (addr >= (unsigned long)__kprobes_text_start &&
 	    addr < (unsigned long)__kprobes_text_end)
 		return -EINVAL;
+	/*
+	 * If there exists a kprobe_blacklist, verify and
+	 * fail any probe registration in the prohibited area
+	 */
+	for (kb = kprobe_blacklist; kb->name != NULL; kb++) {
+		if (kb->start_addr) {
+			if (addr >= kb->start_addr &&
+			    addr < (kb->start_addr + kb->range))
+				return -EINVAL;
+		}
+	}
 	return 0;
 }
 
@@ -805,6 +830,11 @@ void __kprobes unregister_kretprobe(stru
 static int __init init_kprobes(void)
 {
 	int i, err = 0;
+	unsigned long offset = 0, size = 0;
+	char *modname, namebuf[128];
+	const char *symbol_name;
+	void *addr;
+	struct kprobe_blackpoint *kb;
 
 	/* FIXME allocate the probe table, currently defined statically */
 	/* initialize all list heads */
@@ -813,6 +843,28 @@ static int __init init_kprobes(void)
 		INIT_HLIST_HEAD(&kretprobe_inst_table[i]);
 	}
 
+	/*
+	 * Lookup and populate the kprobe_blacklist.
+	 *
+	 * Unlike the kretprobe blacklist, we'll need to determine
+	 * the range of addresses that belong to the said functions,
+	 * since a kprobe need not necessarily be at the beginning
+	 * of a function.
+	 */
+	for (kb = kprobe_blacklist; kb->name != NULL; kb++) {
+		kprobe_lookup_name(kb->name, addr);
+		if (!addr)
+			continue;
+
+		kb->start_addr = (unsigned long)addr;
+		symbol_name = kallsyms_lookup(kb->start_addr,
+				&size, &offset, &modname, namebuf);
+		if (!symbol_name)
+			kb->range = 0;
+		else
+			kb->range = size;
+	}
+
 	if (kretprobe_blacklist_size) {
 		/* lookup the function address from its name */
 		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {

Re: [RFC] [PATCH] To refuse users from probing preempt_schedule()

[Dave Hansen]

On Mon, 2008-02-25 at 11:27 +0530, srinivasa wrote:
> This patch prohibits user from probing preempt_schedule(). One way of
> prohibiting the user from probing functions is by marking such 
> functions with __kprobes. But this method doesn't work for those functions,
> which are already marked to different section like preempt_schedule()
> (belongs to __sched section). So we use blacklist approach to refuse user
> from probing these functions.

preempt_schedule() does sound really, really important.  But, what kinds
of functions can't be kprobed?  It would be nice to give that blacklist
a nice comment on the topic. :)

Also, have you strained your brains to think of other functions that
this should be applied to?  Is it just for functions that are sensitive
and already have an assigned section?  Might be nice to call that out
explicitly.

-- Dave

Re: [RFC] [PATCH] To refuse users from probing preempt_schedule()

[Srinivasa DS]

On Monday 25 February 2008 11:50:24 am Dave Hansen wrote:
> On Mon, 2008-02-25 at 11:27 +0530, srinivasa wrote:
> > This patch prohibits user from probing preempt_schedule(). One way of
> > prohibiting the user from probing functions is by marking such
> > functions with __kprobes. But this method doesn't work for those
> > functions, which are already marked to different section like
> > preempt_schedule() (belongs to __sched section). So we use blacklist
> > approach to refuse user from probing these functions.
>
> preempt_schedule() does sound really, really important.  But, what kinds
> of functions can't be kprobed?  
      
Normally we don't allow user to probe those functions, which are used by 
kprobes internally while registering probe on user specified address.  For 
example kprobes internally makes use of preempt_disable()(this in turn calls 
add_preempt_count()), so we prohibit users from probing add_preempt_count() 
function.  To get comprehensive list of functions which are prohibited from 
probing, please have a look at functions which are marked under __kprobes. 
 
> It would be nice to give that blacklist 
> a nice comment on the topic. :)

Yes, I  have added comments on the blacklist approach in 
     1)init_kprobes(), where we populate entries  for kprobe blacklist.
     2) in_kprobe_function(), where we verify the user specified address with 
the start and end of the blacklisted function.

>
> Also, have you strained your brains to think of other functions that
> this should be applied to?  Is it just for functions that are sensitive
> and already have an assigned section? 

Yes, this kprobes blacklist approach is only for those functions which are 
sensitive and are already assigned to different sections. Right now, 
there is no other function, except preempt_schedule() which satisfies above 
condition. But in future if we encounter any such functions we surely add 
them kprobe blacklist.

Re: [RFC] [PATCH] To refuse users from probing preempt_schedule()

[Andrew Morton]

On Mon, 25 Feb 2008 11:27:40 +0530 srinivasa <srinivasa@in.ibm.com> wrote:

> From: Srinivasa Ds <srinivasa@in.ibm.com>
> 
> This patch prohibits user from probing preempt_schedule(). One way of
> prohibiting the user from probing functions is by marking such 
> functions with __kprobes. But this method doesn't work for those functions,
> which are already marked to different section like preempt_schedule()
> (belongs to __sched section). So we use blacklist approach to refuse user
> from probing these functions.
> 
> In blacklist approach we populate the blacklisted function's starting
> address and its size in kprobe_blacklist structure. Then we verify the user
> specified address against start and end of the blacklisted function.
> So any attempt to register probe on blacklisted functions will be rejected.
> 
> please let me know your comments.
> 

sparc64:

kernel/kprobes.c: In function `in_kprobes_functions':
kernel/kprobes.c:516: error: `kprobe_blacklist' undeclared (first use in this function)
kernel/kprobes.c:516: error: (Each undeclared identifier is reported only once
kernel/kprobes.c:516: error: for each function it appears in.)
kernel/kprobes.c: In function `init_kprobes':
kernel/kprobes.c:860: error: `kprobe_blacklist' undeclared (first use in this function)


> ---
>  include/linux/kprobes.h |    7 ++++++
>  kernel/kprobes.c        |   52 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+)
> 
> Index: linux-2.6.25-rc3/include/linux/kprobes.h
> ===================================================================
> --- linux-2.6.25-rc3.orig/include/linux/kprobes.h
> +++ linux-2.6.25-rc3/include/linux/kprobes.h
> @@ -173,6 +173,13 @@ struct kretprobe_blackpoint {
>  	const char *name;
>  	void *addr;
>  };
> +
> +struct kprobe_blackpoint {
> +	const char *name;
> +	unsigned long start_addr;
> +	unsigned long range;
> +};
> +

I'm assuming that the placement of this definition inside
__ARCH_WANT_KPROBES_INSN_SLOT wqs accidental?

--- a/kernel/kprobes.c~kprobes-prevent-probing-of-preempt_schedule-fix
+++ a/kernel/kprobes.c
@@ -72,6 +72,18 @@ DEFINE_MUTEX(kprobe_mutex);		/* Protects
 DEFINE_SPINLOCK(kretprobe_lock);	/* Protects kretprobe_inst_table */
 static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
 
+/*
+ * Normally, functions that we'd want to prohibit kprobes in, are marked
+ * __kprobes. But, there are cases where such functions already belong to
+ * a different section (__sched for preempt_schedule)
+ *
+ * For such cases, we now have a blacklist
+ */
+struct kprobe_blackpoint kprobe_blacklist[] = {
+        {"preempt_schedule",},
+        {NULL}    /* Terminator */
+};
+
 #ifdef __ARCH_WANT_KPROBES_INSN_SLOT
 /*
  * kprobe->ainsn.insn points to the copy of the instruction to be
@@ -89,18 +101,6 @@ struct kprobe_insn_page {
 	int ngarbage;
 };
 
-/*
- * Normally, functions that we'd want to prohibit kprobes in, are marked
- * __kprobes. But, there are cases where such functions already belong to
- * a different section (__sched for preempt_schedule)
- *
- * For such cases, we now have a blacklist
- */
-struct kprobe_blackpoint kprobe_blacklist[] = {
-        {"preempt_schedule",},
-        {NULL}    /* Terminator */
-};
-
 enum kprobe_slot_state {
 	SLOT_CLEAN = 0,
 	SLOT_DIRTY = 1,
_

Re: [RFC] [PATCH] To refuse users from probing preempt_schedule()

[Srinivasa DS]

On Tuesday 04 March 2008 01:55:21 pm Andrew Morton wrote:
> On Mon, 25 Feb 2008 11:27:40 +0530 srinivasa <srinivasa@in.ibm.com> wrote:
> > From: Srinivasa Ds <srinivasa@in.ibm.com>
> >
> > This patch prohibits user from probing preempt_schedule(). One way of
> > prohibiting the user from probing functions is by marking such
> > functions with __kprobes. But this method doesn't work for those
> > functions, which are already marked to different section like
> > preempt_schedule() (belongs to __sched section). So we use blacklist
> > approach to refuse user from probing these functions.
> >
> > In blacklist approach we populate the blacklisted function's starting
> > address and its size in kprobe_blacklist structure. Then we verify the
> > user specified address against start and end of the blacklisted function.
> > So any attempt to register probe on blacklisted functions will be
> > rejected.
> >
> > please let me know your comments.
>
> sparc64:
>
> kernel/kprobes.c: In function `in_kprobes_functions':
> kernel/kprobes.c:516: error: `kprobe_blacklist' undeclared (first use in
> this function) kernel/kprobes.c:516: error: (Each undeclared identifier is
> reported only once kernel/kprobes.c:516: error: for each function it
> appears in.)
> kernel/kprobes.c: In function `init_kprobes':
> kernel/kprobes.c:860: error: `kprobe_blacklist' undeclared (first use in
> this function)
>
> > ---
> >  include/linux/kprobes.h |    7 ++++++
> >  kernel/kprobes.c        |   52
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 59 insertions(+)
> >
> > Index: linux-2.6.25-rc3/include/linux/kprobes.h
> > ===================================================================
> > --- linux-2.6.25-rc3.orig/include/linux/kprobes.h
> > +++ linux-2.6.25-rc3/include/linux/kprobes.h
> > @@ -173,6 +173,13 @@ struct kretprobe_blackpoint {
> >  	const char *name;
> >  	void *addr;
> >  };
> > +
> > +struct kprobe_blackpoint {
> > +	const char *name;
> > +	unsigned long start_addr;
> > +	unsigned long range;
> > +};
> > +
>
> I'm assuming that the placement of this definition inside
> __ARCH_WANT_KPROBES_INSN_SLOT wqs accidental?

Yes Andrew.  Sorry I over looked __ARCH_WANT_KPROBES_INSN_SLOT and placed the 
struct kprobe_black_point in a wrong place. 

    Patch, that you have attached below looks good, but for your reference, 
Iam attaching the modified patch below.

>
> --- a/kernel/kprobes.c~kprobes-prevent-probing-of-preempt_schedule-fix
> +++ a/kernel/kprobes.c
> @@ -72,6 +72,18 @@ DEFINE_MUTEX(kprobe_mutex);		/* Protects
>  DEFINE_SPINLOCK(kretprobe_lock);	/* Protects kretprobe_inst_table */
>  static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
>
> +/*
> + * Normally, functions that we'd want to prohibit kprobes in, are marked
> + * __kprobes. But, there are cases where such functions already belong to
> + * a different section (__sched for preempt_schedule)
> + *
> + * For such cases, we now have a blacklist
> + */
> +struct kprobe_blackpoint kprobe_blacklist[] = {
> +        {"preempt_schedule",},
> +        {NULL}    /* Terminator */
> +};
> +
>  #ifdef __ARCH_WANT_KPROBES_INSN_SLOT
>  /*
>   * kprobe->ainsn.insn points to the copy of the instruction to be
> @@ -89,18 +101,6 @@ struct kprobe_insn_page {
>  	int ngarbage;
>  };
>
> -/*
> - * Normally, functions that we'd want to prohibit kprobes in, are marked
> - * __kprobes. But, there are cases where such functions already belong to
> - * a different section (__sched for preempt_schedule)
> - *
> - * For such cases, we now have a blacklist
> - */
> -struct kprobe_blackpoint kprobe_blacklist[] = {
> -        {"preempt_schedule",},
> -        {NULL}    /* Terminator */
> -};
> -
>  enum kprobe_slot_state {
>  	SLOT_CLEAN = 0,
>  	SLOT_DIRTY = 1,
> _


---
 include/linux/kprobes.h |    7 ++++++
 kernel/kprobes.c        |   51 
++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

Index: linux-2.6.25-rc3/include/linux/kprobes.h
===================================================================
--- linux-2.6.25-rc3.orig/include/linux/kprobes.h
+++ linux-2.6.25-rc3/include/linux/kprobes.h
@@ -173,6 +173,13 @@ struct kretprobe_blackpoint {
 	const char *name;
 	void *addr;
 };
+
+struct kprobe_blackpoint {
+	const char *name;
+	unsigned long start_addr;
+	unsigned long range;
+};
+
 extern struct kretprobe_blackpoint kretprobe_blacklist[];
 
 static inline void kretprobe_assert(struct kretprobe_instance *ri,
Index: linux-2.6.25-rc3/kernel/kprobes.c
===================================================================
--- linux-2.6.25-rc3.orig/kernel/kprobes.c
+++ linux-2.6.25-rc3/kernel/kprobes.c
@@ -72,6 +72,17 @@ DEFINE_MUTEX(kprobe_mutex);		/* Protects
 DEFINE_SPINLOCK(kretprobe_lock);	/* Protects kretprobe_inst_table */
 static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
 
+/*
+ * Normally, functions that we'd want to prohibit kprobes in, are marked
+ * __kprobes. But, there are cases where such functions already belong to
+ * a different section (__sched for preempt_schedule)
+ *
+ * For such cases, we now have a blacklist
+ */
+struct kprobe_blackpoint kprobe_blacklist[] = {
+        {"preempt_schedule",},
+        {NULL}    /* Terminator */
+
 #ifdef __ARCH_WANT_KPROBES_INSN_SLOT
 /*
  * kprobe->ainsn.insn points to the copy of the instruction to be
@@ -492,9 +503,22 @@ static int __kprobes register_aggr_kprob
 
 static int __kprobes in_kprobes_functions(unsigned long addr)
 {
+	struct kprobe_blackpoint *kb;
+
 	if (addr >= (unsigned long)__kprobes_text_start &&
 	    addr < (unsigned long)__kprobes_text_end)
 		return -EINVAL;
+	/*
+	 * If there exists a kprobe_blacklist, verify and
+	 * fail any probe registration in the prohibited area
+	 */
+	for (kb = kprobe_blacklist; kb->name != NULL; kb++) {
+		if (kb->start_addr) {
+			if (addr >= kb->start_addr &&
+			    addr < (kb->start_addr + kb->range))
+				return -EINVAL;
+		}
+	}
 	return 0;
 }
 
@@ -805,6 +829,11 @@ void __kprobes unregister_kretprobe(stru
 static int __init init_kprobes(void)
 {
 	int i, err = 0;
+	unsigned long offset = 0, size = 0;
+	char *modname, namebuf[128];
+	const char *symbol_name;
+	void *addr;
+	struct kprobe_blackpoint *kb;
 
 	/* FIXME allocate the probe table, currently defined statically */
 	/* initialize all list heads */
@@ -813,6 +842,28 @@ static int __init init_kprobes(void)
 		INIT_HLIST_HEAD(&kretprobe_inst_table[i]);
 	}
 
+	/*
+	 * Lookup and populate the kprobe_blacklist.
+	 *
+	 * Unlike the kretprobe blacklist, we'll need to determine
+	 * the range of addresses that belong to the said functions,
+	 * since a kprobe need not necessarily be at the beginning
+	 * of a function.
+	 */
+	for (kb = kprobe_blacklist; kb->name != NULL; kb++) {
+		kprobe_lookup_name(kb->name, addr);
+		if (!addr)
+			continue;
+
+		kb->start_addr = (unsigned long)addr;
+		symbol_name = kallsyms_lookup(kb->start_addr,
+				&size, &offset, &modname, namebuf);
+		if (!symbol_name)
+			kb->range = 0;
+		else
+			kb->range = size;
+	}
+
 	if (kretprobe_blacklist_size) {
 		/* lookup the function address from its name */
 		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {