KRETPROBES are broken since the commit 73f9b911faa7

KRETPROBES are broken since the commit 73f9b911faa7

[Adam Zabrocki]

Hello,

The recent KRETPROBE kernel changes introduced a potential NULL pointer
dereference bug when return handler is not set. The root of the problem
are the following commits related to "kprobes: Use rethook for kretprobe
if possible":
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f3a112c0c40dd96d53c8bdf3ea8d94d528f3b7b8
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=73f9b911faa74ac5107879de05c9489c419f41bb


When user sets up a KRETPROBE, kernel eventually ends up here:

static void kretprobe_rethook_handler(struct rethook_node *rh, void *data,
                      struct pt_regs *regs)
{
    struct kretprobe *rp = (struct kretprobe *)data;
    struct kretprobe_instance *ri;
    struct kprobe_ctlblk *kcb;

    /* The data must NOT be null. This means rethook data structure is broken. */
    if (WARN_ON_ONCE(!data))
        return;

    __this_cpu_write(current_kprobe, &rp->kp);
    kcb = get_kprobe_ctlblk();
    kcb->kprobe_status = KPROBE_HIT_ACTIVE;

    ri = container_of(rh, struct kretprobe_instance, node);
    rp->handler(ri, regs);

    __this_cpu_write(current_kprobe, NULL);
}
NOKPROBE_SYMBOL(kretprobe_rethook_handler);

Unfortunately, rp->handler is not verified against NULL. If user does not
set up a return handler, KRETPROBE shouldn't call it (that's the defined
behavior in the documentation, quoting "Any or all handlers can be NULL").
However, kretprobe_rethook_handler function does not enforce such behavior.

I'm going to send a patch for it in a reply to this message.

Thanks,
Adam

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

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

[Adam Zabrocki]

[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

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

[Masami Hiramatsu]

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>

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

[Masami Hiramatsu]

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>

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

[Steven Rostedt]

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")

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

[Masami Hiramatsu]

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>

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

[Adam Zabrocki]

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