netdev - Re: [PATCH v11 net-next 11/23] net/tcp: Sign SYN-ACK segments with TCP-AO

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] [day] [month] [year] [list]

Message-ID: <ce39120d-e76a-7353-2e9c-1995de0827b0@arista.com>
Date: Tue, 12 Sep 2023 21:26:28 +0100
From: Dmitry Safonov <dima@...sta.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: David Ahern <dsahern@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Jakub Kicinski <kuba@...nel.org>, "David S. Miller" <davem@...emloft.net>,
 linux-kernel@...r.kernel.org, Andy Lutomirski <luto@...capital.net>,
 Ard Biesheuvel <ardb@...nel.org>, Bob Gilligan <gilligan@...sta.com>,
 Dan Carpenter <error27@...il.com>, David Laight <David.Laight@...lab.com>,
 Dmitry Safonov <0x7f454c46@...il.com>, Donald Cassidy <dcassidy@...hat.com>,
 Eric Biggers <ebiggers@...nel.org>, "Eric W. Biederman"
 <ebiederm@...ssion.com>, Francesco Ruggeri <fruggeri05@...il.com>,
 "Gaillardetz, Dominik" <dgaillar@...na.com>,
 Herbert Xu <herbert@...dor.apana.org.au>,
 Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
 Ivan Delalande <colona@...sta.com>, Leonard Crestez <cdleonard@...il.com>,
 "Nassiri, Mohammad" <mnassiri@...na.com>,
 Salam Noureddine <noureddine@...sta.com>,
 Simon Horman <simon.horman@...igine.com>,
 "Tetreault, Francois" <ftetreau@...na.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v11 net-next 11/23] net/tcp: Sign SYN-ACK segments with
 TCP-AO

On 9/12/23 17:47, Eric Dumazet wrote:
> On Mon, Sep 11, 2023 at 11:04 PM Dmitry Safonov <dima@...sta.com> wrote:
[..]
>> @@ -3777,16 +3787,43 @@ struct sk_buff *tcp_make_synack(const struct sock *sk, struct dst_entry *dst,
>>                         tcp_rsk(req)->snt_synack = tcp_skb_timestamp_us(skb);
>>         }
>>
>> -#ifdef CONFIG_TCP_MD5SIG
>> +#if defined(CONFIG_TCP_MD5SIG) || defined(CONFIG_TCP_AO)
>>         rcu_read_lock();
>> -       md5 = tcp_rsk(req)->af_specific->req_md5_lookup(sk, req_to_sk(req));
>>  #endif
>> +       if (tcp_rsk_used_ao(req)) {
>> +#ifdef CONFIG_TCP_AO
>> +               u8 maclen = tcp_rsk(req)->maclen;
>> +               u8 keyid = tcp_rsk(req)->ao_keyid;
>> +
>> +               ao_key = tcp_sk(sk)->af_specific->ao_lookup(sk, req_to_sk(req),
>> +                                                           keyid, -1);
>> +               /* If there is no matching key - avoid sending anything,
>> +                * especially usigned segments. It could try harder and lookup
>> +                * for another peer-matching key, but the peer has requested
>> +                * ao_keyid (RFC5925 RNextKeyID), so let's keep it simple here.
>> +                */
>> +               if (unlikely(!ao_key || tcp_ao_maclen(ao_key) != maclen)) {
>> +                       rcu_read_unlock();
>> +                       skb_dst_drop(skb);
> 
> This does look necessary ? kfree_skb(skb) should also skb_dst_drop(skb);

Yeah, it seems not necessary, will drop this.

> 
> 
>> +                       kfree_skb(skb);
>> +                       net_warn_ratelimited("TCP-AO: the keyid %u with maclen %u|%u from SYN packet is not present - not sending SYNACK\n",
>> +                                            keyid, maclen,
>> +                                            ao_key ? tcp_ao_maclen(ao_key) : 0);
> 
> dereferencing ao_key after rcu_read_unlock() is a bug.

Thanks for catching, will fix!

-- 
Dmitry