[PATCH v3 0/2] wifi: wilc1000: firmware trust boundary hardening

[PATCH v3 0/2] wifi: wilc1000: firmware trust boundary hardening

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

This series adds missing bounds checks for firmware-controlled fields
in the Microchip wilc1000 driver.

Patch 1 fixes an integer underflow in network info frame length
processing. Patch 2 adds bounds checks for RX packet header fields.

Changes in v3:
  - Regenerated from wireless-next with proper git format-patch.

Changes in v2:
  - No code changes from v1.

Tristan Madani (2):
  wifi: wilc1000: fix integer underflow in wilc_network_info_received()
  wifi: wilc1000: fix OOB read from firmware RX packet header fields

 drivers/net/wireless/microchip/wilc1000/hif.c  | 11 ++++++++++-
 drivers/net/wireless/microchip/wilc1000/wlan.c |  5 +++++
 2 files changed, 15 insertions(+), 1 deletion(-)

-- 
2.47.3

[PATCH v3 1/2] wifi: wilc1000: fix integer underflow in wilc_network_info_received()

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

The firmware-controlled frame length at buffer[6..7] is decremented by 1
and used as the kmemdup size without validating the value. When the
firmware sends 0, the u16 subtraction wraps to 65535, causing a 64KB
out-of-bounds read from the RX buffer. For non-zero but inflated values,
the read exceeds the actual packet data.

Add validation that the frame length is at least 1 and fits within the
available buffer.

Fixes: c5c77ba18ea6 ("staging: wilc1000: Add SDIO/SPI 802.11 driver")
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
Changes in v3:
  - Regenerated from wireless-next with proper git format-patch to
    produce valid index hashes (v2 had post-processed index lines).

Changes in v2:
  - No code changes from v1.

 drivers/net/wireless/microchip/wilc1000/hif.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/hif.c b/drivers/net/wireless/microchip/wilc1000/hif.c
index 009c4770a6f95..473e406c98d87 100644
--- a/drivers/net/wireless/microchip/wilc1000/hif.c
+++ b/drivers/net/wireless/microchip/wilc1000/hif.c
@@ -1576,6 +1576,7 @@ void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length)
 	struct wilc_vif *vif;
 	int srcu_idx;
 	int result;
+	u16 frame_len;
 	int id;
 
 	id = get_unaligned_le32(&buffer[length - 4]);
@@ -1594,7 +1595,15 @@ void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length)
 	if (IS_ERR(msg))
 		goto out;
 
-	msg->body.net_info.frame_len = get_unaligned_le16(&buffer[6]) - 1;
+	frame_len = get_unaligned_le16(&buffer[6]);
+	if (frame_len == 0 || frame_len > length - 9) {
+		netdev_err(vif->ndev,
+			   "%s: invalid frame_len %u (buffer %u)\n",
+			   __func__, frame_len, length);
+		kfree(msg);
+		goto out;
+	}
+	msg->body.net_info.frame_len = frame_len - 1;
 	msg->body.net_info.rssi = buffer[8];
 	msg->body.net_info.mgmt = kmemdup(&buffer[9],
 					  msg->body.net_info.frame_len,
-- 
2.47.3

Re: [PATCH v3 1/2] wifi: wilc1000: fix integer underflow in wilc_network_info_received()

[Johannes Berg]

> @@ -1594,7 +1595,15 @@ void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length)
>  	if (IS_ERR(msg))
>  		goto out;
>  
> -	msg->body.net_info.frame_len = get_unaligned_le16(&buffer[6]) - 1;
> +	frame_len = get_unaligned_le16(&buffer[6]);
> +	if (frame_len == 0 || frame_len > length - 9) {
> +		netdev_err(vif->ndev,
> +			   "%s: invalid frame_len %u (buffer %u)\n",
> +			   __func__, frame_len, length);
> +		kfree(msg);
> +		goto out;
> +	}
> 

It seems it'd be trivial to check this *before* allocating the 'msg',
and be much better that way.

johannes

[PATCH v3 2/2] wifi: wilc1000: fix OOB read from firmware RX packet header fields

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

The firmware-controlled pkt_len, tp_len, and pkt_offset fields from RX
frame headers are used without validation against the buffer size. This
allows a malicious or malfunctioning firmware to cause out-of-bounds
reads from the RX buffer via wilc_frmw_to_host() and
wilc_wfi_mgmt_rx() memcpy operations.

Add bounds checks to ensure tp_len does not exceed remaining buffer
space, and pkt_len + pkt_offset fits within tp_len.

Fixes: c5c77ba18ea6 ("staging: wilc1000: Add SDIO/SPI 802.11 driver")
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
Changes in v3:
  - Regenerated from wireless-next with proper git format-patch to
    produce valid index hashes (v2 had post-processed index lines).

Changes in v2:
  - No code changes from v1.

 drivers/net/wireless/microchip/wilc1000/wlan.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
index 3fa8592eb2503..18024287f56a6 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
@@ -1123,6 +1123,11 @@ static void wilc_wlan_handle_rx_buff(struct wilc *wilc, u8 *buffer, int size)
 		if (pkt_len == 0 || tp_len == 0)
 			break;
 
+		if (tp_len > size - offset || pkt_len > tp_len) {
+			dev_err(wilc->dev, "invalid RX header: tp=%u pkt=%u remain=%d\n",
+				tp_len, pkt_len, size - offset);
+			break;
+		}
 		if (pkt_offset & IS_MANAGMEMENT) {
 			buff_ptr += HOST_HDR_OFFSET;
 			wilc_wfi_mgmt_rx(wilc, buff_ptr, pkt_len,
-- 
2.47.3