[PATCH] wifi: libertas: reject short monitor TX frames

[PATCH] wifi: libertas: reject short monitor TX frames

[Pengpeng Hou]

In monitor mode, lbs_hard_start_xmit() casts skb->data to a
radiotap TX header, skips that header, and then copies the 802.11
destination address from offset 4 in the remaining frame.  The
generic length check only rejects zero-length and oversized skbs, so
a short monitor frame can be read past the end of the skb data.

Require enough bytes for the radiotap TX header and the destination
address field before using the monitor-mode header layout.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/net/wireless/marvell/libertas/tx.c | 7 +++++++
 1 file changed, 7 insertions(+)

--- a/drivers/net/wireless/marvell/libertas/tx.c
+++ b/drivers/net/wireless/marvell/libertas/tx.c
@@ -117,6 +117,13 @@
 	if (priv->wdev->iftype == NL80211_IFTYPE_MONITOR) {
 		struct tx_radiotap_hdr *rtap_hdr = (void *)skb->data;
 
+		if (skb->len < sizeof(*rtap_hdr) + 4 + ETH_ALEN) {
+			lbs_deb_tx("tx err: short monitor frame %u\n", skb->len);
+			dev->stats.tx_dropped++;
+			dev->stats.tx_errors++;
+			goto free;
+		}
+
 		/* set txpd fields from the radiotap header */
 		txpd->tx_control = cpu_to_le32(convert_radiotap_rate_to_mv(rtap_hdr->rate));
 

Re: [PATCH] wifi: libertas: reject short monitor TX frames

[Takashi Iwai]

On Sat, 04 Jul 2026 03:11:40 +0200,
Pengpeng Hou wrote:
> 
> In monitor mode, lbs_hard_start_xmit() casts skb->data to a
> radiotap TX header, skips that header, and then copies the 802.11
> destination address from offset 4 in the remaining frame.  The
> generic length check only rejects zero-length and oversized skbs, so
> a short monitor frame can be read past the end of the skb data.
> 
> Require enough bytes for the radiotap TX header and the destination
> address field before using the monitor-mode header layout.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>  drivers/net/wireless/marvell/libertas/tx.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> --- a/drivers/net/wireless/marvell/libertas/tx.c
> +++ b/drivers/net/wireless/marvell/libertas/tx.c
> @@ -117,6 +117,13 @@
>  	if (priv->wdev->iftype == NL80211_IFTYPE_MONITOR) {
>  		struct tx_radiotap_hdr *rtap_hdr = (void *)skb->data;
>  
> +		if (skb->len < sizeof(*rtap_hdr) + 4 + ETH_ALEN) {
> +			lbs_deb_tx("tx err: short monitor frame %u\n", skb->len);
> +			dev->stats.tx_dropped++;
> +			dev->stats.tx_errors++;
> +			goto free;
> +		}
> +
>  		/* set txpd fields from the radiotap header */
>  		txpd->tx_control = cpu_to_le32(convert_radiotap_rate_to_mv(rtap_hdr->rate));
>  
> 
> 

Now this commit is included in the stable trees, and the review for
distro kernel indicated some bugs.

The place you jump with goto-free is outside the priv->driver_lock
spinlock, while jumping to free follows the spin_unlock_irqrestore():

```
 free:
		dev_kfree_skb_any(skb);
	}

 unlock:
	spin_unlock_irqrestore(&priv->driver_lock, flags);
	wake_up(&priv->waitq);

	return ret;
}
```

So this patch introduced an unbalanced spinlock.

Moreover, at that point, priv->tx_pending_len is set to -1.  Then
netif queues won't be woken up because priv->tx_pending_len isn't
restored -- which may lead to permanently blocking transmission.

I guess this fix needs to be revisited or reverted.


thanks,

Takashi