A bpf prog can do this today:
subflow = bpf_skc_lookup_tcp(...);
msk = bpf_skc_to_mptcp_sock(subflow);
bpf_sk_release(msk);
bpf_skc_to_mptcp_sock() returns subflow->conn without taking any
reference, so bpf_sk_release() drops a refcount nobody took on the msk,
and the subflow reference is leaked:
refcount_t: underflow; use-after-free.
WARNING: lib/refcount.c:28 at refcount_warn_saturate+0xdf/0x120, CPU#2:
mptcp_pair/440
Call Trace:
<IRQ>
sock_gen_put+0xda/0x100
bpf_sk_release+0x5e/0xd0
bpf_prog_780c70b94862636c_rel_msk+0x127/0x132
__dev_queue_xmit+0x104b/0x3c90
ip_finish_output2+0x9af/0x1c40
__ip_finish_output+0x510/0x7e0
ip_finish_output+0x2f/0x320
ip_output+0x17a/0x3f0
ip_local_out+0x12f/0x170
__ip_queue_xmit+0x81d/0x1d50
ip_queue_xmit+0x4a/0x80
__tcp_transmit_skb+0x2f6f/0x5110
__tcp_send_ack.part.0+0x385/0x740
tcp_send_ack+0x70/0x90
__tcp_ack_snd_check+0x1c9/0x8d0
tcp_rcv_established+0xa1e/0x44f0
tcp_v4_do_rcv+0x4b8/0xb30
tcp_v4_rcv+0x27af/0x3e60
ip_protocol_deliver_rcu+0x95/0x410
ip_local_deliver_finish+0x357/0x5b0
ip_local_deliver+0x15c/0x1c0
ip_rcv+0x284/0x320
bpf_skc_to_mptcp_sock() is listed in is_ptr_cast_function(), so if the
subflow is a referenced obj (returned by bpf_skc_lookup_tcp()), the msk
becomes a referenced obj too, which lets bpf_sk_release() take it. That
list is for helpers casting a sock to another type at the same address,
which does not hold here: msk and subflow are two different socks.
Drop it from is_ptr_cast_function(). To keep the msk from outliving the
subflow it was derived from, tie the two together with the existing
parent_id, so this gets rejected as well:
subflow = bpf_skc_lookup_tcp(...);
msk = bpf_skc_to_mptcp_sock(subflow);
bpf_sk_release(subflow);
msk->token; /* rejected now */
The other option was to reject bpf_skc_to_mptcp_sock() on a referenced
subflow altogether, but that breaks progs which only read msk fields, so
go with parent_id.
Reported-by: VEGA <[email protected]>
Fixes: 3bc253c2e652 ("bpf: Add bpf_skc_to_mptcp_sock_proto")
Signed-off-by: Jiayuan Chen <[email protected]>
---
kernel/bpf/verifier.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 9e79750e2480..5db49a0c346f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -522,11 +522,21 @@ static bool is_ptr_cast_function(enum bpf_func_id func_id)
func_id == BPF_FUNC_skc_to_tcp_sock ||
func_id == BPF_FUNC_skc_to_tcp6_sock ||
func_id == BPF_FUNC_skc_to_udp6_sock ||
- func_id == BPF_FUNC_skc_to_mptcp_sock ||
func_id == BPF_FUNC_skc_to_tcp_timewait_sock ||
func_id == BPF_FUNC_skc_to_tcp_request_sock;
}
+/*
+ * bpf_skc_to_mptcp_sock() does not cast its argument. It returns the parent
+ * MPTCP socket of the subflow that was passed in, so the return value must not
+ * inherit the argument's reference, or bpf_sk_release() would put the wrong
+ * socket.
+ */
+static bool is_ptr_derive_function(enum bpf_func_id func_id)
+{
+ return func_id == BPF_FUNC_skc_to_mptcp_sock;
+}
+
static bool is_sync_callback_calling_kfunc(u32 btf_id);
static bool is_async_callback_calling_kfunc(u32 btf_id);
static bool is_callback_calling_kfunc(u32 btf_id);
@@ -11369,6 +11379,14 @@ static int check_helper_call(struct bpf_verifier_env
*env, struct bpf_insn *insn
bpf_diag_mod_begin(env, ®s[BPF_REG_0], NULL,
BPF_DIAG_MOD_WRITE);
regs[BPF_REG_0].type &= ~PTR_MAYBE_NULL;
regs[BPF_REG_0].id = meta.ref_obj.id;
+ } else if (is_ptr_derive_function(func_id) &&
+ find_reference_state(env->cur_state, meta.ref_obj.id)) {
+ err = validate_ref_obj(env, &meta.ref_obj);
+ if (err)
+ return err;
+
+ /* Ensures we don't access the object after a
release_reference() */
+ regs[BPF_REG_0].parent_id = meta.ref_obj.id;
} else if (is_acquire_function(func_id, meta.map.ptr)) {
int id = acquire_reference(env, insn_idx, 0);
--
2.43.0
