[v2] wifi: mwifiex: validate event lengths before copying bodies

[PATCH v2] wifi: mwifiex: validate event lengths before copying bodies

Posted by Pengpeng Hou 4 days, 14 hours ago

mwifiex event packets contain a four-byte event cause followed by the
event body. The USB and SDIO paths copy from data after that header using
the full packet length, so the source range extends four bytes beyond the
skb. The SDIO path also reads the event cause before validating the
packet and publishes oversized events without a copied body.

Reject SDIO events that are shorter than the header or larger than the
event buffer limit, using the same skb-release path for both failures.
Retain the equivalent USB bounds and copy only the bytes after the event
header in both paths.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v1: https://lore.kernel.org/all/20260704011317.50900-1-pengpeng@iscas.ac.cn/
- reject both short and oversized SDIO events through one cleanup path
- copy only the body length in the SDIO and USB paths
- rebase onto v7.2-rc4

 drivers/net/wireless/marvell/mwifiex/sdio.c | 15 +++++++++++----
 drivers/net/wireless/marvell/mwifiex/usb.c  |  3 ++-
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index f039d6f19183..4101bf1edca1 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -1712,12 +1712,19 @@ static int mwifiex_decode_rx_packet(struct mwifiex_adapter *adapter,
 	case MWIFIEX_TYPE_EVENT:
 		mwifiex_dbg(adapter, EVENT,
 			    "info: --- Rx: Event ---\n");
+		if (skb->len < MWIFIEX_EVENT_HEADER_LEN ||
+		    skb->len > MAX_EVENT_SIZE) {
+			mwifiex_dbg(adapter, ERROR,
+				    "EVENT: invalid skb->len %u\n", skb->len);
+			dev_kfree_skb_any(skb);
+			return -1;
+		}
+
 		adapter->event_cause = get_unaligned_le32(skb->data);
 
-		if ((skb->len > 0) && (skb->len  < MAX_EVENT_SIZE))
-			memcpy(adapter->event_body,
-			       skb->data + MWIFIEX_EVENT_HEADER_LEN,
-			       skb->len);
+		memcpy(adapter->event_body,
+		       skb->data + MWIFIEX_EVENT_HEADER_LEN,
+		       skb->len - MWIFIEX_EVENT_HEADER_LEN);
 
 		/* event cause has been saved to adapter->event_cause */
 		adapter->event_received = true;
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index f4b94a1054f6..a6ed82e7228b 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -110,7 +110,8 @@ static int mwifiex_usb_recv(struct mwifiex_adapter *adapter,
 			}
 
 			memcpy(adapter->event_body, skb->data +
-			       MWIFIEX_EVENT_HEADER_LEN, skb->len);
+			       MWIFIEX_EVENT_HEADER_LEN,
+			       skb->len - MWIFIEX_EVENT_HEADER_LEN);
 
 			adapter->event_received = true;
 			adapter->event_skb = skb;

Re: [PATCH v2] wifi: mwifiex: validate event lengths before copying bodies

Posted by Francesco Dolcini 3 days, 20 hours ago

On Mon, Jul 20, 2026 at 07:51:19PM +0800, Pengpeng Hou wrote:
> mwifiex event packets contain a four-byte event cause followed by the
> event body. The USB and SDIO paths copy from data after that header using
> the full packet length, so the source range extends four bytes beyond the
> skb. The SDIO path also reads the event cause before validating the
> packet and publishes oversized events without a copied body.
> 
> Reject SDIO events that are shorter than the header or larger than the
> event buffer limit, using the same skb-release path for both failures.
> Retain the equivalent USB bounds and copy only the bytes after the event
> header in both paths.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> Changes since v1: https://lore.kernel.org/all/20260704011317.50900-1-pengpeng@iscas.ac.cn/
> - reject both short and oversized SDIO events through one cleanup path
> - copy only the body length in the SDIO and USB paths
> - rebase onto v7.2-rc4
> 
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 15 +++++++++++----
>  drivers/net/wireless/marvell/mwifiex/usb.c  |  3 ++-
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index f039d6f19183..4101bf1edca1 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -1712,12 +1712,19 @@ static int mwifiex_decode_rx_packet(struct mwifiex_adapter *adapter,
>  	case MWIFIEX_TYPE_EVENT:
>  		mwifiex_dbg(adapter, EVENT,
>  			    "info: --- Rx: Event ---\n");
> +		if (skb->len < MWIFIEX_EVENT_HEADER_LEN ||
> +		    skb->len > MAX_EVENT_SIZE) {
> +			mwifiex_dbg(adapter, ERROR,
> +				    "EVENT: invalid skb->len %u\n", skb->len);
> +			dev_kfree_skb_any(skb);
> +			return -1;

break instead of return -1?

mwifiex_decode_rx_packet() never return -1 in similar situations, why
doing this only in this case?

Francesco