Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <ce39120d-e76a-7353-2e9c-1995de0827b0@arista.com>
Date:   Tue, 12 Sep 2023 21:26:28 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v11 net-next 11/23] net/tcp: Sign SYN-ACK segments with
 TCP-AO
Content-Language: en-US
To:     Eric Dumazet <edumazet@google.com>
Cc:     David Ahern <dsahern@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
        Jakub Kicinski <kuba@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        linux-kernel@vger.kernel.org,
        Andy Lutomirski <luto@amacapital.net>,
        Ard Biesheuvel <ardb@kernel.org>,
        Bob Gilligan <gilligan@arista.com>,
        Dan Carpenter <error27@gmail.com>,
        David Laight <David.Laight@aculab.com>,
        Dmitry Safonov <0x7f454c46@gmail.com>,
        Donald Cassidy <dcassidy@redhat.com>,
        Eric Biggers <ebiggers@kernel.org>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        Francesco Ruggeri <fruggeri05@gmail.com>,
        "Gaillardetz, Dominik" <dgaillar@ciena.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        Ivan Delalande <colona@arista.com>,
        Leonard Crestez <cdleonard@gmail.com>,
        "Nassiri, Mohammad" <mnassiri@ciena.com>,
        Salam Noureddine <noureddine@arista.com>,
        Simon Horman <simon.horman@corigine.com>,
        "Tetreault, Francois" <ftetreau@ciena.com>, netdev@vger.kernel.org
References: <20230911210346.301750-1-dima@arista.com>
 <20230911210346.301750-12-dima@arista.com>
 <CANn89i+gs2oNDWL-w+ZUfNAYZ3GXf6RqN3wyr+4YqiQm7jq+1w@mail.gmail.com>
From:   Dmitry Safonov <dima@arista.com>
In-Reply-To: <CANn89i+gs2oNDWL-w+ZUfNAYZ3GXf6RqN3wyr+4YqiQm7jq+1w@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 9/12/23 17:47, Eric Dumazet wrote:
> On Mon, Sep 11, 2023 at 11:04 PM Dmitry Safonov <dima@arista.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