Re: [PATCH pre-6.7] kprobes: Fix double free of kretprobe_holder

[Google]

On Wed, 17 Jul 2024 14:52:43 +0200
Petr Pavlu <petr.pavlu@xxxxxxxx> wrote:

> Hello,
> 
> Below is a patch for a kretprobe-related problem that was already fixed
> in v6.7 as a side-effect of the objpool optimization, in commit
> 4bbd93455659 ("kprobes: kretprobe scalability improvement").
> 
> I'm sending it to the list because it might be useful to pick the fix up
> for longterm or distribution kernels. Additionally, I would like to
> propose a small improvement to refcount_t and this gives me an actual
> problem to point to about its motivation.
> 
> Cheers,
> Petr
> 
> ---
> 
> From b0dde62cc5268a7d728cfdb360cb5170266a5e11 Mon Sep 17 00:00:00 2001
> From: Petr Pavlu <petr.pavlu@xxxxxxxx>
> Date: Thu, 4 Apr 2024 16:44:02 +0200
> Subject: [PATCH pre-6.7] kprobes: Fix double free of kretprobe_holder
> 
> When unregistering a kretprobe, the code in unregister_kretprobes() sets
> rp->rph->rp to NULL which forces all associated kretprobe_instances
> still in use to be later freed separately via free_rp_inst_rcu().
> 
> Function unregister_kretprobes() then calls free_rp_inst() which takes
> care of releasing all currently unused kretprobe_instances, the ones
> that are on the kretprobe's freelist. The code in free_rp_inst() counts
> a number of these released kretprobe_instances and invokes
> refcount_sub_and_test(count, &rp->rph->ref) to decrease the
> kretprobe_holder's refcount and subsequently calls kfree(rp->rph) if the
> function returns true, indicating the refcount reached zero.
> 
> It is possible that the number of released kretprobe_instances in
> free_rp_inst() is zero and therefore refcount_sub_and_test() is invoked
> with count=0.

Ah, good catch! Calling unregsiter_kretprobe() when all instances are
used, this happens. To avoid this, usually refcount starts from 1 as
initial reference, but it didn't.

> Additionally, depending on timing, it can happen
> that all previously used kretprobe_instances were already freed via
> free_rp_inst_rcu(). This means the refcount of kretprobe_holder already
> reached zero and was deallocated.
> 
> The resulting call of refcount_sub_and_test(0, &rp->rph->ref) in
> free_rp_inst() is then a use-after-free. If the memory previously
> occupied by the refcount is still set to zero then the call returns true
> and kretprobe_holder gets wrongly freed for the second time.

Right.

> 
> Fix the problem by adding a check for count>0 before calling
> refcount_sub_and_test() in free_rp_inst().

OK, this can avoid use-after-free.

> 
> Note that this code was reworked in v6.7 by commit 4bbd93455659
> ("kprobes: kretprobe scalability improvement") and the new objpool
> implementation doesn't have this problem.
> 

Looks good to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>

This should go directly into stable because there is no applicable code
in the latest kernel.


> Fixes: d741bf41d7c7 ("kprobes: Remove kretprobe hash")
> Signed-off-by: Petr Pavlu <petr.pavlu@xxxxxxxx>
> ---
>  kernel/kprobes.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 0c6185aefaef..7ae5873545a1 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1942,10 +1942,9 @@ static inline void free_rp_inst(struct kretprobe *rp)
>  		count++;
>  	}
>  
> -	if (refcount_sub_and_test(count, &rp->rph->ref)) {
> +	if (count > 0 && refcount_sub_and_test(count, &rp->rph->ref))
>  		kfree(rp->rph);
> -		rp->rph = NULL;
> -	}
> +	rp->rph = NULL;
>  }
>  
>  /* This assumes the 'tsk' is the current task or the is not running. */
> 
> base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa
> -- 
> 2.35.3
> 


-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>