[bpf-next,v9,03/12] bpf: stop unsafely accessing TCP fields in bpf callbacks

Commit Message

Jason Xing Feb. 8, 2025, 10:32 a.m. UTC

The "is_locked_tcp_sock" flag is added to indicate that the callback
site has a tcp_sock locked.

Apply the new member is_locked_tcp_sock in the existing callbacks
where is_fullsock is set to 1 can stop UDP socket accessing struct
tcp_sock and stop TCP socket without sk lock protecting does the
similar thing, or else it could be catastrophe leading to panic.

To keep it simple, instead of distinguishing between read and write
access, users aren't allowed all read/write access to the tcp_sock
through the older bpf_sock_ops ctx. The new timestamping callbacks
can use newer helpers to read everything from a sk (e.g. bpf_core_cast),
so nothing is lost.

Signed-off-by: Jason Xing <[e-mail]>
---
 include/linux/filter.h | 1 +
 include/net/tcp.h      | 1 +
 net/core/filter.c      | 8 ++++----
 net/ipv4/tcp_input.c   | 2 ++
 net/ipv4/tcp_output.c  | 2 ++
 5 files changed, 10 insertions(+), 4 deletions(-)

Comments

Martin KaFai Lau Feb. 11, 2025, 6:34 a.m. UTC | #1

On 2/8/25 2:32 AM, Jason Xing wrote:
> The "is_locked_tcp_sock" flag is added to indicate that the callback
> site has a tcp_sock locked.

It should mention that the later TX timestamping callbacks will not own the 
lock. This is what this patch is primarily for. We know the background, but 
future code readers may not. We will eventually become the readers of this patch 
in a few years' time.

> 
> Apply the new member is_locked_tcp_sock in the existing callbacks

It is hard to read "Apply the new member....". "Apply" could mean a few things. 
"Set to 1" is clearer.


> where is_fullsock is set to 1 can stop UDP socket accessing struct

The UDP part is future proof. This set does not support UDP which has to be 
clear in the commit message. This has been brought up before also.

> tcp_sock and stop TCP socket without sk lock protecting does the
> similar thing, or else it could be catastrophe leading to panic.
> 
> To keep it simple, instead of distinguishing between read and write
> access, users aren't allowed all read/write access to the tcp_sock
> through the older bpf_sock_ops ctx. The new timestamping callbacks
> can use newer helpers to read everything from a sk (e.g. bpf_core_cast),
> so nothing is lost.

(Subject):
bpf: Prevent unsafe access to the sock fields in the BPF timestamping callback

(Why):
The subsequent patch will implement BPF TX timestamping. It will call the 
sockops BPF program without holding the sock lock.

This breaks the current assumption that all sock ops programs will hold the sock 
lock. The sock's fields of the uapi's bpf_sock_ops requires this assumption.

(What and How):
To address this,
a new "u8 is_locked_tcp_sock;" field is added. This patch sets it in the current 
sock_ops callbacks. The "is_fullsock" test is then replaced by the 
"is_locked_tcp_sock" test during sock_ops_convert_ctx_access().

The new TX timestamping callbacks added in the subsequent patch will not have 
this set. This will prevent unsafe access from the new timestamping callbacks.

Potentially, we could allow read-only access. However, this would require 
identifying which callback is read-safe-only and also requires additional BPF 
instruction rewrites in the covert_ctx. Since the BPF program can always read 
everything from a socket (e.g., by using bpf_core_cast), this patch keeps it 
simple and disables all read and write access to any socket fields through the 
bpf_sock_ops UAPI from the new TX timestamping callback.

Moreover, note that some of the fields in bpf_sock_ops are specific to tcp_sock, 
and sock_ops currently only supports tcp_sock. In the future, UDP timestamping 
will be added, which will also break this assumption. The same idea used in this 
patch will be reused. Considering that the current sock_ops only supports 
tcp_sock, the variable is named is_locked_"tcp"_sock.

Jason Xing Feb. 11, 2025, 8:08 a.m. UTC | #2

On Tue, Feb 11, 2025 at 2:34 PM Martin KaFai Lau <[e-mail]> wrote:
>
> On 2/8/25 2:32 AM, Jason Xing wrote:
> > The "is_locked_tcp_sock" flag is added to indicate that the callback
> > site has a tcp_sock locked.
>
> It should mention that the later TX timestamping callbacks will not own the
> lock. This is what this patch is primarily for. We know the background, but
> future code readers may not. We will eventually become the readers of this patch
> in a few years' time.
>
> >
> > Apply the new member is_locked_tcp_sock in the existing callbacks
>
> It is hard to read "Apply the new member....". "Apply" could mean a few things.
> "Set to 1" is clearer.
>
>
> > where is_fullsock is set to 1 can stop UDP socket accessing struct
>
> The UDP part is future proof. This set does not support UDP which has to be
> clear in the commit message. This has been brought up before also.
>
> > tcp_sock and stop TCP socket without sk lock protecting does the
> > similar thing, or else it could be catastrophe leading to panic.
> >
> > To keep it simple, instead of distinguishing between read and write
> > access, users aren't allowed all read/write access to the tcp_sock
> > through the older bpf_sock_ops ctx. The new timestamping callbacks
> > can use newer helpers to read everything from a sk (e.g. bpf_core_cast),
> > so nothing is lost.
>
> (Subject):
> bpf: Prevent unsafe access to the sock fields in the BPF timestamping callback
>
> (Why):
> The subsequent patch will implement BPF TX timestamping. It will call the
> sockops BPF program without holding the sock lock.
>
> This breaks the current assumption that all sock ops programs will hold the sock
> lock. The sock's fields of the uapi's bpf_sock_ops requires this assumption.
>
> (What and How):
> To address this,
> a new "u8 is_locked_tcp_sock;" field is added. This patch sets it in the current
> sock_ops callbacks. The "is_fullsock" test is then replaced by the
> "is_locked_tcp_sock" test during sock_ops_convert_ctx_access().
>
> The new TX timestamping callbacks added in the subsequent patch will not have
> this set. This will prevent unsafe access from the new timestamping callbacks.
>
> Potentially, we could allow read-only access. However, this would require
> identifying which callback is read-safe-only and also requires additional BPF
> instruction rewrites in the covert_ctx. Since the BPF program can always read
> everything from a socket (e.g., by using bpf_core_cast), this patch keeps it
> simple and disables all read and write access to any socket fields through the
> bpf_sock_ops UAPI from the new TX timestamping callback.
>
> Moreover, note that some of the fields in bpf_sock_ops are specific to tcp_sock,
> and sock_ops currently only supports tcp_sock. In the future, UDP timestamping
> will be added, which will also break this assumption. The same idea used in this
> patch will be reused. Considering that the current sock_ops only supports
> tcp_sock, the variable is named is_locked_"tcp"_sock.

