From: Pu Lehui In bpf_netns_link_update_prog, the checks for old_prog and prog type are currently performed locklessly before acquiring netns_bpf_mutex. This creates a race condition that can lead to a UAF issue. If two threads concurrently execute BPF_LINK_UPDATE on the same netns link, the following execution path can trigger a UAF: CPU0 CPU1 bpf_netns_link_update_prog if (old_prog && old_prog != link->prog) return -EPERM; bpf_netns_link_update_prog if (old_prog && old_prog != link->prog) ... old_prog = xchg(&link->prog, new_prog); bpf_prog_put(old_prog); if (new_prog->type != link->prog->type) <-- trigger UAF Fix this by moving the old_prog and prog->type checks inside the netns_bpf_mutex critical section. Fixes: 7f045a49fee0 ("bpf: Add link-based BPF program attachment to network namespace") Reported-by: Sashiko Reviewed-by: Amery Hung Signed-off-by: Pu Lehui --- kernel/bpf/net_namespace.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/kernel/bpf/net_namespace.c b/kernel/bpf/net_namespace.c index 25f30f9edaef..9fc62db1441c 100644 --- a/kernel/bpf/net_namespace.c +++ b/kernel/bpf/net_namespace.c @@ -171,13 +171,17 @@ static int bpf_netns_link_update_prog(struct bpf_link *link, struct net *net; int idx, ret; - if (old_prog && old_prog != link->prog) - return -EPERM; - if (new_prog->type != link->prog->type) - return -EINVAL; - mutex_lock(&netns_bpf_mutex); + if (old_prog && old_prog != link->prog) { + ret = -EPERM; + goto out_unlock; + } + if (new_prog->type != link->prog->type) { + ret = -EINVAL; + goto out_unlock; + } + net = net_link->net; if (!net || !check_net(net)) { /* Link auto-detached or netns dying */ -- 2.34.1 From: Pu Lehui In bpf_mprog_link, the code does not check the link->type first before dereferencing link->prog->type. This missing validation allows a user to pass an abnormal non-netkit or non-tcx link via relative_fd. If doing BPF_LINK_UPDATE on the abnormal link, it can trigger a UAF issue. CPU0 CPU1 netkit_link_prog_attach bpf_mprog_attach bpf_mprog_tuple_relative bpf_mprog_link link = bpf_link_get_from_fd(id_or_fd); BPF_LINK_UPDATE ... old_prog = xchg(&link->link.prog, new_prog); bpf_prog_put(old_prog); if (type && link->prog->type != type) <-- trigger UAF The reason for the UAF is that each subsystem provides its own protection for link->prog. Since there is no cross subsystem protection (if not considering the RCU of prog tear down), dereferencing the prog of an anchor link that does not belong to the current subsystem is not safe: it may have been freed. Therefore, we need to validate link->type to reject foreign anchors. Fix this by strictly validating link->type in bpf_mprog_link against the expected link type. bpf_mprog_tuple_relative is also adjusted to accept and pass down the expected link type. Fixes: 053c8e1f235d ("bpf: Add generic attach/detach/query API for multi-progs") Reported-by: Sashiko Reviewed-by: Amery Hung Signed-off-by: Pu Lehui --- kernel/bpf/mprog.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/kernel/bpf/mprog.c b/kernel/bpf/mprog.c index 1394168062e8..37ce91e0bcd2 100644 --- a/kernel/bpf/mprog.c +++ b/kernel/bpf/mprog.c @@ -6,7 +6,7 @@ static int bpf_mprog_link(struct bpf_tuple *tuple, u32 id_or_fd, u32 flags, - enum bpf_prog_type type) + enum bpf_link_type type) { struct bpf_link *link = ERR_PTR(-EINVAL); bool id = flags & BPF_F_ID; @@ -17,7 +17,7 @@ static int bpf_mprog_link(struct bpf_tuple *tuple, link = bpf_link_get_from_fd(id_or_fd); if (IS_ERR(link)) return PTR_ERR(link); - if (type && link->prog->type != type) { + if (type && link->type != type) { bpf_link_put(link); return -EINVAL; } @@ -52,21 +52,22 @@ static int bpf_mprog_prog(struct bpf_tuple *tuple, static int bpf_mprog_tuple_relative(struct bpf_tuple *tuple, u32 id_or_fd, u32 flags, - enum bpf_prog_type type) + enum bpf_link_type ltype, + enum bpf_prog_type ptype) { bool link = flags & BPF_F_LINK; bool id = flags & BPF_F_ID; memset(tuple, 0, sizeof(*tuple)); if (link) - return bpf_mprog_link(tuple, id_or_fd, flags, type); + return bpf_mprog_link(tuple, id_or_fd, flags, ltype); /* If no relevant flag is set and no id_or_fd was passed, then * tuple link/prog is just NULLed. This is the case when before/ * after selects first/last position without passing fd. */ if (!id && !id_or_fd) return 0; - return bpf_mprog_prog(tuple, id_or_fd, flags, type); + return bpf_mprog_prog(tuple, id_or_fd, flags, ptype); } static void bpf_mprog_tuple_put(struct bpf_tuple *tuple) @@ -243,6 +244,7 @@ int bpf_mprog_attach(struct bpf_mprog_entry *entry, return -EEXIST; ret = bpf_mprog_tuple_relative(&rtuple, id_or_fd, flags & ~BPF_F_REPLACE, + link ? link->type : BPF_LINK_TYPE_UNSPEC, prog_new->type); if (ret) return ret; @@ -343,6 +345,7 @@ int bpf_mprog_detach(struct bpf_mprog_entry *entry, if (!bpf_mprog_total(entry)) return -ENOENT; ret = bpf_mprog_tuple_relative(&rtuple, id_or_fd, flags, + link ? link->type : BPF_LINK_TYPE_UNSPEC, prog ? prog->type : BPF_PROG_TYPE_UNSPEC); if (ret) -- 2.34.1 From: Pu Lehui In bpf_link_show_fdinfo and bpf_link_get_info_by_fd, link->prog is accessed without holding any locks. If the prog is concurrently replaced via bpf_link_update, the old prog can be freed, leading to a potential UAF issue. Before dereferencing the prog, both normal RCU and RCU Tasks Trace read locks would normally be required, as BPF_LINK_TYPE_ITER supports both non-sleepable and sleepable progs. However, as commit 57b23c0f612d ("bpf: Retire rcu_trace_implies_rcu_gp()") clarifies, an RCU Tasks Trace grace period implies an RCU grace period, so holding only rcu_read_lock() is already sufficient. Fixes: 0c991ebc8c69 ("bpf: Implement bpf_prog replacement for an active bpf_cgroup_link") Reported-by: Sashiko Signed-off-by: Pu Lehui --- kernel/bpf/syscall.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 6db306d23b47..cad986807d53 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -3471,9 +3471,10 @@ static const char *bpf_link_type_strs[] = { static void bpf_link_show_fdinfo(struct seq_file *m, struct file *filp) { const struct bpf_link *link = filp->private_data; - const struct bpf_prog *prog = link->prog; + const struct bpf_prog *prog; enum bpf_link_type type = link->type; char prog_tag[sizeof(prog->tag) * 2 + 1] = { }; + u32 prog_id; if (type < ARRAY_SIZE(bpf_link_type_strs) && bpf_link_type_strs[type]) { if (link->type == BPF_LINK_TYPE_KPROBE_MULTI) @@ -3490,13 +3491,20 @@ static void bpf_link_show_fdinfo(struct seq_file *m, struct file *filp) } seq_printf(m, "link_id:\t%u\n", link->id); + rcu_read_lock(); + prog = READ_ONCE(link->prog); if (prog) { bin2hex(prog_tag, prog->tag, sizeof(prog->tag)); + prog_id = prog->aux->id; + } + rcu_read_unlock(); + + if (prog) { seq_printf(m, "prog_tag:\t%s\n" "prog_id:\t%u\n", prog_tag, - prog->aux->id); + prog_id); } if (link->ops->show_fdinfo) link->ops->show_fdinfo(link, m); @@ -5535,6 +5543,7 @@ static int bpf_link_get_info_by_fd(struct file *file, { struct bpf_link_info __user *uinfo = u64_to_user_ptr(attr->info.info); struct bpf_link_info info; + const struct bpf_prog *prog; u32 info_len = attr->info.info_len; int err; @@ -5549,8 +5558,12 @@ static int bpf_link_get_info_by_fd(struct file *file, info.type = link->type; info.id = link->id; - if (link->prog) - info.prog_id = link->prog->aux->id; + + rcu_read_lock(); + prog = READ_ONCE(link->prog); + if (prog) + info.prog_id = prog->aux->id; + rcu_read_unlock(); if (link->ops->fill_link_info) { err = link->ops->fill_link_info(link, &info); -- 2.34.1 From: Pu Lehui Syzkaller reported a storage null-ptr-deref issue after replacing prog. This occurs in the following scenario: 1. prog A, an empty prog, is attached to a cgrp. 2. prog B uses BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE and calls the bpf_get_local_storage helper. 3. link_update is called to replace prog A with prog B. The reason is that __cgroup_bpf_replace fails to alloc and assign the required cgrp storage for the incoming replacement prog. Consequently, the new prog inherits an uninit storage, leading to null-ptr-deref panic when kick the new prog. Fix this by properly allocating the storage and comparing the old and new storage pointers. If the storage changed, fallback to update_effective_progs which performs a RCU-safe update of the entire array. If the storage remains unchanged, we can safely retain the fast-path in-place update. Additionally, handle the error path in __cgroup_bpf_attach strictly. Although it is rare for update_effective_progs to fail in this context, proper rollbacks for storage and flags are added for code rigor. Fixes: 0c991ebc8c69 ("bpf: Implement bpf_prog replacement for an active bpf_cgroup_link") Reviewed-by: Amery Hung Signed-off-by: Pu Lehui --- kernel/bpf/cgroup.c | 44 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c index 4355ccb78a9c..56d538f05520 100644 --- a/kernel/bpf/cgroup.c +++ b/kernel/bpf/cgroup.c @@ -813,10 +813,12 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *old_prog = NULL; struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = {}; struct bpf_cgroup_storage *new_storage[MAX_BPF_CGROUP_STORAGE_TYPE] = {}; + struct bpf_cgroup_storage *old_storage[MAX_BPF_CGROUP_STORAGE_TYPE] = {}; struct bpf_prog *new_prog = prog ? : link->link.prog; enum cgroup_bpf_attach_type atype; struct bpf_prog_list *pl; struct hlist_head *progs; + u8 old_flags; int err; if (((flags & BPF_F_ALLOW_OVERRIDE) && (flags & BPF_F_ALLOW_MULTI)) || @@ -883,7 +885,10 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp, pl->prog = prog; pl->link = link; pl->flags = flags; + if (old_prog) + bpf_cgroup_storages_assign(old_storage, pl->storage); bpf_cgroup_storages_assign(pl->storage, storage); + old_flags = cgrp->bpf.flags[atype]; cgrp->bpf.flags[atype] = saved_flags; if (type == BPF_LSM_CGROUP) { @@ -915,12 +920,14 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp, if (old_prog) { pl->prog = old_prog; pl->link = NULL; + bpf_cgroup_storages_assign(pl->storage, old_storage); } bpf_cgroup_storages_free(new_storage); if (!old_prog) { hlist_del(&pl->node); kfree(pl); } + cgrp->bpf.flags[atype] = old_flags; return err; } @@ -1032,11 +1039,17 @@ static int __cgroup_bpf_replace(struct cgroup *cgrp, struct bpf_cgroup_link *link, struct bpf_prog *new_prog) { + struct bpf_cgroup_storage *new_storage[MAX_BPF_CGROUP_STORAGE_TYPE] = {}; + struct bpf_cgroup_storage *old_storage[MAX_BPF_CGROUP_STORAGE_TYPE] = {}; + struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = {}; + enum bpf_cgroup_storage_type stype; enum cgroup_bpf_attach_type atype; + bool storage_changed = false; struct bpf_prog *old_prog; struct bpf_prog_list *pl; struct hlist_head *progs; bool found = false; + int err; atype = bpf_cgroup_atype_find(link->link.attach_type, new_prog->aux->attach_btf_id); if (atype < 0) @@ -1056,10 +1069,39 @@ static int __cgroup_bpf_replace(struct cgroup *cgrp, if (!found) return -ENOENT; + if (bpf_cgroup_storages_alloc(storage, new_storage, link->link.attach_type, + new_prog, cgrp)) + return -ENOMEM; + + for_each_cgroup_storage_type(stype) { + if (storage[stype] != pl->storage[stype]) { + storage_changed = true; + break; + } + } + cgrp->bpf.revisions[atype] += 1; old_prog = xchg(&link->link.prog, new_prog); - replace_effective_prog(cgrp, atype, pl); + + if (!storage_changed) { + replace_effective_prog(cgrp, atype, pl); + bpf_prog_put(old_prog); + return 0; + } + + bpf_cgroup_storages_assign(old_storage, pl->storage); + bpf_cgroup_storages_assign(pl->storage, storage); + err = update_effective_progs(cgrp, atype); + if (err) { + xchg(&link->link.prog, old_prog); + bpf_cgroup_storages_assign(pl->storage, old_storage); + bpf_cgroup_storages_free(new_storage); + cgrp->bpf.revisions[atype] -= 1; + return err; + } + bpf_prog_put(old_prog); + bpf_cgroup_storages_link(new_storage, cgrp, link->link.attach_type); return 0; } -- 2.34.1