netdev - Re: [PATCH v9 net-next 16/23] net/tcp: Ignore specific ICMPs for TCP-AO connections

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: Thu, 10 Aug 2023 17:26:52 +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>,
 Salam Noureddine <noureddine@...sta.com>,
 "Tetreault, Francois" <ftetreau@...na.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v9 net-next 16/23] net/tcp: Ignore specific ICMPs for
 TCP-AO connections

On 8/8/23 14:43, Eric Dumazet wrote:
> On Wed, Aug 2, 2023 at 7:27 PM Dmitry Safonov <dima@...sta.com> wrote:
[..]
>>
>> +bool tcp_ao_ignore_icmp(struct sock *sk, int type, int code)
> 
> const struct sock *sk ?

Well, I can't really: atomic64_inc(&ao->counters.dropped_icmp)

>> +{
>> +       bool ignore_icmp = false;
>> +       struct tcp_ao_info *ao;
>> +
>> +       /* RFC5925, 7.8:
>> +        * >> A TCP-AO implementation MUST default to ignore incoming ICMPv4
>> +        * messages of Type 3 (destination unreachable), Codes 2-4 (protocol
>> +        * unreachable, port unreachable, and fragmentation needed -- ’hard
>> +        * errors’), and ICMPv6 Type 1 (destination unreachable), Code 1
>> +        * (administratively prohibited) and Code 4 (port unreachable) intended
>> +        * for connections in synchronized states (ESTABLISHED, FIN-WAIT-1, FIN-
>> +        * WAIT-2, CLOSE-WAIT, CLOSING, LAST-ACK, TIME-WAIT) that match MKTs.
>> +        */
> 
> I know this sounds silly, but you should read sk->sk_family once.
> 
> Or risk another KCSAN report with IPV6_ADDRFORM
> 
> if (sk->sk_family == AF_INET) {
>     ...
> } else {
>     /* AF_INET case */
> }

Oh, I didn't know about IPV6_ADDRFORM. Sure, will read it once.

>> +       if (sk->sk_family == AF_INET) {
>> +               if (type != ICMP_DEST_UNREACH)
>> +                       return false;
>> +               if (code < ICMP_PROT_UNREACH || code > ICMP_FRAG_NEEDED)
>> +                       return false;
>> +       } else if (sk->sk_family == AF_INET6) {
>> +               if (type != ICMPV6_DEST_UNREACH)
>> +                       return false;
>> +               if (code != ICMPV6_ADM_PROHIBITED && code != ICMPV6_PORT_UNREACH)
>> +                       return false;
>> +       } else {
> 
> 
> No WARN_ON_ONCE(1) here please.

Ok.

[..]
>> +++ b/net/ipv4/tcp_ipv4.c
>> @@ -494,6 +494,8 @@ int tcp_v4_err(struct sk_buff *skb, u32 info)
>>                 return -ENOENT;
>>         }
>>         if (sk->sk_state == TCP_TIME_WAIT) {
>> +               /* To increase the counter of ignored icmps for TCP-AO */
>> +               tcp_ao_ignore_icmp(sk, type, code);
>>                 inet_twsk_put(inet_twsk(sk));
>>                 return 0;
>>         }
>> @@ -508,6 +510,9 @@ int tcp_v4_err(struct sk_buff *skb, u32 info)
>>         }
>>
>>         bh_lock_sock(sk);
> 
> Do we need to hold the spinlock before calling tcp_ao_ignore_icmp() ?

I don't think so. And I think originally I've written it out of
bh_lock_sock(), but now I can't remember which paranoid thought resulted
in moving it under the lock. Anyway, will move it out again.

>> +       if (tcp_ao_ignore_icmp(sk, type, code))
>> +               goto out;
>> +
>>         /* If too many ICMPs get dropped on busy
>>          * servers this needs to be solved differently.
>>          * We do take care of PMTU discovery (RFC1191) special case :
[..]

Thanks,
           Dmitry