linux-kernel - Re: [PATCH 2/2] net/tcp: Disable TCP-MD5 static key on tcp_md5sig

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]

Date:   Wed, 2 Nov 2022 14:49:38 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     Dmitry Safonov <dima@...sta.com>
Cc:     linux-kernel@...r.kernel.org, David Ahern <dsahern@...nel.org>,
        Bob Gilligan <gilligan@...sta.com>,
        "David S. Miller" <davem@...emloft.net>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Francesco Ruggeri <fruggeri@...sta.com>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Salam Noureddine <noureddine@...sta.com>,
        netdev@...r.kernel.org
Subject: Re: [PATCH 2/2] net/tcp: Disable TCP-MD5 static key on
 tcp_md5sig_info destruction

On Wed, Nov 2, 2022 at 2:40 PM Dmitry Safonov <dima@...sta.com> wrote:
>
> On 11/2/22 21:25, Eric Dumazet wrote:
> > On Wed, Nov 2, 2022 at 2:14 PM Dmitry Safonov <dima@...sta.com> wrote:
> [..]
> >> +int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr,
> >> +                  int family, u8 prefixlen, int l3index, u8 flags,
> >> +                  const u8 *newkey, u8 newkeylen)
> >> +{
> >> +       struct tcp_sock *tp = tcp_sk(sk);
> >> +
> >> +       if (!rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk))) {
> >> +               if (tcp_md5sig_info_add(sk, GFP_KERNEL))
> >> +                       return -ENOMEM;
> >> +
> >> +               static_branch_inc(&tcp_md5_needed.key);
> >> +       }
> >> +
> >> +       return __tcp_md5_do_add(sk, addr, family, prefixlen, l3index, flags,
> >> +                               newkey, newkeylen, GFP_KERNEL);
> >> +}
> >>  EXPORT_SYMBOL(tcp_md5_do_add);
> >>
> >> +int tcp_md5_key_copy(struct sock *sk, const union tcp_md5_addr *addr,
> >> +                    int family, u8 prefixlen, int l3index,
> >> +                    struct tcp_md5sig_key *key)
> >> +{
> >> +       struct tcp_sock *tp = tcp_sk(sk);
> >> +
> >> +       if (!rcu_dereference_protected(tp->md5sig_info, lockdep_sock_is_held(sk))) {
> >> +               if (tcp_md5sig_info_add(sk, sk_gfp_mask(sk, GFP_ATOMIC)))
> >> +                       return -ENOMEM;
> >> +
> >> +               atomic_inc(&tcp_md5_needed.key.key.enabled);
> >
> > static_branch_inc ?
>
> That's the difference between tcp_md5_do_add() and tcp_md5_key_copy():
> the first one can sleep on either allocation or static branch patching,
> while the second one is used where there is md5 key and it can't get
> destroyed during the function call. tcp_md5_key_copy() is called
> somewhere from the softirq handler so it needs an atomic allocation as
> well as this a little bit hacky part.
>

Are you sure ?

static_branch_inc() is what we want here, it is a nice wrapper around
the correct internal details,
and ultimately boils to an atomic_inc(). It is safe for all contexts.

But if/when jump labels get refcount_t one day, we will not have to
change TCP stack because
it made some implementation assumptions.