[PATCH] x86/kprobes: Fix KRETPROBES when CONFIG_KRETPROBE_ON_RETHOOK is set

The recent kernel change "kprobes: Use rethook for kretprobe if possible",
introduced a potential NULL pointer dereference bug in the KRETPROBE
mechanism. The official Kprobes documentation defines that "Any or all
handlers can be NULL". Unfortunately, there is a missing return handler
verification to fulfill these requirements and can result in a NULL pointer
dereference bug.

This patch adds such verification in kretprobe_rethook_handler() function.

Fixes: 73f9b911faa7 ("kprobes: Use rethook for kretprobe if possible")
Signed-off-by: Adam Zabrocki <pi3@pi3.com.pl>
---
 kernel/kprobes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index dbe57df2e199..dd58c0be9ce2 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2126,7 +2126,7 @@ static void kretprobe_rethook_handler(struct rethook_node *rh, void *data,
 	struct kprobe_ctlblk *kcb;
 
 	/* The data must NOT be null. This means rethook data structure is broken. */
-	if (WARN_ON_ONCE(!data))
+	if (WARN_ON_ONCE(!data) || !rp->handler)
 		return;
 
 	__this_cpu_write(current_kprobe, &rp->kp);

-- 
pi3 (pi3ki31ny) - pi3 (at) itsec pl
http://pi3.com.pl

Hi Steve,

Can you pick this fix to urgent branch? Or Should I ask bpf people?

On Fri, 15 Apr 2022 20:10:06 +0200
Adam Zabrocki <pi3@pi3.com.pl> wrote:

> [PATCH] x86/kprobes: Fix KRETPROBES when CONFIG_KRETPROBE_ON_RETHOOK is set
> 
> The recent kernel change "kprobes: Use rethook for kretprobe if possible",
> introduced a potential NULL pointer dereference bug in the KRETPROBE
> mechanism. The official Kprobes documentation defines that "Any or all
> handlers can be NULL". Unfortunately, there is a missing return handler
> verification to fulfill these requirements and can result in a NULL pointer
> dereference bug.
> 
> This patch adds such verification in kretprobe_rethook_handler() function.
> 

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you,

> Fixes: 73f9b911faa7 ("kprobes: Use rethook for kretprobe if possible")
> Signed-off-by: Adam Zabrocki <pi3@pi3.com.pl>
> ---
>  kernel/kprobes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index dbe57df2e199..dd58c0be9ce2 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2126,7 +2126,7 @@ static void kretprobe_rethook_handler(struct rethook_node *rh, void *data,
>  	struct kprobe_ctlblk *kcb;
>  
>  	/* The data must NOT be null. This means rethook data structure is broken. */
> -	if (WARN_ON_ONCE(!data))
> +	if (WARN_ON_ONCE(!data) || !rp->handler)
>  		return;
>  
>  	__this_cpu_write(current_kprobe, &rp->kp);
> 
> -- 
> pi3 (pi3ki31ny) - pi3 (at) itsec pl
> http://pi3.com.pl
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

On Fri, 22 Apr 2022 22:05:23 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi Steve,
> 
> Can you pick this fix to urgent branch? Or Should I ask bpf people?

It looks to fix a bug from a commit pulled in from the BPF folks. I would
have them take it.

Thanks,

-- Steve
> 
> > Fixes: 73f9b911faa7 ("kprobes: Use rethook for kretprobe if possible")

On Fri, 22 Apr 2022 09:43:50 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 22 Apr 2022 22:05:23 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Hi Steve,
> > 
> > Can you pick this fix to urgent branch? Or Should I ask bpf people?
> 
> It looks to fix a bug from a commit pulled in from the BPF folks. I would
> have them take it.

OK, thanks!

Adam, can you resend your patch with my Ack and modify the PATCH tag as
"[PATCH bpf]", to "Alexei Starovoitov <ast@kernel.org>", "Andrii Nakryiko <andrii@kernel.org>",
 and Cc: "bpf@vger.kernel.org" ? 

Thank you,

> 
> Thanks,
> 
> -- Steve
> > 
> > > Fixes: 73f9b911faa7 ("kprobes: Use rethook for kretprobe if possible")


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Sure, I will do it.

Thanks for help,
Adam

On Fri, Apr 22, 2022 at 11:54:01PM +0900, Masami Hiramatsu wrote:
> On Fri, 22 Apr 2022 09:43:50 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Fri, 22 Apr 2022 22:05:23 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > 
> > > Hi Steve,
> > > 
> > > Can you pick this fix to urgent branch? Or Should I ask bpf people?
> > 
> > It looks to fix a bug from a commit pulled in from the BPF folks. I would
> > have them take it.
> 
> OK, thanks!
> 
> Adam, can you resend your patch with my Ack and modify the PATCH tag as
> "[PATCH bpf]", to "Alexei Starovoitov <ast@kernel.org>", "Andrii Nakryiko <andrii@kernel.org>",
>  and Cc: "bpf@vger.kernel.org" ? 
> 
> Thank you,
> 
> > 
> > Thanks,
> > 
> > -- Steve
> > > 
> > > > Fixes: 73f9b911faa7 ("kprobes: Use rethook for kretprobe if possible")
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

-- 
pi3 (pi3ki31ny) - pi3 (at) itsec pl
http://pi3.com.pl

On Fri, 15 Apr 2022 20:10:06 +0200
Adam Zabrocki <pi3@pi3.com.pl> wrote:

> [PATCH] x86/kprobes: Fix KRETPROBES when CONFIG_KRETPROBE_ON_RETHOOK is set
> 
> The recent kernel change "kprobes: Use rethook for kretprobe if possible",
> introduced a potential NULL pointer dereference bug in the KRETPROBE
> mechanism. The official Kprobes documentation defines that "Any or all
> handlers can be NULL". Unfortunately, there is a missing return handler
> verification to fulfill these requirements and can result in a NULL pointer
> dereference bug.
> 
> This patch adds such verification in kretprobe_rethook_handler() function.
> 

Good catch! I forgot that user can register kretprobe without handler...

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you!

> Fixes: 73f9b911faa7 ("kprobes: Use rethook for kretprobe if possible")
> Signed-off-by: Adam Zabrocki <pi3@pi3.com.pl>
> ---
>  kernel/kprobes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index dbe57df2e199..dd58c0be9ce2 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2126,7 +2126,7 @@ static void kretprobe_rethook_handler(struct rethook_node *rh, void *data,
>  	struct kprobe_ctlblk *kcb;
>  
>  	/* The data must NOT be null. This means rethook data structure is broken. */
> -	if (WARN_ON_ONCE(!data))
> +	if (WARN_ON_ONCE(!data) || !rp->handler)
>  		return;
>  
>  	__this_cpu_write(current_kprobe, &rp->kp);
> 
> -- 
> pi3 (pi3ki31ny) - pi3 (at) itsec pl
> http://pi3.com.pl
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>