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: <CAL+tcoANvM3hD6avRm+j_jf5XkW8NaqtSYXxcfEcx3xGnmqKVg@mail.gmail.com>
Date: Sat, 25 Jan 2025 10:58:10 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
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 Sat, Jan 25, 2025 at 10:26 AM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> 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.

Oh, after reading the above a few times, I finally realized you're
right. I'll do it. Thanks!!

>
> 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.

Right, right!

Thanks,
Jason