From: Hui Zhu <[email protected]>
When bpf_trampoline_update() fails before modify_fentry_multi()/
unregister_fentry_multi() is called, cur_image is unchanged
(cur_image == old_image) and ftrace still calls into it. Freeing
old_image in that case causes a UAF.
Only free old_image when it differs from cur_image.
Also change bpf_trampoline_multi_detach() to return void and drop
the WARN_ON_ONCE at its call site.
Fixes: aef4dfa790b2 ("bpf: Add bpf_trampoline_multi_attach/detach functions")
Signed-off-by: Hui Zhu <[email protected]>
---
Changelog:
v4:
According to the comments of bot+bpf-ci, add Fixes: and update comments
of bpf_trampoline_multi_attach_free.
v3:
According to the comments of Jiri Olsa, drop patches 2/3 and the
prog-side machinery.
keep only the simplified image-side fix in
bpf_trampoline_multi_attach_free() and make
bpf_trampoline_multi_detach() return void.
v2:
Folded v1's two detach patches into patch 1.
According to the comments of Jiri Olsa, Pin the prog (pinned_prog) on
cur_image so it stays alive while ftrace may still call into it.
Make bpf_trampoline_multi_detach() return void.
Fix the same UAF in standard (non-multi) trampolines.
According to the comments of sashiko, Fix the prog UAF in
bpf_trampoline_multi_attach() rollback.
Leak the trampoline in bpf_trampoline_put() when cur_image is left
by a rollback.
include/linux/bpf.h | 9 ++++-----
kernel/bpf/trampoline.c | 16 +++++++++++++---
kernel/trace/bpf_trace.c | 2 +-
3 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 73bacfc6444d..3255c6f5ea6b 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1518,8 +1518,8 @@ int arch_prepare_bpf_dispatcher(void *image, void *buf,
s64 *funcs, int num_func
int bpf_trampoline_multi_attach(struct bpf_prog *prog, u32 *ids,
struct bpf_tracing_multi_link *link);
-int bpf_trampoline_multi_detach(struct bpf_prog *prog,
- struct bpf_tracing_multi_link *link);
+void bpf_trampoline_multi_detach(struct bpf_prog *prog,
+ struct bpf_tracing_multi_link *link);
void bpf_trampoline_set_flags(struct bpf_trampoline *tr, u32 flags);
/*
@@ -1639,10 +1639,9 @@ static inline int bpf_trampoline_multi_attach(struct
bpf_prog *prog, u32 *ids,
{
return -ENOTSUPP;
}
-static inline int bpf_trampoline_multi_detach(struct bpf_prog *prog,
- struct bpf_tracing_multi_link
*link)
+static inline void bpf_trampoline_multi_detach(struct bpf_prog *prog,
+ struct bpf_tracing_multi_link
*link)
{
- return -ENOTSUPP;
}
static inline void bpf_trampoline_set_flags(struct bpf_trampoline *tr, u32
flags) {}
#endif
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index ed7999ad6c66..e30425789487 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -1595,7 +1595,17 @@ static void bpf_trampoline_multi_attach_init(struct
bpf_trampoline *tr)
static void bpf_trampoline_multi_attach_free(struct bpf_trampoline *tr)
{
- if (tr->multi_attach.old_image)
+ /*
+ * Only free old_image if it is no longer the active image.
+ * When bpf_trampoline_update() fails before modify_fentry_multi()/
+ * unregister_fentry_multi() is called, cur_image is unchanged
+ * (cur_image == old_image) and ftrace still points to it. Freeing
+ * it would cause a UAF when ftrace calls into the freed memory.
+ * On success, cur_image is either a new image or NULL, so
+ * old_image != cur_image means the image is stale.
+ */
+ if (tr->multi_attach.old_image &&
+ tr->multi_attach.old_image != tr->cur_image)
bpf_tramp_image_put(tr->multi_attach.old_image);
tr->multi_attach.old_image = NULL;
@@ -1719,7 +1729,8 @@ int bpf_trampoline_multi_attach(struct bpf_prog *prog,
u32 *ids,
return err;
}
-int bpf_trampoline_multi_detach(struct bpf_prog *prog, struct
bpf_tracing_multi_link *link)
+void bpf_trampoline_multi_detach(struct bpf_prog *prog,
+ struct bpf_tracing_multi_link *link)
{
struct bpf_tracing_multi_data *data = &link->data;
struct bpf_tracing_multi_node *mnode;
@@ -1749,7 +1760,6 @@ int bpf_trampoline_multi_detach(struct bpf_prog *prog,
struct bpf_tracing_multi_
bpf_trampoline_put(mnode->trampoline);
clear_tracing_multi_data(data);
- return 0;
}
#undef for_each_mnode_cnt
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 891897f8a1b3..29260951aa87 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -3687,7 +3687,7 @@ static void bpf_tracing_multi_link_release(struct
bpf_link *link)
struct bpf_tracing_multi_link *tr_link =
container_of(link, struct bpf_tracing_multi_link, link);
- WARN_ON_ONCE(bpf_trampoline_multi_detach(link->prog, tr_link));
+ bpf_trampoline_multi_detach(link->prog, tr_link);
}
static void bpf_tracing_multi_link_dealloc(struct bpf_link *link)
--
2.53.0
