netdev - Re: [PATCH net] net-timestamp: make sk_tskey more predictable in error path

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: <e1977c1d-9db8-4a4b-b871-4dfb0b78cfc6@linux.dev>
Date: Sun, 11 Feb 2024 18:46:11 -0500
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>,
 Vadim Fedorenko <vadfed@...a.com>, Andy Lutomirski <luto@...capital.net>
Cc: Jakub Kicinski <kuba@...nel.org>, "David S . Miller"
 <davem@...emloft.net>, Willem de Bruijn <willemb@...gle.com>,
 netdev@...r.kernel.org
Subject: Re: [PATCH net] net-timestamp: make sk_tskey more predictable in
 error path

On 11/02/2024 12:42, Willem de Bruijn wrote:
> Vadim Fedorenko wrote:
>> When SOF_TIMESTAMPING_OPT_ID is used to ambiguate timestamped datagrams,
>> the sk_tskey can become unpredictable in case of any error happened
>> during sendmsg(). Move increment later in the code and make decrement of
>> sk_tskey in error path. This solution is still racy in case of multiple
>> threads doing snedmsg() over the very same socket in parallel, but still
>> makes error path much more predictable.
>>
>> Fixes: 09c2d251b707 ("net-timestamp: add key to disambiguate concurrent datagrams")
>> Reported-by: Andy Lutomirski <luto@...capital.net>
>> Signed-off-by: Vadim Fedorenko <vadfed@...a.com>
>> ---
>>   net/ipv4/ip_output.c  | 14 +++++++++-----
>>   net/ipv6/ip6_output.c | 14 +++++++++-----
>>   2 files changed, 18 insertions(+), 10 deletions(-)
>>
>> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
>> index 41537d18eecf..ac4995ed17c7 100644
>> --- a/net/ipv4/ip_output.c
>> +++ b/net/ipv4/ip_output.c
>> @@ -974,7 +974,7 @@ static int __ip_append_data(struct sock *sk,
>>   	struct rtable *rt = (struct rtable *)cork->dst;
>>   	unsigned int wmem_alloc_delta = 0;
>>   	bool paged, extra_uref = false;
>> -	u32 tskey = 0;
>> +	u32 tsflags, tskey = 0;
>>   
>>   	skb = skb_peek_tail(queue);
>>   
>> @@ -982,10 +982,6 @@ static int __ip_append_data(struct sock *sk,
>>   	mtu = cork->gso_size ? IP_MAX_MTU : cork->fragsize;
>>   	paged = !!cork->gso_size;
>>   
>> -	if (cork->tx_flags & SKBTX_ANY_TSTAMP &&
>> -	    READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_OPT_ID)
>> -		tskey = atomic_inc_return(&sk->sk_tskey) - 1;
>> -
>>   	hh_len = LL_RESERVED_SPACE(rt->dst.dev);
>>   
>>   	fragheaderlen = sizeof(struct iphdr) + (opt ? opt->optlen : 0);
>> @@ -1052,6 +1048,11 @@ static int __ip_append_data(struct sock *sk,
>>   
>>   	cork->length += length;
>>   
>> +	tsflags = READ_ONCE(sk->sk_tsflags);
>> +	if (cork->tx_flags & SKBTX_ANY_TSTAMP &&
>> +	    tsflags & SOF_TIMESTAMPING_OPT_ID)
>> +		tskey = atomic_inc_return(&sk->sk_tskey) - 1;
>> +
>>   	/* So, what's going on in the loop below?
>>   	 *
>>   	 * We use calculated fragment length to generate chained skb,
>> @@ -1274,6 +1275,9 @@ static int __ip_append_data(struct sock *sk,
>>   	cork->length -= length;
>>   	IP_INC_STATS(sock_net(sk), IPSTATS_MIB_OUTDISCARDS);
>>   	refcount_add(wmem_alloc_delta, &sk->sk_wmem_alloc);
>> +	if (cork->tx_flags & SKBTX_ANY_TSTAMP &&
>> +	    tsflags & SOF_TIMESTAMPING_OPT_ID)
>> +		atomic_dec(&sk->sk_tskey);
> 
> Instead of testing the same conditional twice have a local bool,
> e.g., hold_tskey? Akin to extra_uarf for MSG_ZEROCOPY.
> 

Ok, sure, will post v2 soon