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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <65c90702da50f_178c3c294a3@willemb.c.googlers.com.notmuch>
Date: Sun, 11 Feb 2024 12:42:26 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Vadim Fedorenko <vadfed@...a.com>, 
 Vadim Fedorenko <vadim.fedorenko@...ux.dev>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 Andy Lutomirski <luto@...capital.net>
Cc: Vadim Fedorenko <vadfed@...a.com>, 
 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

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.