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

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

[Adam Zabrocki]

[PATCH bpf] 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>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 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 bpf] x86/kprobes: Fix KRETPROBES when CONFIG_KRETPROBE_ON_RETHOOK is set

[Daniel Borkmann]

On 4/22/22 6:40 PM, Adam Zabrocki wrote:
> [PATCH bpf] 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>
> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

I don't mind if this fix gets routed via bpf tree if all parties are okay with
it (Masami? Steven?). Just noting that there is currently no specific dependency
in bpf tree for it, but if it's easier to route this way, happy to take it.

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

Thanks,
Daniel

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

[Masami Hiramatsu]

On Mon, 25 Apr 2022 16:42:12 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:

> On 4/22/22 6:40 PM, Adam Zabrocki wrote:
> > [PATCH bpf] 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>
> > Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
> 
> I don't mind if this fix gets routed via bpf tree if all parties are okay with
> it (Masami? Steven?). Just noting that there is currently no specific dependency
> in bpf tree for it, but if it's easier to route this way, happy to take it.

Yeah, I and Steve talked about it and he suggested this to be merged
via BPF tree since the original commit came from the BPF tree.

Thank you,

> 
> > ---
> >   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);
> > 
> 
> Thanks,
> Daniel


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[Daniel Borkmann]

On 4/26/22 10:50 AM, Masami Hiramatsu wrote:
> On Mon, 25 Apr 2022 16:42:12 +0200
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 4/22/22 6:40 PM, Adam Zabrocki wrote:
>>> [PATCH bpf] 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>
>>> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
>>
>> I don't mind if this fix gets routed via bpf tree if all parties are okay with
>> it (Masami? Steven?). Just noting that there is currently no specific dependency
>> in bpf tree for it, but if it's easier to route this way, happy to take it.
> 
> Yeah, I and Steve talked about it and he suggested this to be merged
> via BPF tree since the original commit came from the BPF tree.

Okay, I just applied it to bpf tree then.

Thanks everyone,
Daniel