[PATCH bpf-next 2/2] bpf: Fix UAF in bpf_trampoline_multi_detach when ftrace update fails

[Hui Zhu]

From: Hui Zhu <zhuhui@xxxxxxxxxx>

After the per-mnode unlink loop, the return values of the batch
update_ftrace_direct_del()/update_ftrace_direct_mod() calls are only
WARN_ON_ONCE()'d, not checked. bpf_trampoline_multi_attach_free() is
then called unconditionally for every mnode, freeing old_image
whenever the single-point unlink succeeded (old_image != cur_image).

If the batch update fails, ftrace still points to old_image for the
affected IPs, so freeing it is a UAF.

Capture the two return values and, for mnodes whose single-point
unlink succeeded but whose batch update failed, call
bpf_trampoline_multi_attach_rollback() instead of
bpf_trampoline_multi_attach_free(). cur_image == NULL identifies the
unreg path (set by unregister_fentry_multi when total == 0),
cur_image != NULL identifies the modify path (set by
modify_fentry_multi when total > 0), so err_unreg/err_mod can be
matched to the right mnodes. Rollback restores cur_image = old_image,
the image ftrace is still actually calling.

This mirrors the existing error handling in
bpf_trampoline_multi_attach()'s rollback_unlink path.

Fixes: aef4dfa790b2 ("bpf: Add bpf_trampoline_multi_attach/detach functions")
Signed-off-by: Hui Zhu <zhuhui@xxxxxxxxxx>
---
 kernel/bpf/trampoline.c | 42 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index a78fbf726fad..44ef25beb4cc 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -1723,7 +1723,7 @@ int bpf_trampoline_multi_detach(struct bpf_prog *prog, struct bpf_tracing_multi_
 {
 	struct bpf_tracing_multi_data *data = &link->data;
 	struct bpf_tracing_multi_node *mnode;
-	int i;
+	int i, err_unreg = 0, err_mod = 0;
 
 	trampoline_lock_all();
 
@@ -1734,13 +1734,41 @@ int bpf_trampoline_multi_detach(struct bpf_prog *prog, struct bpf_tracing_multi_
 					NULL, &trampoline_multi_ops, data));
 	}
 
-	if (ftrace_hash_count(data->unreg))
-		WARN_ON_ONCE(update_ftrace_direct_del(&direct_ops, data->unreg));
-	if (ftrace_hash_count(data->modify))
-		WARN_ON_ONCE(update_ftrace_direct_mod(&direct_ops, data->modify, true));
+	if (ftrace_hash_count(data->unreg)) {
+		err_unreg = update_ftrace_direct_del(&direct_ops, data->unreg);
+		WARN_ON_ONCE(err_unreg);
+	}
+	if (ftrace_hash_count(data->modify)) {
+		err_mod = update_ftrace_direct_mod(&direct_ops, data->modify, true);
+		WARN_ON_ONCE(err_mod);
+	}
 
-	for_each_mnode(mnode, link)
-		bpf_trampoline_multi_attach_free(mnode->trampoline);
+	for_each_mnode(mnode, link) {
+		struct bpf_trampoline *tr = mnode->trampoline;
+
+		/* If the batch ftrace update failed for this mnode's path,
+		 * ftrace still points to old_image. Use rollback to restore
+		 * cur_image to old_image (putting the new cur_image if any)
+		 * so the trampoline keeps the image ftrace is calling.
+		 *
+		 * This relies on update_ftrace_direct_del/mod being atomic:
+		 * on failure, NO IPs in the hash are modified in ftrace (all
+		 * validation/allocation happens before any ftrace record is
+		 * touched). If this assumption is broken in the future (i.e.,
+		 * partial success becomes possible), this rollback logic would
+		 * need to be revisited.
+		 *
+		 * cur_image == NULL indicates the unreg path (total == 0);
+		 * cur_image != NULL indicates the modify path (total > 0).
+		 */
+		if (tr->multi_attach.old_image &&
+		    tr->multi_attach.old_image != tr->cur_image &&
+		    ((err_unreg && !tr->cur_image) ||
+		     (err_mod && tr->cur_image)))
+			bpf_trampoline_multi_attach_rollback(tr);
+		else
+			bpf_trampoline_multi_attach_free(tr);
+	}
 
 	trampoline_unlock_all();
 
-- 
2.43.0