netdev - Re: [RFC PATCH net-next v6 04/13] bpf: stop UDP sock accessing TCP fields in sock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <331cec22-3931-4723-aa5a-03d8a9dc6040@linux.dev>
Date: Fri, 24 Jan 2025 18:25:40 -0800
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Jason Xing <kerneljasonxing@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, dsahern@...nel.org, willemdebruijn.kernel@...il.com,
 willemb@...gle.com, ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
 eddyz87@...il.com, song@...nel.org, yonghong.song@...ux.dev,
 john.fastabend@...il.com, kpsingh@...nel.org, sdf@...ichev.me,
 haoluo@...gle.com, jolsa@...nel.org, horms@...nel.org, bpf@...r.kernel.org,
 netdev@...r.kernel.org
Subject: Re: [RFC PATCH net-next v6 04/13] bpf: stop UDP sock accessing TCP
 fields in sock_op BPF CALLs

On 1/24/25 5:15 PM, Jason Xing wrote:
>>> +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;
>>
>> More bike shedding...
>>
>> After sleeping on it again, I think it can just test the
>> bpf_sock->allow_tcp_access instead.
> 
> Sorry, I don't think it can work for all the cases because:
> 1) please see BPF_SOCK_OPS_WRITE_HDR_OPT_CB/BPF_SOCK_OPS_HDR_OPT_LEN_CB,
> if req exists, there is no allow_tcp_access initialization. Then
> calling some function like bpf_sock_ops_setsockopt will be rejected
> because allow_tcp_access is zero.
> 2) tcp_call_bpf() only set allow_tcp_access only when the socket is
> fullsock. As far as I know, all the callers have the full stock for
> now, but in the future it might not.

Note that the existing helper bpf_sock_ops_cb_flags_set and 
bpf_sock_ops_{set,get}sockopt itself have done the sk_fullsock() test and then 
return -EINVAL. bpf_sock->sk is fullsock or not does not matter to these helpers.

You are right on the BPF_SOCK_OPS_WRITE_HDR_OPT_CB/BPF_SOCK_OPS_HDR_OPT_LEN_CB 
but the only helper left that testing allow_tcp_access is not enough is 
bpf_sock_ops_load_hdr_opt(). Potentially, it can test "if 
(!bpf_sock->allow_tcp_access && !bpf_sock->syn_skb) { return -EOPNOTSUPP; }".

Agree to stay with the current "bpf_sock->op <= BPF_SOCK_OPS_WRITE_HDR_OPT_CB" 
as in this patch. It is cleaner.

>>> @@ -5673,7 +5678,12 @@ static const struct bpf_func_proto bpf_sock_addr_getsockopt_proto = {
>>>    BPF_CALL_5(bpf_sock_ops_setsockopt, struct bpf_sock_ops_kern *, bpf_sock,
>>>           int, level, int, optname, char *, optval, int, optlen)
>>>    {
>>> -     return _bpf_setsockopt(bpf_sock->sk, level, optname, optval, optlen);
>>> +     struct sock *sk = bpf_sock->sk;
>>> +
>>> +     if (is_locked_tcp_sock_ops(bpf_sock) && sk_fullsock(sk))
>>
>> afaict, the new timestamping callbacks still can do setsockopt and it is
>> incorrect. It should be:
>>
>>          if (!bpf_sock->allow_tcp_access)
>>                  return -EOPNOTSUPP;
>>
>> I recalled I have asked in v5 but it may be buried in the long thread, so asking
>> here again. Please add test(s) to check that the new timestamping callbacks
>> cannot call setsockopt and read/write to some of the tcp_sock fields through the
>> bpf_sock_ops.
>>
>>> +             sock_owned_by_me(sk);
>>
>> Not needed and instead...
> 
> Sorry I don't get you here. What I was doing was letting non
> timestamping callbacks be checked by the sock_owned_by_me() function.
> 
> If the callback belongs to timestamping, we will skip the check.

It will skip the sock_owned_by_me() test and
continue to do the following __bpf_setsockopt() which the new timetamping 
callback should not do, no?

It should be just this at the very beginning of bpf_sock_ops_setsockopt:

	if (!is_locked_tcp_sock_ops(bpf_sock))
		return -EOPNOTSUPP;
> 
>>
>>> +
>>> +     return __bpf_setsockopt(sk, level, optname, optval, optlen);
>>
>> keep the original _bpf_setsockopt().
> 
> Oh, I remembered we've already assumed/agreed the timestamping socket
> must be full sock. I will use it.
> 
>>
>>>    }
>>>
>>>    static const struct bpf_func_proto bpf_sock_ops_setsockopt_proto = {
>>> @@ -5759,6 +5769,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) {
>>
>> No need to allow getsockopt regardless what SOL_* it is asking. To keep it
>> simple, I would just disable both getsockopt and setsockopt for all SOL_* for
> 
> Really? I'm shocked because the selftests in this series call
> bpf_sock_ops_getsockopt() and bpf_sock_ops_setsockopt() in patch
> [13/13]:

Yes, really. It may be late Friday for me here. Please double check your test if 
the bpf_set/getsockopt is called from the new timestamp callback or it is only 
called from the existing BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB callback.

Note that I am only asking to disable the set/getsockopt, 
bpf_sock_ops_cb_flags_set, and the bpf_sock_ops_load_hdr_opt for the new 
timestamping callbacks.