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

[Petr Pavlu <petr.pavlu@suse.com>]

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@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. 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.

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

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.

Fixes: d741bf41d7c7 ("kprobes: Remove kretprobe hash")
Signed-off-by: Petr Pavlu <petr.pavlu@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