[PATCH] wifi: ar5523: fix integer underflow in ar5523_data_rx_cb()

[PATCH] wifi: ar5523: fix integer underflow in ar5523_data_rx_cb()

[pip-izony]

From: Seungjin Bae <eeodqql09@gmail.com>

In ar5523_data_rx_cb(), the `rxlen` variable is derived from desc->len,
which is provided by the USB device.

The function checks for an upper bound (rxlen > ar->rxbufsz) and for a
zero value (!rxlen), but it fails to check for a proper lower bound
against the size of the descriptor.

If a malicious device provides an `rxlen` value that is positive
but smaller than sizeof(struct ar5523_rx_desc), the subtraction in
the call to skb_put() will result in an integer underflow.

This passes a very large unsigned value to skb_put(), which then
triggers a kernel panic upon detecting the potential buffer overflow.

Fix this by adding a check to ensure `rxlen` is at least
sizeof(struct ar5523_rx_desc) before performing the substraction.

Fixes: b7d572e1871df ("ar5523: Add new driver")
Signed-off-by: Seungjin Bae <eeodqql09@gmail.com>
---
 drivers/net/wireless/ath/ar5523/ar5523.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/wireless/ath/ar5523/ar5523.c b/drivers/net/wireless/ath/ar5523/ar5523.c
index 1230e6278f23..dfaccf241560 100644
--- a/drivers/net/wireless/ath/ar5523/ar5523.c
+++ b/drivers/net/wireless/ath/ar5523/ar5523.c
@@ -589,6 +589,12 @@ static void ar5523_data_rx_cb(struct urb *urb)
 		goto skip;
 	}
 
+	if (rxlen < sizeof(struct ar5523_rx_desc)) {
+		ar5523_dbg(ar, "RX: Bad descriptor (len=%d is too small)\n",
+			   rxlen);
+		goto skip;
+	}
+
 	skb_reserve(data->skb, sizeof(*chunk));
 	skb_put(data->skb, rxlen - sizeof(struct ar5523_rx_desc));
 
-- 
2.43.0

Re: [PATCH] wifi: ar5523: fix integer underflow in ar5523_data_rx_cb()

[Jeff Johnson]

On 11/3/2025 11:55 AM, pip-izony wrote:
> From: Seungjin Bae <eeodqql09@gmail.com>
> 
> In ar5523_data_rx_cb(), the `rxlen` variable is derived from desc->len,
> which is provided by the USB device.
> 
> The function checks for an upper bound (rxlen > ar->rxbufsz) and for a
> zero value (!rxlen), but it fails to check for a proper lower bound
> against the size of the descriptor.
> 
> If a malicious device provides an `rxlen` value that is positive
> but smaller than sizeof(struct ar5523_rx_desc), the subtraction in
> the call to skb_put() will result in an integer underflow.
> 
> This passes a very large unsigned value to skb_put(), which then
> triggers a kernel panic upon detecting the potential buffer overflow.
> 
> Fix this by adding a check to ensure `rxlen` is at least
> sizeof(struct ar5523_rx_desc) before performing the substraction.
> 
> Fixes: b7d572e1871df ("ar5523: Add new driver")
> Signed-off-by: Seungjin Bae <eeodqql09@gmail.com>
> ---
>  drivers/net/wireless/ath/ar5523/ar5523.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ar5523/ar5523.c b/drivers/net/wireless/ath/ar5523/ar5523.c
> index 1230e6278f23..dfaccf241560 100644
> --- a/drivers/net/wireless/ath/ar5523/ar5523.c
> +++ b/drivers/net/wireless/ath/ar5523/ar5523.c
> @@ -589,6 +589,12 @@ static void ar5523_data_rx_cb(struct urb *urb)
>  		goto skip;
>  	}
>  
> +	if (rxlen < sizeof(struct ar5523_rx_desc)) {
> +		ar5523_dbg(ar, "RX: Bad descriptor (len=%d is too small)\n",
> +			   rxlen);
> +		goto skip;
> +	}
> +
>  	skb_reserve(data->skb, sizeof(*chunk));
>  	skb_put(data->skb, rxlen - sizeof(struct ar5523_rx_desc));
>  

Since this is an orphaned driver, this patch has been sitting in my patchwork
queue for a very long time. Today I decided to take a look.

The patch does address the issue mentioned, but really doesn't completely
address the crux of the issue. I believe there should be a fixed correlation
between the rxlen and the usblen. I posed the following question to my review
agent:

given that the usblen should account for the ar5523_chunk header, the payload,
and the ar5523_rx_desc, shouldn't the rxlen always be the same as the usblen -
sizeof(ar5523_chunk)?

