All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kuniyuki Iwashima <kuniyu@amazon.com>
To: "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>
Cc: Kuniyuki Iwashima <kuniyu@amazon.com>,
	Kuniyuki Iwashima <kuni1840@gmail.com>, <netdev@vger.kernel.org>,
	syzbot <syzkaller@googlegroups.com>
Subject: [PATCH v2 net] net: Fix sk->sk_stamp race in sock_recv_cmsgs().
Date: Mon, 8 May 2023 09:58:15 -0700	[thread overview]
Message-ID: <20230508165815.45602-1-kuniyu@amazon.com> (raw)

KCSAN found a data race in sock_recv_cmsgs() [0] where the read access
to sk->sk_stamp needs READ_ONCE().

Also, there is another race like below.  If the torn load of the high
32-bits precedes WRITE_ONCE(sk, skb->tstamp) and later the written
lower 32-bits happens to match with SK_DEFAULT_STAMP, the final result
of sk->sk_stamp could be 0.

  sock_recv_cmsgs()  ioctl(SIOCGSTAMP)      sock_recv_cmsgs()
  |                  |                      |
  |- if (sock_flag(sk, SOCK_TIMESTAMP))     |
  |                  |                      |
  |                  `- sock_set_flag(sk, SOCK_TIMESTAMP)
  |                                         |
  |                                          `- if (sock_flag(sk, SOCK_TIMESTAMP))
  `- if (sk->sk_stamp == SK_DEFAULT_STAMP)      `- sock_write_timestamp(sk, skb->tstamp)
      `- sock_write_timestamp(sk, 0)

Even with READ_ONCE(), we could get the same result if READ_ONCE() precedes
WRITE_ONCE() because the SK_DEFAULT_STAMP check and WRITE_ONCE(sk_stamp, 0)
are not atomic.

Let's avoid the race by cmpxchg() on 64-bits architecture or seqlock on
32-bits machines.

[0]:
BUG: KCSAN: data-race in packet_recvmsg / packet_recvmsg

write (marked) to 0xffff88803c81f258 of 8 bytes by task 19171 on cpu 0:
 sock_write_timestamp include/net/sock.h:2670 [inline]
 sock_recv_cmsgs include/net/sock.h:2722 [inline]
 packet_recvmsg+0xb97/0xd00 net/packet/af_packet.c:3489
 sock_recvmsg_nosec net/socket.c:1019 [inline]
 sock_recvmsg+0x11a/0x130 net/socket.c:1040
 sock_read_iter+0x176/0x220 net/socket.c:1118
 call_read_iter include/linux/fs.h:1845 [inline]
 new_sync_read fs/read_write.c:389 [inline]
 vfs_read+0x5e0/0x630 fs/read_write.c:470
 ksys_read+0x163/0x1a0 fs/read_write.c:613
 __do_sys_read fs/read_write.c:623 [inline]
 __se_sys_read fs/read_write.c:621 [inline]
 __x64_sys_read+0x41/0x50 fs/read_write.c:621
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x3b/0x90 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x72/0xdc

read to 0xffff88803c81f258 of 8 bytes by task 19183 on cpu 1:
 sock_recv_cmsgs include/net/sock.h:2721 [inline]
 packet_recvmsg+0xb64/0xd00 net/packet/af_packet.c:3489
 sock_recvmsg_nosec net/socket.c:1019 [inline]
 sock_recvmsg+0x11a/0x130 net/socket.c:1040
 sock_read_iter+0x176/0x220 net/socket.c:1118
 call_read_iter include/linux/fs.h:1845 [inline]
 new_sync_read fs/read_write.c:389 [inline]
 vfs_read+0x5e0/0x630 fs/read_write.c:470
 ksys_read+0x163/0x1a0 fs/read_write.c:613
 __do_sys_read fs/read_write.c:623 [inline]
 __se_sys_read fs/read_write.c:621 [inline]
 __x64_sys_read+0x41/0x50 fs/read_write.c:621
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x3b/0x90 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x72/0xdc

value changed: 0xffffffffc4653600 -> 0x0000000000000000

Reported by Kernel Concurrency Sanitizer on:
CPU: 1 PID: 19183 Comm: syz-executor.5 Not tainted 6.3.0-rc7-02330-gca6270c12e20 #2
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014

Fixes: 6c7c98bad488 ("sock: avoid dirtying sk_stamp, if possible")
Reported-by: syzbot <syzkaller@googlegroups.com>
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
v2:
  * Add Fixes tag

v1: https://lore.kernel.org/netdev/20230506022325.99106-1-kuniyu@amazon.com/
---
 include/net/sock.h | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 8b7ed7167243..c2a8b799283e 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2671,6 +2671,20 @@ static inline void sock_write_timestamp(struct sock *sk, ktime_t kt)
 #endif
 }
 
+#define SK_DEFAULT_STAMP (-1L * NSEC_PER_SEC)
+
+static inline void sock_zero_timestamp(struct sock *sk)
+{
+#if BITS_PER_LONG==32
+	write_seqlock(&sk->sk_stamp_seq);
+	if (sk->sk_stamp == SK_DEFAULT_STAMP)
+		sk->sk_stamp = 0;
+	write_sequnlock(&sk->sk_stamp_seq);
+#else
+	cmpxchg(&sk->sk_stamp, SK_DEFAULT_STAMP, 0);
+#endif
+}
+
 void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
 			   struct sk_buff *skb);
 void __sock_recv_wifi_status(struct msghdr *msg, struct sock *sk,
@@ -2704,7 +2718,6 @@ sock_recv_timestamp(struct msghdr *msg, struct sock *sk, struct sk_buff *skb)
 void __sock_recv_cmsgs(struct msghdr *msg, struct sock *sk,
 		       struct sk_buff *skb);
 
-#define SK_DEFAULT_STAMP (-1L * NSEC_PER_SEC)
 static inline void sock_recv_cmsgs(struct msghdr *msg, struct sock *sk,
 				   struct sk_buff *skb)
 {
@@ -2718,8 +2731,8 @@ static inline void sock_recv_cmsgs(struct msghdr *msg, struct sock *sk,
 		__sock_recv_cmsgs(msg, sk, skb);
 	else if (unlikely(sock_flag(sk, SOCK_TIMESTAMP)))
 		sock_write_timestamp(sk, skb->tstamp);
-	else if (unlikely(sk->sk_stamp == SK_DEFAULT_STAMP))
-		sock_write_timestamp(sk, 0);
+	else
+		sock_zero_timestamp(sk);
 }
 
 void __sock_tx_timestamp(__u16 tsflags, __u8 *tx_flags);
-- 
2.30.2


             reply	other threads:[~2023-05-08 16:58 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-08 16:58 Kuniyuki Iwashima [this message]
2023-05-08 17:08 ` [PATCH v2 net] net: Fix sk->sk_stamp race in sock_recv_cmsgs() Eric Dumazet
2023-05-08 17:20   ` Kuniyuki Iwashima
2023-05-08 17:31     ` Eric Dumazet
2023-05-08 17:39       ` Kuniyuki Iwashima
2023-05-09  7:38 ` kernel test robot
2023-05-09  8:12 ` kernel test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230508165815.45602-1-kuniyu@amazon.com \
    --to=kuniyu@amazon.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=kuba@kernel.org \
    --cc=kuni1840@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=syzkaller@googlegroups.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.