| 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
| ||
|
Date: Wed, 7 Sep 2022 19:19:00 +0300
From: Leonard Crestez <cdleonard@...il.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: David Ahern <dsahern@...nel.org>,
Dmitry Safonov <0x7f454c46@...il.com>,
Francesco Ruggeri <fruggeri@...sta.com>,
Salam Noureddine <noureddine@...sta.com>,
Philip Paeps <philip@...uble.is>,
Shuah Khan <shuah@...nel.org>,
"David S. Miller" <davem@...emloft.net>,
Herbert Xu <herbert@...dor.apana.org.au>,
Kuniyuki Iwashima <kuniyu@...zon.co.jp>,
Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
Jakub Kicinski <kuba@...nel.org>,
Yuchung Cheng <ycheng@...gle.com>,
Mat Martineau <mathew.j.martineau@...ux.intel.com>,
Christoph Paasch <cpaasch@...le.com>,
Ivan Delalande <colona@...sta.com>,
Caowangbao <caowangbao@...wei.com>,
Priyaranjan Jha <priyarjha@...gle.com>,
netdev <netdev@...r.kernel.org>,
"open list:HARDWARE RANDOM NUMBER GENERATOR CORE"
<linux-crypto@...r.kernel.org>,
"open list:KERNEL SELFTEST FRAMEWORK"
<linux-kselftest@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v8 01/26] tcp: authopt: Initial support and key management
On 9/7/22 01:57, Eric Dumazet wrote:
> On Mon, Sep 5, 2022 at 12:06 AM Leonard Crestez <cdleonard@...il.com> wrote:
>>
>> This commit adds support to add and remove keys but does not use them
>> further.
>>
>> Similar to tcp md5 a single pointer to a struct tcp_authopt_info* struct
>> is added to struct tcp_sock, this avoids increasing memory usage. The
>> data structures related to tcp_authopt are initialized on setsockopt and
>> only freed on socket close.
>>
>
> Thanks Leonard.
>
> Small points from my side, please find them attached.
...
>> +/* Free info and keys.
>> + * Don't touch tp->authopt_info, it might not even be assigned yes.
>> + */
>> +void tcp_authopt_free(struct sock *sk, struct tcp_authopt_info *info)
>> +{
>> + kfree_rcu(info, rcu);
>> +}
>> +
>> +/* Free everything and clear tcp_sock.authopt_info to NULL */
>> +void tcp_authopt_clear(struct sock *sk)
>> +{
>> + struct tcp_authopt_info *info;
>> +
>> + info = rcu_dereference_protected(tcp_sk(sk)->authopt_info, lockdep_sock_is_held(sk));
>> + if (info) {
>> + tcp_authopt_free(sk, info);
>> + tcp_sk(sk)->authopt_info = NULL;
>
> RCU rules at deletion mandate that the pointer must be cleared before
> the call_rcu()/kfree_rcu() call.
>
> It is possible that current MD5 code has an issue here, let's not copy/paste it.
OK. Is there a need for some special form of assignment or is current
plain form enough?
>
>> + }
>> +}
>> +
>> +/* checks that ipv4 or ipv6 addr matches. */
>> +static bool ipvx_addr_match(struct sockaddr_storage *a1,
>> + struct sockaddr_storage *a2)
>> +{
>> + if (a1->ss_family != a2->ss_family)
>> + return false;
>> + if (a1->ss_family == AF_INET &&
>> + (((struct sockaddr_in *)a1)->sin_addr.s_addr !=
>> + ((struct sockaddr_in *)a2)->sin_addr.s_addr))
>> + return false;
>> + if (a1->ss_family == AF_INET6 &&
>> + !ipv6_addr_equal(&((struct sockaddr_in6 *)a1)->sin6_addr,
>> + &((struct sockaddr_in6 *)a2)->sin6_addr))
>> + return false;
>> + return true;
>> +}
>
> Always surprising to see this kind of generic helper being added in a patch.
I remember looking for an equivalent and not finding it. Many places
have distinct code paths for ipv4 and ipv6 and my use of
"sockaddr_storage" as ipv4/ipv6 union is uncommon.
It also wastes some memory.
>> +int tcp_get_authopt_val(struct sock *sk, struct tcp_authopt *opt)
>> +{
>> + struct tcp_sock *tp = tcp_sk(sk);
>> + struct tcp_authopt_info *info;
>> +
>> + memset(opt, 0, sizeof(*opt));
>> + sock_owned_by_me(sk);
>> +
>> + info = rcu_dereference_check(tp->authopt_info, lockdep_sock_is_held(sk));
>
> Probably not a big deal, but it seems the prior sock_owned_by_me()
> might be redundant.
The sock_owned_by_me call checks checks lockdep_sock_is_held
The rcu_dereference_check call checks lockdep_sock_is_held ||
rcu_read_lock_held()
This is a getsockopt so caller ensures socket locking but
rcu_read_lock_held() == 0.
The sock_owned_by_me is indeed redundant because it seems very unlikely
the sockopt calling conditions will be changes. It was mostly there to
clarify for myself because I had probably at one time with locking
warnings. I guess they can be removed.
>> +int tcp_set_authopt_key(struct sock *sk, sockptr_t optval, unsigned int optlen)
>> +{
>> + struct tcp_authopt_key opt;
>> + struct tcp_authopt_info *info;
>> + struct tcp_authopt_key_info *key_info, *old_key_info;
>> + struct netns_tcp_authopt *net = sock_net_tcp_authopt(sk);
>> + int err;
>> +
>> + sock_owned_by_me(sk);
>> + if (!ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN))
>> + return -EPERM;
>> +
>> + err = _copy_from_sockptr_tolerant((u8 *)&opt, sizeof(opt), optval, optlen);
>> + if (err)
>> + return err;
>> +
>> + if (opt.flags & ~TCP_AUTHOPT_KEY_KNOWN_FLAGS)
>> + return -EINVAL;
>> +
>> + if (opt.keylen > TCP_AUTHOPT_MAXKEYLEN)
>> + return -EINVAL;
>> +
>> + /* Delete is a special case: */
>> + if (opt.flags & TCP_AUTHOPT_KEY_DEL) {
>> + mutex_lock(&net->mutex);
>> + key_info = tcp_authopt_key_lookup_exact(sk, net, &opt);
>> + if (key_info) {
>> + tcp_authopt_key_del(net, key_info);
>> + err = 0;
>> + } else {
>> + err = -ENOENT;
>> + }
>> + mutex_unlock(&net->mutex);
>> + return err;
>> + }
>> +
>> + /* check key family */
>> + if (opt.flags & TCP_AUTHOPT_KEY_ADDR_BIND) {
>> + if (sk->sk_family != opt.addr.ss_family)
>> + return -EINVAL;
>> + }
>> +
>> + /* Initialize tcp_authopt_info if not already set */
>> + info = __tcp_authopt_info_get_or_create(sk);
>> + if (IS_ERR(info))
>> + return PTR_ERR(info);
>> +
>> + key_info = kmalloc(sizeof(*key_info), GFP_KERNEL | __GFP_ZERO);
>
> kzalloc() ?
Yes
>> +static int tcp_authopt_init_net(struct net *full_net)
>
> Hmmm... our convention is to use "struct net *net"
>
>> +{
>> + struct netns_tcp_authopt *net = &full_net->tcp_authopt;
>
> Here, you should use a different name ...
OK, will replace with net_ao
>> @@ -2267,10 +2268,11 @@ void tcp_v4_destroy_sock(struct sock *sk)
>> tcp_clear_md5_list(sk);
>> kfree_rcu(rcu_dereference_protected(tp->md5sig_info, 1), rcu);
>> tp->md5sig_info = NULL;
>> }
>> #endif
>> + tcp_authopt_clear(sk);
>
> Do we really own the socket lock at this point ?
Not sure how I would tell but there is a lockdep_sock_is_held check
inside tcp_authopt_clear. I also added sock_owned_by_me and there were
no warnings.
Powered by blists - more mailing lists