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

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

Posted by Pengpeng Hou 1 day, 2 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. Discard them through the same free-and-break contract
used for unknown upload types. Retain USB's existing error path and copy
only the bytes after the event header in both paths.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v2: https://lore.kernel.org/all/20260720115119.80059-1-pengpeng@iscas.ac.cn/
- replace the new SDIO return -1 with free plus break as requested
- preserve USB's established error return contract
- rebase onto the current wireless tree

 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..3588c36f64cf 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);
+			break;
+		}
+
 		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;
-- 
2.50.1 (Apple Git-155)

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

Posted by Jeff Chen 19 hours ago

On Thu, Jul 23, 2026 at 10:40:05 AM +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. Discard them through the same free-and-break contract
> used for unknown upload types. Retain USB's existing error path and copy
> only the bytes after the event header in both paths.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> Changes since v2: https://lore.kernel.org/all/20260720115119.80059-1-pengpeng@iscas.ac.cn/
> - replace the new SDIO return -1 with free plus break as requested
> - preserve USB's established error return contract
> - rebase onto the current wireless tree
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index f039d6f19183..3588c36f64cf 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);
> +			break;
> +		}

skb->len includes the 4-byte header, so the body size is
skb->len - MWIFIEX_EVENT_HEADER_LEN. The upper bound should be:
skb->len - MWIFIEX_EVENT_HEADER_LEN > MAX_EVENT_SIZE

Otherwise packets with a full MAX_EVENT_SIZE body are incorrectly
rejected.

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

Posted by Francesco Dolcini 14 hours ago

On Thu, Jul 23, 2026 at 05:23:23PM +0800, Jeff Chen wrote:
> On Thu, Jul 23, 2026 at 10:40:05 AM +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. Discard them through the same free-and-break contract
> > used for unknown upload types. Retain USB's existing error path and copy
> > only the bytes after the event header in both paths.
> > 
> > Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> > ---
> > Changes since v2: https://lore.kernel.org/all/20260720115119.80059-1-pengpeng@iscas.ac.cn/
> > - replace the new SDIO return -1 with free plus break as requested
> > - preserve USB's established error return contract
> > - rebase onto the current wireless tree
> > 
> > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > index f039d6f19183..3588c36f64cf 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);
> > +			break;
> > +		}
> 
> skb->len includes the 4-byte header, so the body size is
> skb->len - MWIFIEX_EVENT_HEADER_LEN. The upper bound should be:
> skb->len - MWIFIEX_EVENT_HEADER_LEN > MAX_EVENT_SIZE
> 
> Otherwise packets with a full MAX_EVENT_SIZE body are incorrectly
> rejected.

Agree.

Please send a v4 With that fixed, and with that feel free to add

Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>