[PATCH v3 wireless] wifi: wfx: validate firmware message body lengths before processing

[PATCH v3 wireless] wifi: wfx: validate firmware message body lengths before processing

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

The HIF message handlers cast the body pointer to typed structures
without verifying that the firmware actually sent enough data.  A
shorter-than-expected message causes reads beyond the message boundary.

Add a min_body_len field to the hif_handlers dispatch table and reject
messages that are too short before calling into any handler.  For the
multi-transmit confirmation, which has a variable-length tx_conf_payload
array, additionally validate the array extent with struct_size(), both
in rx_helper() before buffer accounting and in the handler itself.
Guard the body->data accesses in the error and exception indication
handlers against truncated messages.  Add the same minimum length check
for the inline-handled HIF_IND_ID_RX and generic confirmation paths.

Fixes: 9bca45f3d692 ("staging: wfx: allow to send 802.11 frames")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
v3:
  - Add min_body_len field to hif_handlers dispatch table and validate
    before calling any handler, addressing all hif_handlers (Jerome)
  - Guard flexible-array data[] accesses in error and exception handlers
    against truncated messages
  - Add body length check for inline-handled RX indication and generic
    confirmation paths
  - Keep struct_size() validation for multi-transmit flexible array
v2:
  - Added validation in bh.c rx_helper() before buffer accounting
  - Used struct_size() for overflow-safe flexible array validation
 drivers/net/wireless/silabs/wfx/bh.c     | 20 +++++--
 drivers/net/wireless/silabs/wfx/hif_rx.c | 70 ++++++++++++++++++------
 2 files changed, 69 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireless/silabs/wfx/bh.c b/drivers/net/wireless/silabs/wfx/bh.c
index 0071659bc9c98..b877dd01fb00a 100644
--- a/drivers/net/wireless/silabs/wfx/bh.c
+++ b/drivers/net/wireless/silabs/wfx/bh.c
@@ -6,6 +6,7 @@
  * Copyright (c) 2010, ST-Ericsson
  */
 #include <linux/gpio/consumer.h>
+#include <linux/overflow.h>
 #include <net/mac80211.h>
 
 #include "bh.h"
@@ -99,11 +100,22 @@ static int rx_helper(struct wfx_dev *wdev, size_t read_len, int *is_cnf)
 
 	if (!(hif->id & HIF_ID_IS_INDICATION)) {
 		(*is_cnf)++;
-		if (hif->id == HIF_CNF_ID_MULTI_TRANSMIT)
-			release_count =
-				((struct wfx_hif_cnf_multi_transmit *)hif->body)->num_tx_confs;
-		else
+		if (hif->id == HIF_CNF_ID_MULTI_TRANSMIT) {
+			const struct wfx_hif_cnf_multi_transmit *body =
+				(const struct wfx_hif_cnf_multi_transmit *)hif->body;
+			size_t body_len = le16_to_cpu(hif->len) - sizeof(*hif);
+
+			if (body_len < sizeof(*body) ||
+			    body_len < struct_size(body, tx_conf_payload,
+						   body->num_tx_confs)) {
+				dev_err(wdev->dev,
+					"invalid multi_tx_confirm length\n");
+				goto err;
+			}
+			release_count = body->num_tx_confs;
+		} else {
 			release_count = 1;
+		}
 		WARN(wdev->hif.tx_buffers_used < release_count, "corrupted buffer counter");
 		wdev->hif.tx_buffers_used -= release_count;
 	}
diff --git a/drivers/net/wireless/silabs/wfx/hif_rx.c b/drivers/net/wireless/silabs/wfx/hif_rx.c
index 64ca8acb8e4f2..ea3eba49552d7 100644
--- a/drivers/net/wireless/silabs/wfx/hif_rx.c
+++ b/drivers/net/wireless/silabs/wfx/hif_rx.c
@@ -7,6 +7,7 @@
  */
 #include <linux/skbuff.h>
 #include <linux/etherdevice.h>
+#include <linux/overflow.h>
 
 #include "hif_rx.h"
 #include "wfx.h"