Thanks so much for polishing the commit message. I appreciate it!

Will adjust it.

Thanks,
Jason

Patch

diff --git a/include/linux/filter.h b/include/linux/filter.h
index a3ea46281595..d36d5d5180b1 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1508,6 +1508,7 @@  struct bpf_sock_ops_kern {
 	void	*skb_data_end;
 	u8	op;
 	u8	is_fullsock;
+	u8	is_locked_tcp_sock;
 	u8	remaining_opt_len;
 	u64	temp;			/* temp and everything after is not
 					 * initialized to 0 before calling
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 5b2b04835688..4c4dca59352b 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2649,6 +2649,7 @@  static inline int tcp_call_bpf(struct sock *sk, int op, u32 nargs, u32 *args)
 	memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp));
 	if (sk_fullsock(sk)) {
 		sock_ops.is_fullsock = 1;
+		sock_ops.is_locked_tcp_sock = 1;
 		sock_owned_by_me(sk);
 	}
 
diff --git a/net/core/filter.c b/net/core/filter.c
index 1c6c07507a78..8631036f6b64 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -10381,10 +10381,10 @@  static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
 		}							      \
 		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(			      \
 						struct bpf_sock_ops_kern,     \
-						is_fullsock),		      \
+						is_locked_tcp_sock),	      \
 				      fullsock_reg, si->src_reg,	      \
 				      offsetof(struct bpf_sock_ops_kern,      \
-					       is_fullsock));		      \
+					       is_locked_tcp_sock));	      \
 		*insn++ = BPF_JMP_IMM(BPF_JEQ, fullsock_reg, 0, jmp);	      \
 		if (si->dst_reg == si->src_reg)				      \
 			*insn++ = BPF_LDX_MEM(BPF_DW, reg, si->src_reg,	      \
@@ -10469,10 +10469,10 @@  static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
 					       temp));			      \
 		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(			      \
 						struct bpf_sock_ops_kern,     \
-						is_fullsock),		      \
+						is_locked_tcp_sock),	      \
 				      reg, si->dst_reg,			      \
 				      offsetof(struct bpf_sock_ops_kern,      \
-					       is_fullsock));		      \
+					       is_locked_tcp_sock));	      \
 		*insn++ = BPF_JMP_IMM(BPF_JEQ, reg, 0, 2);		      \
 		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(			      \
 						struct bpf_sock_ops_kern, sk),\
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index eb82e01da911..95733dcdfb4b 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -169,6 +169,7 @@  static void bpf_skops_parse_hdr(struct sock *sk, struct sk_buff *skb)
 	memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp));
 	sock_ops.op = BPF_SOCK_OPS_PARSE_HDR_OPT_CB;
 	sock_ops.is_fullsock = 1;
+	sock_ops.is_locked_tcp_sock = 1;
 	sock_ops.sk = sk;
 	bpf_skops_init_skb(&sock_ops, skb, tcp_hdrlen(skb));
 
@@ -185,6 +186,7 @@  static void bpf_skops_established(struct sock *sk, int bpf_op,
 	memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp));
 	sock_ops.op = bpf_op;
 	sock_ops.is_fullsock = 1;
+	sock_ops.is_locked_tcp_sock = 1;
 	sock_ops.sk = sk;
 	/* sk with TCP_REPAIR_ON does not have skb in tcp_finish_connect */
 	if (skb)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 0e5b9a654254..75e935ec7916 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -522,6 +522,7 @@  static void bpf_skops_hdr_opt_len(struct sock *sk, struct sk_buff *skb,
 		sock_owned_by_me(sk);
 
 		sock_ops.is_fullsock = 1;
+		sock_ops.is_locked_tcp_sock = 1;
 		sock_ops.sk = sk;
 	}
 
@@ -567,6 +568,7 @@  static void bpf_skops_write_hdr_opt(struct sock *sk, struct sk_buff *skb,
 		sock_owned_by_me(sk);
 
 		sock_ops.is_fullsock = 1;
+		sock_ops.is_locked_tcp_sock = 1;
 		sock_ops.sk = sk;
 	}