On Wed, 17 Jul 2024 14:52:43 +0200
Petr Pavlu <petr.pa...@suse.com> 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.pa...@suse.com>
> 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) <mhira...@kernel.org>
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.pa...@suse.com>
> ---
> 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) <mhira...@kernel.org>