And my agent agreed:

Yes, that's exactly right. Looking at the struct comment "msg length including
header" for desc->len and the frame layout:

  [ar5523_chunk (4B)] [802.11 payload] [ar5523_rx_desc]
   ←──────────────────── usblen ──────────────────────→

  The invariant should be:

  rxlen == usblen - sizeof(ar5523_chunk)

  Because rxlen covers [802.11 payload] + [ar5523_rx_desc], and usblen covers
everything including the chunk header.

  The code never cross-validates them. It independently bounds-checks usblen
(via AR5523_MIN_RXBUFSZ) and rxlen (upper bound vs rxbufsz, lower bound added
by the patch), but if a malicious device provides rxlen > usblen -
sizeof(ar5523_chunk), then:

  skb_put(data->skb, rxlen - sizeof(struct ar5523_rx_desc));

  ...would expose bytes beyond the actual 802.11 payload — potentially biting
into the ar5523_rx_desc trailer bytes, or uninitialized skb tail space up to
rxbufsz, and pass that garbage to ieee80211_rx_irqsafe().

  The existing upper bound check rxlen > ar->rxbufsz prevents a skb_put
overflow (since the skb is allocated with rxbufsz), but it's far too loose.
The right check would be:

  if (rxlen != usblen - sizeof(struct ar5523_chunk))
      goto skip;

  Or at minimum a strict upper bound:

  if (rxlen > usblen - sizeof(struct ar5523_chunk))
      goto skip;

  So the patch fixes the underflow but misses this inconsistency between rxlen
and usblen that could allow overreading valid-but-wrong data within the skb
buffer.

So please consider this analysis and propose a v2

/jeff

[PATCH v2] wifi: ar5523: validate rxlen against descriptor size and usblen in ar5523_data_rx_cb()

[pip-izony]

From: Seungjin Bae <eeodqql09@gmail.com>

In ar5523_data_rx_cb(), the `rxlen` variable is derived from desc->len,
which is provided by the USB device.

The function checks for an upper bound (rxlen > ar->rxbufsz) and for a
zero value (!rxlen), but it fails to check for a proper lower bound
against the size of the descriptor.

If a malicious device provides an `rxlen` value that is positive
but smaller than sizeof(struct ar5523_rx_desc), the subtraction in
the call to skb_put() will result in an integer underflow.

This passes a very large unsigned value to skb_put(), which then
triggers a kernel panic upon detecting the potential buffer overflow.

Fix this by adding a check to ensure `rxlen` is at least
sizeof(struct ar5523_rx_desc) before performing the subtraction.

Additionally, `rxlen` is not cross-checked against usblen (the number
of bytes actually received). Since the frame layout is
[ar5523_chunk][802.11 payload][ar5523_rx_desc] and `usblen` covers the
whole buffer, a valid rxlen cannot exceed `usblen - sizeof(chunk)`. A
device reporting a larger `rxlen` would cause skb_put() to expose
descriptor trailer bytes or uninitialized skb tail data as 802.11
payload to ieee80211_rx_irqsafe().

Add a check to reject such frames.

Fixes: b7d572e1871df ("ar5523: Add new driver")
Suggested-by: Jeff Johnson <jeff.johnson@oss.qualcomm.com>
Signed-off-by: Seungjin Bae <eeodqql09@gmail.com>
---
 v1 -> v2: cross-check rxlen against usblen to prevent overreading past the received payload
 
 drivers/net/wireless/ath/ar5523/ar5523.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/wireless/ath/ar5523/ar5523.c b/drivers/net/wireless/ath/ar5523/ar5523.c
index 66ac396858a5..3ee4b020f416 100644
--- a/drivers/net/wireless/ath/ar5523/ar5523.c
+++ b/drivers/net/wireless/ath/ar5523/ar5523.c
@@ -589,6 +589,18 @@ static void ar5523_data_rx_cb(struct urb *urb)
 		goto skip;
 	}
 
+	if (rxlen < sizeof(struct ar5523_rx_desc)) {
+		ar5523_dbg(ar, "RX: Bad descriptor (len=%u is too small)\n",
+			   rxlen);
+		goto skip;
+	}
+
+	if (rxlen > usblen - sizeof(struct ar5523_chunk)) {
+		ar5523_dbg(ar, "RX: rxlen too large (rxlen=%u usblen=%d)\n",
+			   rxlen, usblen);
+		goto skip;
+	}
+
 	skb_reserve(data->skb, sizeof(*chunk));
 	skb_put(data->skb, rxlen - sizeof(struct ar5523_rx_desc));
 
-- 
2.43.0