Re: [PATCH net-next v5 05/15] net-timestamp: add strict check in some BPF calls

[Martin KaFai Lau <martin.lau@linux.dev>]

On 1/12/25 3:37 AM, Jason Xing wrote:
> In the next round, we will support the UDP proto for SO_TIMESTAMPING
> bpf extension, so we need to ensure there is no safety problem.
> 
> Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> ---
>   net/core/filter.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 0e915268db5f..517f09aabc92 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -5571,7 +5571,7 @@ static int __bpf_getsockopt(struct sock *sk, int level, int optname,
>   static int _bpf_getsockopt(struct sock *sk, int level, int optname,
>   			   char *optval, int optlen)
>   {
> -	if (sk_fullsock(sk))
> +	if (sk_fullsock(sk) && optname != SK_BPF_CB_FLAGS)
>   		sock_owned_by_me(sk);
>   	return __bpf_getsockopt(sk, level, optname, optval, optlen);
>   }
> @@ -5776,6 +5776,7 @@ BPF_CALL_5(bpf_sock_ops_getsockopt, struct bpf_sock_ops_kern *, bpf_sock,
>   	   int, level, int, optname, char *, optval, int, optlen)
>   {
>   	if (IS_ENABLED(CONFIG_INET) && level == SOL_TCP &&
> +	    bpf_sock->sk->sk_protocol == IPPROTO_TCP &&
>   	    optname >= TCP_BPF_SYN && optname <= TCP_BPF_SYN_MAC) {
>   		int ret, copy_len = 0;
>   		const u8 *start;
> @@ -5817,7 +5818,8 @@ BPF_CALL_2(bpf_sock_ops_cb_flags_set, struct bpf_sock_ops_kern *, bpf_sock,
>   	struct sock *sk = bpf_sock->sk;
>   	int val = argval & BPF_SOCK_OPS_ALL_CB_FLAGS;
>   
> -	if (!IS_ENABLED(CONFIG_INET) || !sk_fullsock(sk))
> +	if (!IS_ENABLED(CONFIG_INET) || !sk_fullsock(sk) ||
> +	    sk->sk_protocol != IPPROTO_TCP)
>   		return -EINVAL;
>   
>   	tcp_sk(sk)->bpf_sock_ops_cb_flags = val;
> @@ -7626,6 +7628,9 @@ BPF_CALL_4(bpf_sock_ops_load_hdr_opt, struct bpf_sock_ops_kern *, bpf_sock,
>   	u8 search_kind, search_len, copy_len, magic_len;
>   	int ret;
>   
> +	if (bpf_sock->op != SK_BPF_CB_FLAGS)

SK_BPF_CB_FLAGS is not an op enum, so the check is incorrect. It does break the 
existing test.

./test_progs -t tcp_hdr_options
WARNING! Selftests relying on bpf_testmod.ko will be skipped.
#402/1   tcp_hdr_options/simple_estab:FAIL
#402/2   tcp_hdr_options/no_exprm_estab:FAIL
#402/3   tcp_hdr_options/syncookie_estab:FAIL
#402/4   tcp_hdr_options/fastopen_estab:FAIL
#402/5   tcp_hdr_options/fin:FAIL
#402/6   tcp_hdr_options/misc:FAIL
#402     tcp_hdr_options:FAIL
#402/1   tcp_hdr_options/simple_estab:FAIL
#402/2   tcp_hdr_options/no_exprm_estab:FAIL
#402/3   tcp_hdr_options/syncookie_estab:FAIL
#402/4   tcp_hdr_options/fastopen_estab:FAIL
#402/5   tcp_hdr_options/fin:FAIL
#402/6   tcp_hdr_options/misc:FAIL
#402     tcp_hdr_options:FAIL


Many changes of this set is in bpf and the newly added selftest is also a bpf 
prog, all bpf selftests should be run before posting. 
(Documentation/bpf/bpf_devel_QA.rst)

The bpf CI can automatically pick it up and get an auto email on breakage like 
this if the set is tagged to bpf-next. We can figure out where to land the set 
later (bpf-next/net or net-next/main) when it is ready.

All these changes also need a test in selftests/bpf. For example, I expect there 
is a test to ensure calling these bpf helpers from the new tstamp callback will 
get a negative errno value.

For patch 4 and patch 5, I would suggest keeping it simple to only check for 
bpf_sock->op for the helpers that make tcp_sock and/or locked sk assumption.
Something like this on top of your patch. Untested:

diff --git i/net/core/filter.c w/net/core/filter.c
index 517f09aabc92..ccb13b61c528 100644
--- i/net/core/filter.c
+++ w/net/core/filter.c
@@ -7620,6 +7620,11 @@ static const u8 *bpf_search_tcp_opt(const u8 *op, const 
u8 *opend,
  	return ERR_PTR(-ENOMSG);
  }

+static bool is_locked_tcp_sock_ops(struct bpf_sock_ops_kern *bpf_sock)
+{
+	return bpf_sock->op <= BPF_SOCK_OPS_WRITE_HDR_OPT_CB;
+}
+
  BPF_CALL_4(bpf_sock_ops_load_hdr_opt, struct bpf_sock_ops_kern *, bpf_sock,
  	   void *, search_res, u32, len, u64, flags)
  {
@@ -7628,8 +7633,8 @@ BPF_CALL_4(bpf_sock_ops_load_hdr_opt, struct 
bpf_sock_ops_kern *, bpf_sock,
  	u8 search_kind, search_len, copy_len, magic_len;
  	int ret;

-	if (bpf_sock->op != SK_BPF_CB_FLAGS)
-		return -EINVAL;
+	if (!is_locked_tcp_sock_ops(bpf_sock))
+		return -EOPNOTSUPP;

  	/* 2 byte is the minimal option len except TCPOPT_NOP and
  	 * TCPOPT_EOL which are useless for the bpf prog to learn


> +		return -EINVAL;
> +
>   	/* 2 byte is the minimal option len except TCPOPT_NOP and
>   	 * TCPOPT_EOL which are useless for the bpf prog to learn
>   	 * and this helper disallow loading them also.