Skip to contentSkip to conversation

wireguard: mark racy persistent_keepalive_interval accesses

PATCH Applied Nguyen Ngoc Thang 2 participants 2 messages Oct 11 – Nov 10

Subsystems (3)
Networking Network drivers Wireguard secure network tunnel
lore
Share & tools
lore

Nguyen Ngoc Thang

author
#0 lore raw

PATCH 1/1 wireguard: mark racy persistent_keepalive_interval accesses

wg_set_device_doit() updates peer->persistent_keepalive_interval under device_update_lock, while the RX/TX paths read it locklessly in wg_timers_any_authenticated_packet_traversal() and in the persistent keepalive timer. KCSAN reports the race:

  BUG: KCSAN: data-race in set_peer / wg_timers_any_authenticated_packet_traversal

write to ... by task 3648 on cpu 1:
set_peer+0x... drivers/net/wireguard/netlink.c:457 wg_set_device_doit+0x...
read to ... by task 3652 on cpu 0:
wg_timers_any_authenticated_packet_traversal+0x... timers.c:217 wg_packet_consume_data_done+0x... wg_packet_rx_poll+0x...

The race is benign, but the reader also loads the field twice, so the zero check and the timer expiry computation can see different values. Load it once with READ_ONCE() and mark the writer and the other lockless reader accordingly.

Reported-by [email protected]
Closes https://syzkaller.appspot.com/bug?extid=de9701a2cad597c14d34
Fixes e7096c131e51 ("net: WireGuard secure network tunnel")
Signed-off-by Nguyen Ngoc Thang <[email protected]>
---
 drivers/net/wireguard/netlink.c | 3 ++-
 drivers/net/wireguard/timers.c  | 8 +++++---
 2 files changed, 7 insertions(+), 4 deletions(-)
drivers/net/wireguard/netlink.c +2 -1
index 1da7e98d0d50..fafe2900c92b 100644
@@ -454,7 +454,8 @@ static int set_peer(struct wg_device *wg, struct nlattr **attrs)
 			persistent_keepalive_interval &&
 			netif_running(wg->dev);
 
-		peer->persistent_keepalive_interval = persistent_keepalive_interval;
+		WRITE_ONCE(peer->persistent_keepalive_interval,
+			   persistent_keepalive_interval);
 		if (send_keepalive)
 			wg_packet_send_keepalive(peer);
 	}
drivers/net/wireguard/timers.c +5 -3
index 4016a3065602..c4697e8522b3 100644
@@ -140,7 +140,7 @@ static void wg_expired_send_persistent_keepalive(struct timer_list *timer)
 	struct wg_peer *peer = timer_container_of(peer, timer,
 						  timer_persistent_keepalive);
 
-	if (likely(peer->persistent_keepalive_interval))
+	if (likely(READ_ONCE(peer->persistent_keepalive_interval)))
 		wg_packet_send_keepalive(peer);
 }
 

@@ -214,9 +214,11 @@ void wg_timers_session_derived(struct wg_peer *peer)
  */
 void wg_timers_any_authenticated_packet_traversal(struct wg_peer *peer)
 {
-	if (peer->persistent_keepalive_interval)
+	u16 interval = READ_ONCE(peer->persistent_keepalive_interval);
+
+	if (interval)
 		mod_peer_timer(peer, &peer->timer_persistent_keepalive,
-			jiffies + peer->persistent_keepalive_interval * HZ);
+			jiffies + interval * HZ);
 }
 
 void wg_timers_init(struct wg_peer *peer)
Signature · 1 line
2.43.0
maintainer Applied
#1 lore raw
Quoted 15 lines · Nguyen Ngoc Thang

wg_set_device_doit() updates peer->persistent_keepalive_interval under device_update_lock, while the RX/TX paths read it locklessly in wg_timers_any_authenticated_packet_traversal() and in the persistent keepalive timer. KCSAN reports the race:

  BUG: KCSAN: data-race in set_peer / wg_timers_any_authenticated_packet_traversal

write to ... by task 3648 on cpu 1:

   set_peer+0x...  drivers/net/wireguard/netlink.c:457
   wg_set_device_doit+0x...
  read to ... by task 3652 on cpu 0:
   wg_timers_any_authenticated_packet_traversal+0x...  timers.c:217
   wg_packet_consume_data_done+0x...
   wg_packet_rx_poll+0x...
Quoted 4 lines

The race is benign, but the reader also loads the field twice, so the zero check and the timer expiry computation can see different values. Load it once with READ_ONCE() and mark the writer and the other lockless reader accordingly.

Applied, thank you!