[PATCH bpf] bpf, lsm: Fix cgroup shim selection for socket hooks

[PATCH bpf] bpf, lsm: Fix cgroup shim selection for socket hooks

[Andrea Parri]

bpf_lsm_find_cgroup_shim() picks the shim that runs a BPF_LSM_CGROUP
program from the first argument of the attached hook. It compares
args[0].type, the BTF id of the argument's pointer type, against
btf_sock_ids[], which holds the struct ids of struct socket and struct
sock. A pointer id is never equal to a struct id, so neither branch is
taken and every cgroup LSM program runs through
__cgroup_bpf_run_lsm_current().

Socket and sock hooks therefore run against task_dfl_cgroup(current)
instead of the socket's cgroup. When a socket is used from a cgroup other
than the one that created it, the caller's cgroup policy is applied instead
of the socket's, and __cgroup_bpf_run_lsm_socket() and
__cgroup_bpf_run_lsm_sock() are dead code.

Resolve the argument's pointee with btf_type_resolve_ptr() and compare that
id against btf_sock_ids[].

This makes __cgroup_bpf_run_lsm_socket() reachable for calls it cannot
handle: ip_cmsg_recv_security() passes a NULL socket to
socket_getpeersec_dgram, and sock_create_lite() calls socket_post_create
before sock->sk is set. Fall back to __cgroup_bpf_run_lsm_current() in
those cases instead of dereferencing sock->sk.

__cgroup_bpf_run_lsm_sock() needs no such fallback. sk_alloc_security and
sk_free_security run while the sk has no cgroup and stay on
bpf_lsm_current_hooks; every other hook whose first argument is a struct
sock is called with a non-NULL sk whose cgroup is already set.

Existing BPF_LSM_CGROUP users will see a behavior change. Socket and sock
hooks now run the programs attached to the socket's cgroup, as described
in commit 69fd337a975c ("bpf: per-cgroup lsm flavor"). Hooks that can run
in softirq, such as socket_sock_rcv_skb, inet_conn_request and
sk_getsecid, no longer depend on which task was interrupted.

The bug was found with the help of an LLM.

Fixes: 69fd337a975c ("bpf: per-cgroup lsm flavor")
Cc: stable@vger.kernel.org
Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
---
This changes behavior for existing BPF_LSM_CGROUP users: since
69fd337a975c, socket and sock hooks have run the programs of the current
task's cgroup, and with this patch they run those of the socket's cgroup,
as that commit intended. I'm sending it to bpf with Cc: stable because a
socket used from another cgroup is currently checked against that cgroup's
policy instead of its own. Happy to retarget bpf-next and drop the stable
Cc if you prefer.

 kernel/bpf/bpf_lsm.c | 8 ++++++--
 kernel/bpf/cgroup.c  | 7 ++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index f58dce888ff4a..9547d4f351103 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -95,6 +95,7 @@ void bpf_lsm_find_cgroup_shim(const struct bpf_prog *prog,
 			     bpf_func_t *bpf_func)
 {
 	const struct btf_param *args __maybe_unused;
+	u32 type_id __maybe_unused = 0;
 
 	if (btf_type_vlen(prog->aux->attach_func_proto) < 1 ||
 	    btf_id_set_contains(&bpf_lsm_current_hooks,
@@ -106,9 +107,12 @@ void bpf_lsm_find_cgroup_shim(const struct bpf_prog *prog,
 #ifdef CONFIG_NET
 	args = btf_params(prog->aux->attach_func_proto);
 
-	if (args[0].type == btf_sock_ids[BTF_SOCK_TYPE_SOCKET])
+	/* args[0] is a pointer; match the struct it points to. */
+	btf_type_resolve_ptr(prog->aux->attach_btf, args[0].type, &type_id);
+
+	if (type_id == btf_sock_ids[BTF_SOCK_TYPE_SOCKET])
 		*bpf_func = __cgroup_bpf_run_lsm_socket;
-	else if (args[0].type == btf_sock_ids[BTF_SOCK_TYPE_SOCK])
+	else if (type_id == btf_sock_ids[BTF_SOCK_TYPE_SOCK])
 		*bpf_func = __cgroup_bpf_run_lsm_sock;
 	else
 #endif
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 57a785241d42f..9100d1c4f35ff 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -171,15 +171,20 @@ unsigned int __cgroup_bpf_run_lsm_socket(const void *ctx,
 	const struct bpf_prog *shim_prog;
 	struct socket *sock;
 	struct cgroup *cgrp;
+	struct sock *sk;
 	int ret = 0;
 	u64 *args;
 
 	args = (u64 *)ctx;
 	sock = (void *)(unsigned long)args[0];
+	sk = sock ? sock->sk : NULL;
+	/* NULL from ip_cmsg_recv_security(), no sk yet in sock_create_lite() */
+	if (unlikely(!sk))
+		return __cgroup_bpf_run_lsm_current(ctx, insn);
 	/*shim_prog = container_of(insn, struct bpf_prog, insnsi);*/
 	shim_prog = (const struct bpf_prog *)((void *)insn - offsetof(struct bpf_prog, insnsi));
 
-	cgrp = sock_cgroup_ptr(&sock->sk->sk_cgrp_data);
+	cgrp = sock_cgroup_ptr(&sk->sk_cgrp_data);
 	if (likely(cgrp))
 		ret = bpf_prog_run_array_cg(&cgrp->bpf,
 					    shim_prog->aux->cgroup_atype,
-- 
2.53.0