@@ -62,9 +63,12 @@ static int wfx_hif_multi_tx_confirm(struct wfx_dev *wdev,
 				    const struct wfx_hif_msg *hif, const void *buf)
 {
 	const struct wfx_hif_cnf_multi_transmit *body = buf;
+	size_t body_len = le16_to_cpu(hif->len) - sizeof(*hif);
 	int i;
 
 	WARN(body->num_tx_confs <= 0, "corrupted message");
+	if (body_len < struct_size(body, tx_conf_payload, body->num_tx_confs))
+		return -EINVAL;
 	for (i = 0; i < body->num_tx_confs; i++)
 		wfx_tx_confirm_cb(wdev, &body->tx_conf_payload[i]);
 	return 0;
@@ -297,17 +301,17 @@ static int wfx_hif_error_indication(struct wfx_dev *wdev,
 				    const struct wfx_hif_msg *hif, const void *buf)
 {
 	const struct wfx_hif_ind_error *body = buf;
+	size_t body_len = le16_to_cpu(hif->len) - sizeof(*hif);
 	int type = le32_to_cpu(body->type);
-	int param = (s8)body->data[0];
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(hif_errors); i++)
 		if (type == hif_errors[i].val)
 			break;
 	if (i < ARRAY_SIZE(hif_errors))
-		if (hif_errors[i].has_param)
+		if (hif_errors[i].has_param && body_len > sizeof(*body))
 			dev_err(wdev->dev, "asynchronous error: %s: %d\n",
-				hif_errors[i].str, param);
+				hif_errors[i].str, (s8)body->data[0]);
 		else
 			dev_err(wdev->dev, "asynchronous error: %s\n", hif_errors[i].str);
 	else
