[PATCH] wifi: zd1211rw: Fix potential data race in zd_mac_tx_to_dev()

[PATCH] wifi: zd1211rw: Fix potential data race in zd_mac_tx_to_dev()

[Daniil Dulov]

There is a potential data race in zd_mac_tx_to_dev(). For example, it is
possible for filter_ack() to clear the ack_wait_queue right after
zd_mac_tx_to_dev() checks that queue has more than 50 elements, but before
it dequeues any skb. This results in skb_dequeue() returns NULL and the
pointer is dereferenced in zd_mac_tx_status().

In order to avoid potential data races and leading dereference of a NULL
pointer, acquire the queue lock before any work with the queue is done and
replace all skb_* calls with their lockless version.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 459c51ad6e1f ("zd1211rw: port to mac80211")
Signed-off-by: Daniil Dulov <d.dulov@aladdin.ru>
---
 drivers/net/wireless/zydas/zd1211rw/zd_mac.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/zydas/zd1211rw/zd_mac.c b/drivers/net/wireless/zydas/zd1211rw/zd_mac.c
index 9653dbaac3c0..e7e0d1b7b9ab 100644
--- a/drivers/net/wireless/zydas/zd1211rw/zd_mac.c
+++ b/drivers/net/wireless/zydas/zd1211rw/zd_mac.c
@@ -568,6 +568,7 @@ void zd_mac_tx_to_dev(struct sk_buff *skb, int error)
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
 	struct ieee80211_hw *hw = info->rate_driver_data[0];
 	struct zd_mac *mac = zd_hw_mac(hw);
+	unsigned long flags;
 
 	ieee80211_tx_info_clear_status(info);
 
@@ -581,13 +582,17 @@ void zd_mac_tx_to_dev(struct sk_buff *skb, int error)
 	} else {
 		struct sk_buff_head *q = &mac->ack_wait_queue;
 
-		skb_queue_tail(q, skb);
+		spin_lock_irqsave(&q->lock, flags);
+
+		__skb_queue_tail(q, skb);
 		while (skb_queue_len(q) > ZD_MAC_MAX_ACK_WAITERS) {
-			zd_mac_tx_status(hw, skb_dequeue(q),
+			zd_mac_tx_status(hw, __skb_dequeue(q),
 					 mac->ack_pending ? mac->ack_signal : 0,
 					 NULL);
 			mac->ack_pending = 0;
 		}
+
+		spin_unlock_irqrestore(&q->lock, flags);
 	}
 }
 
-- 
2.34.1

Re: [PATCH] wifi: zd1211rw: Fix potential data race in zd_mac_tx_to_dev()

[Johannes Berg]

Hi,

So ... I have no idea why you're CC'ing all kinds of people who never
had anything to do with this driver, or haven't worked on WiFi in like a
decade or so ... Please don't. Even I should've been CC'ed with a
different address, at most.

I also have no idea who's maintaining this driver now though, if anyone.
I have hardware if someone wants it ;-)

> In order to avoid potential data races and leading dereference of a NULL
> pointer, acquire the queue lock before any work with the queue is done and
> replace all skb_* calls with their lockless version.

You should explain why the locking changes are OK.

johannes

Re: [PATCH] wifi: zd1211rw: Fix potential data race in zd_mac_tx_to_dev()

[Daniil Dulov]

On Wed, 11 Jun 2025 11:35:18 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:

> Hi,
> 

Hello,

Thank you for the review and sorry for such a late response.

> So ... I have no idea why you're CC'ing all kinds of people who never
> had anything to do with this driver, or haven't worked on WiFi in
> like a decade or so ... Please don't. Even I should've been CC'ed
> with a different address, at most.
>
> I also have no idea who's maintaining this driver now though, if
> anyone. I have hardware if someone wants it ;-)
>

Also, sorry for the mess with the CC list. The driver is marked as
"Orphaned" now and this is the first time I am trying to send a patch
for an orphaned driver. MAINTAINERS file only gives the Wireless
mailing list and the rest of CC list was generated by get_maintainer.pl
script.

I could not find any relevant information on how to send patches for
orphaned drivers in kernel docs. 

Can you, please, give me some advice on a process of submitting patches
to orphaned drivers or, if there is any relevant documentation on the
topic, can you share a link?

> > In order to avoid potential data races and leading dereference of a
> > NULL pointer, acquire the queue lock before any work with the queue
> > is done and replace all skb_* calls with their lockless version.  
> 
> You should explain why the locking changes are OK.
> 

Ok, I'll improve the patch description, thank you!

> johannes

Kind regards,
Daniil Dulov