[RFC][PATCH] kprobes/x86: While list ftrace locations in kprobe blacklist areas

[RFC][PATCH] kprobes/x86: While list ftrace locations in kprobe blacklist areas

[Steven Rostedt]

From: Steven Rostedt (VMware) <rostedt@goodmis.org>

I noticed some of my old tests failing on kprobes, and realized that
this was due to black listing irq_entry functions on x86 from being
used by kprobes. IIRC, this was due to the cr2 being corrupted and
such, and I believe other things were to cause. But black listing all
irq_entry code is a big hammer to this.

 (See commit 0eae81dc9f026 "x86/kprobes: Prohibit probing on IRQ
 handlers directly" for more details)

Anyway, if kprobes is using ftrace as a hook, there shouldn't be any
problems here. If we white list ftrace locations in the range of
kprobe_add_area_blacklist(), it should be safe.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index d9770a5393c8..9d28a279282c 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2124,6 +2124,11 @@ int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
 	int ret = 0;
 
 	for (entry = start; entry < end; entry += ret) {
+#ifdef CONFIG_KPROBES_ON_FTRACE
+		/* We are safe if using ftrace */
+		if (ftrace_location(entry))
+			continue;
+#endif
 		ret = kprobe_add_ksym_blacklist(entry);
 		if (ret < 0)
 			return ret;

Re: [RFC][PATCH] kprobes/x86: While list ftrace locations in kprobe blacklist areas

[Steven Rostedt]

On Thu, 10 Oct 2019 17:52:16 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> I noticed some of my old tests failing on kprobes, and realized that
> this was due to black listing irq_entry functions on x86 from being
> used by kprobes. IIRC, this was due to the cr2 being corrupted and
> such, and I believe other things were to cause. But black listing all
> irq_entry code is a big hammer to this.
> 
>  (See commit 0eae81dc9f026 "x86/kprobes: Prohibit probing on IRQ
>  handlers directly" for more details)

BTW, I noticed this recently (again) when running my tests by hand. I
forgot that I have my automated tests revert the above commit before
compiling the kernel it is about to test (because it tests kprobes on
irq entry locations!). My tests never had issues with kprobes on irq
entry locations.

-- Steve

Re: [RFC][PATCH] kprobes/x86: While list ftrace locations in kprobe blacklist areas

[Masami Hiramatsu]

Hi Steve,

On Thu, 10 Oct 2019 17:52:16 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> I noticed some of my old tests failing on kprobes, and realized that
> this was due to black listing irq_entry functions on x86 from being
> used by kprobes. IIRC, this was due to the cr2 being corrupted and
> such, and I believe other things were to cause. But black listing all
> irq_entry code is a big hammer to this.

OK, I think if we can use ftrace for hooking, probing on "that address"
is good, but the function body still can not be probed.

> 
>  (See commit 0eae81dc9f026 "x86/kprobes: Prohibit probing on IRQ
>  handlers directly" for more details)
> 
> Anyway, if kprobes is using ftrace as a hook, there shouldn't be any
> problems here. If we white list ftrace locations in the range of
> kprobe_add_area_blacklist(), it should be safe.

Agreed.

> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index d9770a5393c8..9d28a279282c 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2124,6 +2124,11 @@ int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
>  	int ret = 0;
>  
>  	for (entry = start; entry < end; entry += ret) {
> +#ifdef CONFIG_KPROBES_ON_FTRACE
> +		/* We are safe if using ftrace */
> +		if (ftrace_location(entry))
> +			continue;
> +#endif

Have you tested the patch? it doesn't measure the entry function's size.
(kprobe_add_ksym_blacklist(entry) returns the function size)

Could you do this in kprobe_add_ksym_blacklist()?
Instead of continue, increment ent->start_addr by MCOUNT size(?) will be OK.
(Note that since each blacklist symbol is managed independently, you can make
 a "gap" between them as a safe area)

Thank you,

>  		ret = kprobe_add_ksym_blacklist(entry);
>  		if (ret < 0)
>  			return ret;


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [RFC][PATCH] kprobes/x86: While list ftrace locations in kprobe blacklist areas

[Steven Rostedt]

On Fri, 11 Oct 2019 17:39:29 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi Steve,
> 
> On Thu, 10 Oct 2019 17:52:16 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > 
> > I noticed some of my old tests failing on kprobes, and realized that
> > this was due to black listing irq_entry functions on x86 from being
> > used by kprobes. IIRC, this was due to the cr2 being corrupted and
> > such, and I believe other things were to cause. But black listing all
> > irq_entry code is a big hammer to this.  
> 
> OK, I think if we can use ftrace for hooking, probing on "that address"
> is good, but the function body still can not be probed.
> 
> > 
> >  (See commit 0eae81dc9f026 "x86/kprobes: Prohibit probing on IRQ
> >  handlers directly" for more details)
> > 
> > Anyway, if kprobes is using ftrace as a hook, there shouldn't be any
> > problems here. If we white list ftrace locations in the range of
> > kprobe_add_area_blacklist(), it should be safe.  
> 
> Agreed.
> 
> > 
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > ---
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index d9770a5393c8..9d28a279282c 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -2124,6 +2124,11 @@ int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
> >  	int ret = 0;
> >  
> >  	for (entry = start; entry < end; entry += ret) {
> > +#ifdef CONFIG_KPROBES_ON_FTRACE
> > +		/* We are safe if using ftrace */
> > +		if (ftrace_location(entry))
> > +			continue;
> > +#endif  
> 
> Have you tested the patch? it doesn't measure the entry function's size.
> (kprobe_add_ksym_blacklist(entry) returns the function size)

Yeah, I tested the patch, but it is obviously buggy. It fixed the issue
I was having (was able to test against do_IRQ ;-)

> 
> Could you do this in kprobe_add_ksym_blacklist()?
> Instead of continue, increment ent->start_addr by MCOUNT size(?) will be OK.
> (Note that since each blacklist symbol is managed independently, you can make
>  a "gap" between them as a safe area)

OK, I'll move it to kprobe_add_ksym_blacklist.

-- Steve

> 
> Thank you,
> 
> >  		ret = kprobe_add_ksym_blacklist(entry);
> >  		if (ret < 0)
> >  			return ret;  
> 
>