[PATCH v2 net 0/3] net: udp_tunnel: fix races and use-after-free

[PATCH v2 net 0/3] net: udp_tunnel: fix races and use-after-free

[Eric Dumazet]

Yue Sun reported a use-after-free and debugobjects warning in
udp_tunnel_nic_device_sync_work() when concurrently creating and
destroying netdevsim and geneve devices.

This series resolves the UAF and the underlying data races that
make the fix vulnerable.

The core issue is a workqueue re-queue race combined with data races
introduced by the lock-splitting in commit 1ead7501094c ("udp_tunnel:
remove rtnl_lock dependency"). That commit allowed the device reset
path (reset_ntf) to run without holding the RTNL lock (using only
utn->lock), while the port addition paths (add_port) still run under
RTNL without acquiring utn->lock.

This series fixes these issues in three steps:

1. Patch 1 (Jakub's fix) addresses the UAF by preventing double-queueing
   of the sync work. If work_pending is already set, we return early
   in device_sync(), blocking a second work item from entering the
   queue while the first is blocked on RTNL.

2. Patch 2 converts the state flags (need_sync, need_replay, work_pending)
   from bitfields to atomic bitops. Because these flags share a single
   byte, concurrent RMW writes from the RTNL-locked path and the RTNL-less
   reset path corrupt the byte. This corruption could clear work_pending,
   defeating the UAF fix.

3. Patch 3 fixes a similar data race on the 'missed' bitmap. Writes
   (__set_bit) happen under RTNL, while reads (should_replay) happen
   under utn->lock without RTNL. We convert this to use atomic set_bit(),
   READ_ONCE() for the fast-path read, and WRITE_ONCE() for clearing.

Reported-by: Yue Sun <samsun1006219@gmail.com>

Eric Dumazet (3):
  net: udp_tunnel: prevent double queueing in udp_tunnel_nic_device_sync
  net: udp_tunnel: convert state flags to atomic bitops
  net: udp_tunnel: use atomic bitops for missed bitmap

 net/ipv4/udp_tunnel_nic.c | 51 +++++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v2 net 1/3] net: udp_tunnel: prevent double queueing in udp_tunnel_nic_device_sync

[Eric Dumazet]

Yue Sun reported a use-after-free and debugobjects warning in
udp_tunnel_nic_device_sync_work() during concurrent device operations.

The workqueue core clears the internal pending bit before invoking the
worker. At that point, a concurrent thread can queue the work again.
When the already running worker eventually clears the work_pending flag
to 0, it mistakenly clears the flag for the newly queued instance.
udp_tunnel_nic_unregister() then observes work_pending as 0 and frees
the structure while the second work item is still active in the queue,
leading to UAF.

Fix this by returning early in udp_tunnel_nic_device_sync() if
work_pending is already set, preventing redundant work queueing.

Fixes: cc4e3835eff4 ("udp_tunnel: add central NIC RX port offload infrastructure")
Reported-by: Yue Sun <samsun1006219@gmail.com>
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/udp_tunnel_nic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/udp_tunnel_nic.c b/net/ipv4/udp_tunnel_nic.c
index 9944ed923ddfd10f9adf6ad788c0740daeaf2adb..3b32a0afa9798d3c416d9ae570e6d529f70e6697 100644
--- a/net/ipv4/udp_tunnel_nic.c
+++ b/net/ipv4/udp_tunnel_nic.c
@@ -301,7 +301,7 @@ __udp_tunnel_nic_device_sync(struct net_device *dev, struct udp_tunnel_nic *utn)
 static void
 udp_tunnel_nic_device_sync(struct net_device *dev, struct udp_tunnel_nic *utn)
 {
-	if (!utn->need_sync)
+	if (!utn->need_sync || utn->work_pending)
 		return;
 
 	queue_work(udp_tunnel_nic_workqueue, &utn->work);
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v2 net 2/3] net: udp_tunnel: convert state flags to atomic bitops

[Eric Dumazet]

The state flags of struct udp_tunnel_nic (need_sync, need_replay,
work_pending) are currently bitfields sharing a single byte.

These flags can be modified concurrently from different contexts:
- RTNL-locked paths (like add_port/del_port) write to need_sync and
  work_pending.
- The RTNL-less reset path (reset_ntf, used by netdevsim) writes to
  need_sync and need_replay under utn->lock.

Since they share a byte, concurrent writes are compiled into non-atomic
Read-Modify-Write (RMW) operations that can corrupt each other. For
example, a write to need_replay in reset_ntf can overwrite and clear
work_pending, defeating the double-queueing prevention and causing UAF.

Fix this by converting these state flags to atomic bitops, ensuring
safe concurrent writes across RTNL-locked and RTNL-less paths.

Fixes: 1ead7501094c ("udp_tunnel: remove rtnl_lock dependency")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/udp_tunnel_nic.c | 43 ++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/net/ipv4/udp_tunnel_nic.c b/net/ipv4/udp_tunnel_nic.c
index 3b32a0afa9798d3c416d9ae570e6d529f70e6697..840be5d79fc0ac3142049dcb9f1105a5844da9ae 100644
--- a/net/ipv4/udp_tunnel_nic.c
+++ b/net/ipv4/udp_tunnel_nic.c
@@ -30,9 +30,7 @@ struct udp_tunnel_nic_table_entry {
  * @work:	async work for talking to hardware from process context
  * @dev:	netdev pointer
  * @lock:	protects all fields
- * @need_sync:	at least one port start changed
- * @need_replay: space was freed, we need a replay of all ports
- * @work_pending: @work is currently scheduled
+ * @flags:	sync, replay, pending flags
  * @n_tables:	number of tables under @entries
  * @missed:	bitmap of tables which overflown
  * @entries:	table of tables of ports currently offloaded
@@ -44,9 +42,10 @@ struct udp_tunnel_nic {
 
 	struct mutex lock;
 
-	u8 need_sync:1;
-	u8 need_replay:1;
-	u8 work_pending:1;
+	unsigned long flags;
+#define UDP_TUNNEL_NIC_NEED_SYNC	0
+#define UDP_TUNNEL_NIC_NEED_REPLAY	1
+#define UDP_TUNNEL_NIC_WORK_PENDING	2
 
 	unsigned int n_tables;
 	unsigned long missed;
@@ -116,7 +115,7 @@ udp_tunnel_nic_entry_queue(struct udp_tunnel_nic *utn,
 			   unsigned int flag)
 {
 	entry->flags |= flag;
-	utn->need_sync = 1;
+	set_bit(UDP_TUNNEL_NIC_NEED_SYNC, &utn->flags);
 }
 
 static void
@@ -283,7 +282,7 @@ udp_tunnel_nic_device_sync_by_table(struct net_device *dev,
 static void
 __udp_tunnel_nic_device_sync(struct net_device *dev, struct udp_tunnel_nic *utn)
 {
-	if (!utn->need_sync)
+	if (!test_bit(UDP_TUNNEL_NIC_NEED_SYNC, &utn->flags))
 		return;
 
 	if (dev->udp_tunnel_nic_info->sync_table)
@@ -291,21 +290,27 @@ __udp_tunnel_nic_device_sync(struct net_device *dev, struct udp_tunnel_nic *utn)
 	else
 		udp_tunnel_nic_device_sync_by_port(dev, utn);
 
-	utn->need_sync = 0;
+	clear_bit(UDP_TUNNEL_NIC_NEED_SYNC, &utn->flags);
 	/* Can't replay directly here, in case we come from the tunnel driver's
 	 * notification - trying to replay may deadlock inside tunnel driver.
 	 */
-	utn->need_replay = udp_tunnel_nic_should_replay(dev, utn);
+	if (udp_tunnel_nic_should_replay(dev, utn))
+		set_bit(UDP_TUNNEL_NIC_NEED_REPLAY, &utn->flags);
+	else
+		clear_bit(UDP_TUNNEL_NIC_NEED_REPLAY, &utn->flags);
 }
 
 static void
 udp_tunnel_nic_device_sync(struct net_device *dev, struct udp_tunnel_nic *utn)
 {
-	if (!utn->need_sync || utn->work_pending)
+	if (!test_bit(UDP_TUNNEL_NIC_NEED_SYNC, &utn->flags))
+		return;
+
+	if (test_bit(UDP_TUNNEL_NIC_WORK_PENDING, &utn->flags))
 		return;
 
 	queue_work(udp_tunnel_nic_workqueue, &utn->work);
-	utn->work_pending = 1;
+	set_bit(UDP_TUNNEL_NIC_WORK_PENDING, &utn->flags);
 }
 
 static bool
@@ -552,7 +557,7 @@ static void __udp_tunnel_nic_reset_ntf(struct net_device *dev)
 
 	mutex_lock(&utn->lock);
 
-	utn->need_sync = false;
+	clear_bit(UDP_TUNNEL_NIC_NEED_SYNC, &utn->flags);
 	for (i = 0; i < utn->n_tables; i++)
 		for (j = 0; j < info->tables[i].n_entries; j++) {
 			struct udp_tunnel_nic_table_entry *entry;
@@ -696,8 +701,8 @@ udp_tunnel_nic_flush(struct net_device *dev, struct udp_tunnel_nic *utn)
 	for (i = 0; i < utn->n_tables; i++)
 		memset(utn->entries[i], 0, array_size(info->tables[i].n_entries,
 						      sizeof(**utn->entries)));
-	WARN_ON(utn->need_sync);
-	utn->need_replay = 0;
+	WARN_ON(test_bit(UDP_TUNNEL_NIC_NEED_SYNC, &utn->flags));
+	clear_bit(UDP_TUNNEL_NIC_NEED_REPLAY, &utn->flags);
 }
 
 static void
@@ -714,7 +719,7 @@ udp_tunnel_nic_replay(struct net_device *dev, struct udp_tunnel_nic *utn)
 		for (j = 0; j < info->tables[i].n_entries; j++)
 			udp_tunnel_nic_entry_freeze_used(&utn->entries[i][j]);
 	utn->missed = 0;
-	utn->need_replay = 0;
+	clear_bit(UDP_TUNNEL_NIC_NEED_REPLAY, &utn->flags);
 
 	if (!info->shared) {
 		udp_tunnel_get_rx_info(dev);
@@ -736,10 +741,10 @@ static void udp_tunnel_nic_device_sync_work(struct work_struct *work)
 	rtnl_lock();
 	mutex_lock(&utn->lock);
 
-	utn->work_pending = 0;
+	clear_bit(UDP_TUNNEL_NIC_WORK_PENDING, &utn->flags);
 	__udp_tunnel_nic_device_sync(utn->dev, utn);
 
-	if (utn->need_replay)
+	if (test_bit(UDP_TUNNEL_NIC_NEED_REPLAY, &utn->flags))
 		udp_tunnel_nic_replay(utn->dev, utn);
 
 	mutex_unlock(&utn->lock);
@@ -904,7 +909,7 @@ udp_tunnel_nic_unregister(struct net_device *dev, struct udp_tunnel_nic *utn)
 	/* Wait for the work to be done using the state, netdev core will
 	 * retry unregister until we give up our reference on this device.
 	 */
-	if (utn->work_pending)
+	if (test_bit(UDP_TUNNEL_NIC_WORK_PENDING, &utn->flags))
 		return;
 
 	udp_tunnel_nic_free(utn);
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v2 net 3/3] net: udp_tunnel: use atomic bitops for missed bitmap

[Eric Dumazet]

The 'missed' bitmap in struct udp_tunnel_nic can be accessed
concurrently:
- Writes (__set_bit) happen in the port add path (add_port), which
  holds the RTNL lock.
- Reads (checking if missed is non-zero) happen in the reset path
  (reset_ntf) via __udp_tunnel_nic_device_sync(), which holds
  utn->lock but does not hold RTNL after the blamed commit.

This setup creates a data race between concurrent writes and reads
on different CPUs. Fix this by using atomic set_bit() for writes,
READ_ONCE() for the fast-path read, and WRITE_ONCE() for clearing
the bitmap.

Fixes: 1ead7501094c ("udp_tunnel: remove rtnl_lock dependency")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/udp_tunnel_nic.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/udp_tunnel_nic.c b/net/ipv4/udp_tunnel_nic.c
index 840be5d79fc0ac3142049dcb9f1105a5844da9ae..9a567a87635caaf76f5b88029a7f28a65c795efc 100644
--- a/net/ipv4/udp_tunnel_nic.c
+++ b/net/ipv4/udp_tunnel_nic.c
@@ -147,7 +147,7 @@ udp_tunnel_nic_should_replay(struct net_device *dev, struct udp_tunnel_nic *utn)
 	const struct udp_tunnel_nic_table_info *table;
 	unsigned int i, j;
 
-	if (!utn->missed)
+	if (!READ_ONCE(utn->missed))
 		return false;
 
 	for (i = 0; i < utn->n_tables; i++) {
@@ -353,7 +353,7 @@ udp_tunnel_nic_has_collision(struct net_device *dev, struct udp_tunnel_nic *utn,
 			if (!udp_tunnel_nic_entry_is_free(entry) &&
 			    entry->port == ti->port &&
 			    entry->type != ti->type) {
-				__set_bit(i, &utn->missed);
+				set_bit(i, &utn->missed);
 				return true;
 			}
 		}
@@ -488,7 +488,7 @@ udp_tunnel_nic_add_new(struct net_device *dev, struct udp_tunnel_nic *utn,
 		 * are no devices currently which have multiple tables accepting
 		 * the same tunnel type, and false positives are okay.
 		 */
-		__set_bit(i, &utn->missed);
+		set_bit(i, &utn->missed);
 	}
 
 	return false;
@@ -718,7 +718,7 @@ udp_tunnel_nic_replay(struct net_device *dev, struct udp_tunnel_nic *utn)
 	for (i = 0; i < utn->n_tables; i++)
 		for (j = 0; j < info->tables[i].n_entries; j++)
 			udp_tunnel_nic_entry_freeze_used(&utn->entries[i][j]);
-	utn->missed = 0;
+	WRITE_ONCE(utn->missed, 0);
 	clear_bit(UDP_TUNNEL_NIC_NEED_REPLAY, &utn->flags);
 
 	if (!info->shared) {
-- 
2.55.0.rc0.799.gd6f94ed593-goog