netdev - Re: [PATCH net] udp: fix data-race in udp_set_dev

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]

Message-ID: <5ef3ce11785c58bc93ff7809cc1b35dfb354974f.camel@redhat.com>
Date:   Fri, 25 Oct 2019 10:00:16 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Eric Dumazet <edumazet@...gle.com>,
        "David S . Miller" <davem@...emloft.net>
Cc:     netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>,
        syzbot <syzkaller@...glegroups.com>
Subject: Re: [PATCH net] udp: fix data-race in udp_set_dev_scratch()

On Thu, 2019-10-24 at 11:43 -0700, Eric Dumazet wrote:
> KCSAN reported a data-race in udp_set_dev_scratch() [1]
> 
> The issue here is that we must not write over skb fields
> if skb is shared. A similar issue has been fixed in commit
> 89c22d8c3b27 ("net: Fix skb csum races when peeking")
> 
> While we are at it, use a helper only dealing with
> udp_skb_scratch(skb)->csum_unnecessary, as this allows
> udp_set_dev_scratch() to be called once and thus inlined.
> 
> [1]
> BUG: KCSAN: data-race in udp_set_dev_scratch / udpv6_recvmsg
> 
> write to 0xffff888120278317 of 1 bytes by task 10411 on cpu 1:
>  udp_set_dev_scratch+0xea/0x200 net/ipv4/udp.c:1308
>  __first_packet_length+0x147/0x420 net/ipv4/udp.c:1556
>  first_packet_length+0x68/0x2a0 net/ipv4/udp.c:1579
>  udp_poll+0xea/0x110 net/ipv4/udp.c:2720
>  sock_poll+0xed/0x250 net/socket.c:1256
>  vfs_poll include/linux/poll.h:90 [inline]
>  do_select+0x7d0/0x1020 fs/select.c:534
>  core_sys_select+0x381/0x550 fs/select.c:677
>  do_pselect.constprop.0+0x11d/0x160 fs/select.c:759
>  __do_sys_pselect6 fs/select.c:784 [inline]
>  __se_sys_pselect6 fs/select.c:769 [inline]
>  __x64_sys_pselect6+0x12e/0x170 fs/select.c:769
>  do_syscall_64+0xcc/0x370 arch/x86/entry/common.c:290
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> read to 0xffff888120278317 of 1 bytes by task 10413 on cpu 0:
>  udp_skb_csum_unnecessary include/net/udp.h:358 [inline]
>  udpv6_recvmsg+0x43e/0xe90 net/ipv6/udp.c:310
>  inet6_recvmsg+0xbb/0x240 net/ipv6/af_inet6.c:592
>  sock_recvmsg_nosec+0x5c/0x70 net/socket.c:871
>  ___sys_recvmsg+0x1a0/0x3e0 net/socket.c:2480
>  do_recvmmsg+0x19a/0x5c0 net/socket.c:2601
>  __sys_recvmmsg+0x1ef/0x200 net/socket.c:2680
>  __do_sys_recvmmsg net/socket.c:2703 [inline]
>  __se_sys_recvmmsg net/socket.c:2696 [inline]
>  __x64_sys_recvmmsg+0x89/0xb0 net/socket.c:2696
>  do_syscall_64+0xcc/0x370 arch/x86/entry/common.c:290
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> Reported by Kernel Concurrency Sanitizer on:
> CPU: 0 PID: 10413 Comm: syz-executor.0 Not tainted 5.4.0-rc3+ #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> 
> Fixes: 2276f58ac589 ("udp: use a separate rx queue for packet reception")
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Reported-by: syzbot <syzkaller@...glegroups.com>
> Cc: Paolo Abeni <pabeni@...hat.com>
> ---
>  net/ipv4/udp.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 345a3d43f5a655e009e99c16bb19e047cdf003c6..d1ed160af202c054839387201abd3f13b55d00e9 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1316,6 +1316,20 @@ static void udp_set_dev_scratch(struct sk_buff *skb)
>  		scratch->_tsize_state |= UDP_SKB_IS_STATELESS;
>  }
>  
> +static void udp_skb_csum_unnecessary_set(struct sk_buff *skb)
> +{
> +	/* We come here after udp_lib_checksum_complete() returned 0.
> +	 * This means that __skb_checksum_complete() might have
> +	 * set skb->csum_valid to 1.
> +	 * On 64bit platforms, we can set csum_unnecessary
> +	 * to true, but only if the skb is not shared.
> +	 */
> +#if BITS_PER_LONG == 64
> +	if (!skb_shared(skb))
> +		udp_skb_scratch(skb)->csum_unnecessary = true;
> +#endif
> +}
> +
>  static int udp_skb_truesize(struct sk_buff *skb)
>  {
>  	return udp_skb_scratch(skb)->_tsize_state & ~UDP_SKB_IS_STATELESS;
> @@ -1550,10 +1564,7 @@ static struct sk_buff *__first_packet_length(struct sock *sk,
>  			*total += skb->truesize;
>  			kfree_skb(skb);
>  		} else {
> -			/* the csum related bits could be changed, refresh
> -			 * the scratch area
> -			 */
> -			udp_set_dev_scratch(skb);
> +			udp_skb_csum_unnecessary_set(skb);
>  			break;
>  		}
>  	}

LGTM, Thanks Eric!

Reviewed-by: Paolo Abeni <pabeni@...hat.com>