kprobes: Fix NULL pointer dereference in unregister_kretprobes()

[PATCH] kprobes: Fix NULL pointer dereference in unregister_kretprobes()

Posted by Luigi Rizzo 1 month, 3 weeks ago

In unregister_kretprobes(), rps[i]->rph can be NULL e.g. when called
after kretprobe failed registration. Under !CONFIG_KRETPROBE_ON_RETHOOK,
the unconditional access to rps[i]->rph->rp, causes a kernel panic due
to NULL pointer dereference.

Add a NULL check for rps[i]->rph before invoking rcu_assign_pointer().

Fixes: d839a656d0f3 ("kprobes: consistent rcu api usage for kretprobe holder")
Signed-off-by: Luigi Rizzo <lrizzo@google.com>
---
 kernel/kprobes.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index bfc89083daa93..5dd4786c455de 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2359,7 +2359,8 @@ void unregister_kretprobes(struct kretprobe **rps, int num)
 #ifdef CONFIG_KRETPROBE_ON_RETHOOK
 		rethook_free(rps[i]->rh);
 #else
-		rcu_assign_pointer(rps[i]->rph->rp, NULL);
+		if (rps[i]->rph)
+			rcu_assign_pointer(rps[i]->rph->rp, NULL);
 #endif
 	}
 
-- 
2.48.1.500.g5897711438-goog

Re: [PATCH] kprobes: Fix NULL pointer dereference in unregister_kretprobes()

Posted by Masami Hiramatsu (Google) 1 month, 3 weeks ago

On Wed,  5 Aug 2026 16:12:21 +0000
Luigi Rizzo <lrizzo@google.com> wrote:

> In unregister_kretprobes(), rps[i]->rph can be NULL e.g. when called
> after kretprobe failed registration. Under !CONFIG_KRETPROBE_ON_RETHOOK,
> the unconditional access to rps[i]->rph->rp, causes a kernel panic due
> to NULL pointer dereference.

This is not a bug, since if register_kretprobe(rp) fails, rp must NOT be
passed to unregister_kretprobe(rp). Or, do you find any cases where
register_kretprobe() fails, preventing proper cleanup, and requiring
unregister_kretprobe()? If so, we have to fix that case.

> 
> Add a NULL check for rps[i]->rph before invoking rcu_assign_pointer().
> 

But this could be a kind of protective improvemet for someone
misunderstand that.

Thank you,

> Fixes: d839a656d0f3 ("kprobes: consistent rcu api usage for kretprobe holder")
> Signed-off-by: Luigi Rizzo <lrizzo@google.com>
> ---
>  kernel/kprobes.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index bfc89083daa93..5dd4786c455de 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2359,7 +2359,8 @@ void unregister_kretprobes(struct kretprobe **rps, int num)
>  #ifdef CONFIG_KRETPROBE_ON_RETHOOK
>  		rethook_free(rps[i]->rh);
>  #else
> -		rcu_assign_pointer(rps[i]->rph->rp, NULL);
> +		if (rps[i]->rph)
> +			rcu_assign_pointer(rps[i]->rph->rp, NULL);
>  #endif
>  	}
>  
> -- 
> 2.48.1.500.g5897711438-goog
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH] kprobes: Fix NULL pointer dereference in unregister_kretprobes()

Posted by Luigi Rizzo 1 month, 3 weeks ago

On Thu, Aug 6, 2026 at 2:02 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Wed,  5 Aug 2026 16:12:21 +0000
> Luigi Rizzo <lrizzo@google.com> wrote:
>
> > In unregister_kretprobes(), rps[i]->rph can be NULL e.g. when called
> > after kretprobe failed registration. Under !CONFIG_KRETPROBE_ON_RETHOOK,
> > the unconditional access to rps[i]->rph->rp, causes a kernel panic due
> > to NULL pointer dereference.
>
> This is not a bug, since if register_kretprobe(rp) fails, rp must NOT be
> passed to unregister_kretprobe(rp). Or, do you find any cases where
> register_kretprobe() fails, preventing proper cleanup, and requiring
> unregister_kretprobe()? If so, we have to fix that case.

