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

[Dmitry Safonov]

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