netdev - Re: [PATCH net] udp6: set dst cache for a connected sk before udp_v6_send

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:   Fri, 23 Mar 2018 20:13:54 +0300
From:   Alexey Kodanev <alexey.kodanev@...cle.com>
To:     Eric Dumazet <eric.dumazet@...il.com>, netdev@...r.kernel.org
Cc:     David Miller <davem@...emloft.net>
Subject: Re: [PATCH net] udp6: set dst cache for a connected sk before
 udp_v6_send_skb

On 03/23/2018 06:50 PM, Eric Dumazet wrote:
> 
> 
> On 03/23/2018 07:39 AM, Alexey Kodanev wrote:
>> After commit 33c162a980fe ("ipv6: datagram: Update dst cache of a
>> connected datagram sk during pmtu update"), when the error occurs on
>> sending datagram in udpv6_sendmsg() due to ICMPV6_PKT_TOOBIG type,
>> error handler can trigger the following path and call ip6_dst_store():
>>
>>     udpv6_err()
>>         ip6_sk_update_pmtu()
>>             ip6_datagram_dst_update()
>>                 ip6_dst_lookup_flow(), can create a RTF_CACHE clone
>>                 ...
>>                 ip6_dst_store()
>>
>> It can happen before a connected UDP socket invokes ip6_dst_store()
>> in the end of udpv6_sendmsg(), on destination release, as a result,
>> the last one changes dst to the old one, preventing getting updated
>> dst cache on the next udpv6_sendmsg() call.
>>
>> This patch moves ip6_dst_store() in udpv6_sendmsg(), so that it is
>> invoked after ip6_sk_dst_lookup_flow() and before udp_v6_send_skb().
>>
> 
> 
> A Fixes: tag would be nice, for automatic tools (and humans as well)

Hi Eric,

I see, will add the commit 33c162a980fe.

...
>>  
>> +	if (connected)
>> +		ip6_dst_store(sk, dst,
>> +			      ipv6_addr_equal(&fl6.daddr, &sk->sk_v6_daddr) ?
>> +			      &sk->sk_v6_daddr : NULL,
>> +#ifdef CONFIG_IPV6_SUBTREES
>> +			      ipv6_addr_equal(&fl6.saddr, &np->saddr) ?
>> +			      &np->saddr :
>> +#endif
>> +			      NULL);
>> +
> 
> What about the MSG_CONFIRM stuff ?
> 
>>  	if (msg->msg_flags&MSG_CONFIRM)
>>  		goto do_confirm;
>>  back_from_confirm:
> 
> Should not you move the above code here instead ?

Ah, you are right, it can release that dst if it go to "do_confirm".

> > Also ip6_dst_store() does not increment dst refcount.
> 
> I fear that as soon as dst is visible to other cpus, it might be stolen.
> 

So we should pass dst_clone(dst) to ip6_dst_store() instead of dst,
because udpv6_err() can release it if it's set the new one.

Then, I guess, we could left udpv6_sendmsg()/ip6_dst_store() where it
is now in the patch and remove the check for "connected" before dst_relase(),
similar to udp_sendmsg(), right?

Thanks,
Alexey