Masami, you are right, the kernel tree does not call unregister_kretprobes()
on a failed registration. I was confused by the unregister_kretprobes(rps, i);
call in the cleanup in register_kretprobes(), but the failed entry i is
not unregistered).

So aside from protective coding (but where would one stop ?
null rps, null rps[i], ... ), there is no need for this patch.

thanks for the feedback
Luigi

Re: [PATCH] kprobes: Fix NULL pointer dereference in unregister_kretprobes()

Posted by Masami Hiramatsu (Google) 1 month, 3 weeks ago

On Thu, 6 Aug 2026 09:23:29 +0200
Luigi Rizzo <lrizzo@google.com> wrote:

> On Thu, Aug 6, 2026 at 2:02 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > On Wed,  5 Aug 2026 16:12:21 +0000
> > Luigi Rizzo <lrizzo@google.com> wrote:
> >
> > > In unregister_kretprobes(), rps[i]->rph can be NULL e.g. when called
> > > after kretprobe failed registration. Under !CONFIG_KRETPROBE_ON_RETHOOK,
> > > the unconditional access to rps[i]->rph->rp, causes a kernel panic due
> > > to NULL pointer dereference.
> >
> > This is not a bug, since if register_kretprobe(rp) fails, rp must NOT be
> > passed to unregister_kretprobe(rp). Or, do you find any cases where
> > register_kretprobe() fails, preventing proper cleanup, and requiring
> > unregister_kretprobe()? If so, we have to fix that case.
> 
> Masami, you are right, the kernel tree does not call unregister_kretprobes()
> on a failed registration. I was confused by the unregister_kretprobes(rps, i);
> call in the cleanup in register_kretprobes(), but the failed entry i is
> not unregistered).

Yes, in that case rps[i] is not unregistered ;)

> 
> So aside from protective coding (but where would one stop ?
> null rps, null rps[i], ... ), there is no need for this patch.

OK, Thanks for the confirmation!

Thanks,


> 
> thanks for the feedback
> Luigi


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH] kprobes: Fix NULL pointer dereference in unregister_kretprobes()

Posted by JP Kobryn 1 month, 3 weeks ago

On 8/5/2026 9:12 AM, Luigi Rizzo wrote:
> In unregister_kretprobes(), rps[i]->rph can be NULL e.g. when called
> after kretprobe failed registration. Under !CONFIG_KRETPROBE_ON_RETHOOK,
> the unconditional access to rps[i]->rph->rp, causes a kernel panic due
> to NULL pointer dereference.
> 
> Add a NULL check for rps[i]->rph before invoking rcu_assign_pointer().
> 
> Fixes: d839a656d0f3 ("kprobes: consistent rcu api usage for kretprobe holder")

The bug was not introduced in this commit. It goes further back to:

d741bf41d7c7 ("kprobes: Remove kretprobe hash")

> Signed-off-by: Luigi Rizzo <lrizzo@google.com>
> ---
>   kernel/kprobes.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index bfc89083daa93..5dd4786c455de 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2359,7 +2359,8 @@ void unregister_kretprobes(struct kretprobe **rps, int num)
>   #ifdef CONFIG_KRETPROBE_ON_RETHOOK
>   		rethook_free(rps[i]->rh);
>   #else
> -		rcu_assign_pointer(rps[i]->rph->rp, NULL);
> +		if (rps[i]->rph)
> +			rcu_assign_pointer(rps[i]->rph->rp, NULL);
>   #endif
>   	}
>

Re: [PATCH] kprobes: Fix NULL pointer dereference in unregister_kretprobes()

Posted by Bradley Morgan 1 month, 3 weeks ago

Thanks for the patch, good catch.

 
Reviewed-by: Bradley Morgan <include@grrlz.net>


Thanks!