@@ -323,10 +327,13 @@ static int wfx_hif_exception_indication(struct wfx_dev *wdev,
 					const struct wfx_hif_msg *hif, const void *buf)
 {
 	const struct wfx_hif_ind_exception *body = buf;
+	size_t body_len = le16_to_cpu(hif->len) - sizeof(*hif);
 	int type = le32_to_cpu(body->type);
 
-	if (type == 4)
+	if (type == 4 && body_len >= sizeof(*body) + sizeof(__le32))
 		dev_err(wdev->dev, "firmware assert %d\n", le32_to_cpup((__le32 *)body->data));
+	else if (type == 4)
+		dev_err(wdev->dev, "firmware assert\n");
 	else
 		dev_err(wdev->dev, "firmware exception\n");
 	print_hex_dump(KERN_INFO, "hif: ", DUMP_PREFIX_OFFSET,
@@ -339,21 +346,31 @@ static int wfx_hif_exception_indication(struct wfx_dev *wdev,
 static const struct {
 	int msg_id;
 	int (*handler)(struct wfx_dev *wdev, const struct wfx_hif_msg *hif, const void *buf);
+	size_t min_body_len;
 } hif_handlers[] = {
 	/* Confirmations */
-	{ HIF_CNF_ID_TX,                wfx_hif_tx_confirm },
-	{ HIF_CNF_ID_MULTI_TRANSMIT,    wfx_hif_multi_tx_confirm },
+	{ HIF_CNF_ID_TX,                wfx_hif_tx_confirm,
+	  sizeof(struct wfx_hif_cnf_tx) },
+	{ HIF_CNF_ID_MULTI_TRANSMIT,    wfx_hif_multi_tx_confirm,
+	  sizeof(struct wfx_hif_cnf_multi_transmit) },
 	/* Indications */
-	{ HIF_IND_ID_STARTUP,           wfx_hif_startup_indication },
-	{ HIF_IND_ID_WAKEUP,            wfx_hif_wakeup_indication },
-	{ HIF_IND_ID_JOIN_COMPLETE,     wfx_hif_join_complete_indication },
-	{ HIF_IND_ID_SET_PM_MODE_CMPL,  wfx_hif_pm_mode_complete_indication },
-	{ HIF_IND_ID_SCAN_CMPL,         wfx_hif_scan_complete_indication },
-	{ HIF_IND_ID_SUSPEND_RESUME_TX, wfx_hif_suspend_resume_indication },
-	{ HIF_IND_ID_EVENT,             wfx_hif_event_indication },
-	{ HIF_IND_ID_GENERIC,           wfx_hif_generic_indication },
-	{ HIF_IND_ID_ERROR,             wfx_hif_error_indication },
-	{ HIF_IND_ID_EXCEPTION,         wfx_hif_exception_indication },
+	{ HIF_IND_ID_STARTUP,           wfx_hif_startup_indication,
+	  sizeof(struct wfx_hif_ind_startup) },
+	{ HIF_IND_ID_WAKEUP,            wfx_hif_wakeup_indication, 0 },
+	{ HIF_IND_ID_JOIN_COMPLETE,     wfx_hif_join_complete_indication, 0 },
+	{ HIF_IND_ID_SET_PM_MODE_CMPL,  wfx_hif_pm_mode_complete_indication, 0 },
+	{ HIF_IND_ID_SCAN_CMPL,         wfx_hif_scan_complete_indication,
+	  sizeof(struct wfx_hif_ind_scan_cmpl) },
+	{ HIF_IND_ID_SUSPEND_RESUME_TX, wfx_hif_suspend_resume_indication,
+	  sizeof(struct wfx_hif_ind_suspend_resume_tx) },
+	{ HIF_IND_ID_EVENT,             wfx_hif_event_indication,
+	  sizeof(struct wfx_hif_ind_event) },
+	{ HIF_IND_ID_GENERIC,           wfx_hif_generic_indication,
+	  sizeof(struct wfx_hif_ind_generic) },
+	{ HIF_IND_ID_ERROR,             wfx_hif_error_indication,
+	  sizeof(struct wfx_hif_ind_error) },
+	{ HIF_IND_ID_EXCEPTION,         wfx_hif_exception_indication,
+	  sizeof(struct wfx_hif_ind_exception) },
 	/* FIXME: allocate skb_p from wfx_hif_receive_indication and make it generic */
 	//{ HIF_IND_ID_RX,              wfx_hif_receive_indication },
 };
@@ -363,8 +380,14 @@ void wfx_handle_rx(struct wfx_dev *wdev, struct sk_buff *skb)
 	int i;
 	const struct wfx_hif_msg *hif = (const struct wfx_hif_msg *)skb->data;
 	int hif_id = hif->id;
+	size_t body_len = le16_to_cpu(hif->len) - sizeof(*hif);
 
 	if (hif_id == HIF_IND_ID_RX) {
+		if (body_len < sizeof(struct wfx_hif_ind_rx)) {
+			dev_err(wdev->dev, "rx indication too short: %zu\n",
+				body_len);
+			goto free;
+		}
 		/* wfx_hif_receive_indication take care of skb lifetime */
 		wfx_hif_receive_indication(wdev, hif, hif->body, skb);
 		return;
@@ -372,13 +395,26 @@ void wfx_handle_rx(struct wfx_dev *wdev, struct sk_buff *skb)
 	/* Note: mutex_is_lock cause an implicit memory barrier that protect buf_send */
 	if (mutex_is_locked(&wdev->hif_cmd.lock) &&
 	    wdev->hif_cmd.buf_send && wdev->hif_cmd.buf_send->id == hif_id) {
+		if (body_len < sizeof(__le32)) {
+			dev_err(wdev->dev, "confirmation too short: %zu\n",
+				body_len);
+			goto free;
+		}
 		wfx_hif_generic_confirm(wdev, hif, hif->body);
 		goto free;
 	}
 	for (i = 0; i < ARRAY_SIZE(hif_handlers); i++) {
 		if (hif_handlers[i].msg_id == hif_id) {
-			if (hif_handlers[i].handler)
+			if (hif_handlers[i].handler) {
+				if (body_len < hif_handlers[i].min_body_len) {
+					dev_err(wdev->dev,
+						"message 0x%02x too short: %zu < %zu\n",
+						hif_id, body_len,
+						hif_handlers[i].min_body_len);
+					goto free;
+				}
 				hif_handlers[i].handler(wdev, hif, hif->body);
+			}
 			goto free;
 		}
 	}
-- 
2.47.3