[PATCH v3] p54: validate firmware record lengths

[PATCH v3] p54: validate firmware record lengths

[Heyang Tan]

The p54 firmware parser accesses component ID and component version
records without checking that their lengths cover the fields being read.
A malformed record can therefore make the parser read past the record.

Validate the component ID and component version record lengths before
accessing their data. The descriptor record is also checked against the
shortest descriptor length found in the supported firmware images.

Reject descriptor values that underflow the RX offset or produce an empty
RX range before updating the driver state.

Assisted-by: LLM Codex
Tested-by: Christian Lamparter <chunkeey@gmail.com>
Signed-off-by: Heyang Tan <thy15333007817@163.com>
---
Changes in v3:
- Simplify the component ID length check to its one-word format.
- Use a local bootrec_comp_ver pointer for version record access.
- Express the 24-byte version minimum as six 32-bit words.

 drivers/net/wireless/intersil/p54/fwio.c | 45 ++++++++++++++++++++----
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/intersil/p54/fwio.c b/drivers/net/wireless/intersil/p54/fwio.c
index a3d9053f043c..b19d054441b8 100644
--- a/drivers/net/wireless/intersil/p54/fwio.c
+++ b/drivers/net/wireless/intersil/p54/fwio.c
@@ -52,6 +52,12 @@ int p54_parse_firmware(struct ieee80211_hw *dev, const struct firmware *fw)
 		u32 code = le32_to_cpu(bootrec->code);
 		switch (code) {
 		case BR_CODE_COMPONENT_ID:
+			if (len != 1) {
+				wiphy_err(priv->hw->wiphy,
+					  "firmware component ID has invalid length\n");
+				return -EINVAL;
+			}
+
 			priv->fw_interface = be32_to_cpup((__be32 *)
 					     bootrec->data);
 			switch (priv->fw_interface) {
@@ -71,17 +77,44 @@ int p54_parse_firmware(struct ieee80211_hw *dev, const struct firmware *fw)
 				return -ENODEV;
 			}
 			break;
-		case BR_CODE_COMPONENT_VERSION:
+		case BR_CODE_COMPONENT_VERSION: {
+			struct bootrec_comp_ver *desc =
+				(struct bootrec_comp_ver *)bootrec->data;
+
+			if (len < 6) {
+				wiphy_err(priv->hw->wiphy,
+					  "firmware component version is too short\n");
+				return -EINVAL;
+			}
+
 			/* 24 bytes should be enough for all firmwares */
-			if (strnlen((unsigned char *) bootrec->data, 24) < 24)
-				fw_version = (unsigned char *) bootrec->data;
+			if (strnlen(desc->fw_version, sizeof(desc->fw_version)) <
+			    sizeof(desc->fw_version))
+				fw_version = (unsigned char *)desc->fw_version;
 			break;
+		}
 		case BR_CODE_DESCR: {
 			struct bootrec_desc *desc =
 				(struct bootrec_desc *)bootrec->data;
-			priv->rx_start = le32_to_cpu(desc->rx_start);
-			/* FIXME add sanity checking */
-			priv->rx_end = le32_to_cpu(desc->rx_end) - 0x3500;
+			u32 rx_start, rx_end;
+
+			/* 0xa is the shortest descriptor in supported firmware. */
+			if (len < 0xa) {
+				wiphy_err(priv->hw->wiphy,
+					  "firmware descriptor is too short\n");
+				return -EINVAL;
+			}
+
+			rx_start = le32_to_cpu(desc->rx_start);
+			rx_end = le32_to_cpu(desc->rx_end);
+			if (rx_end < 0x3500 || rx_end - 0x3500 <= rx_start) {
+				wiphy_err(priv->hw->wiphy,
+					  "firmware descriptor has invalid RX range\n");
+				return -EINVAL;
+			}
+
+			priv->rx_start = rx_start;
+			priv->rx_end = rx_end - 0x3500;
 			priv->headroom = desc->headroom;
 			priv->tailroom = desc->tailroom;
 			priv->privacy_caps = desc->privacy_caps;
-- 
2.34.1

Re: [PATCH v3] p54: validate firmware record lengths

[Christian Lamparter]

Hi!

On 10/10/26 5:37 AM, Heyang Tan wrote:
> The p54 firmware parser accesses component ID and component version
> records without checking that their lengths cover the fields being read.
> A malformed record can therefore make the parser read past the record.
> 
> Validate the component ID and component version record lengths before
> accessing their data. The descriptor record is also checked against the
> shortest descriptor length found in the supported firmware images.
> 
> Reject descriptor values that underflow the RX offset or produce an empty
> RX range before updating the driver state.
> 
> Assisted-by: LLM Codex
> Tested-by: Christian Lamparter <chunkeey@gmail.com>
> Signed-off-by: Heyang Tan <thy15333007817@163.com>
Acked-by: Christian Lamparter <chunkeey@gmail.com>
Cc: stable@vger.kernel.org

Yes, I think this looks good.

Thank you and Cheers,
Christian

> ---
> Changes in v3:
> - Simplify the component ID length check to its one-word format.
> - Use a local bootrec_comp_ver pointer for version record access.
> - Express the 24-byte version minimum as six 32-bit words.
> 
>   drivers/net/wireless/intersil/p54/fwio.c | 45 ++++++++++++++++++++----
>   1 file changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/intersil/p54/fwio.c b/drivers/net/wireless/intersil/p54/fwio.c
> index a3d9053f043c..b19d054441b8 100644
> --- a/drivers/net/wireless/intersil/p54/fwio.c
> +++ b/drivers/net/wireless/intersil/p54/fwio.c
> @@ -52,6 +52,12 @@ int p54_parse_firmware(struct ieee80211_hw *dev, const struct firmware *fw)
>   		u32 code = le32_to_cpu(bootrec->code);
>   		switch (code) {
>   		case BR_CODE_COMPONENT_ID:
> +			if (len != 1) {
> +				wiphy_err(priv->hw->wiphy,
> +					  "firmware component ID has invalid length\n");
> +				return -EINVAL;
> +			}
> +
>   			priv->fw_interface = be32_to_cpup((__be32 *)
>   					     bootrec->data);
>   			switch (priv->fw_interface) {
> @@ -71,17 +77,44 @@ int p54_parse_firmware(struct ieee80211_hw *dev, const struct firmware *fw)
>   				return -ENODEV;
>   			}
>   			break;
> -		case BR_CODE_COMPONENT_VERSION:
> +		case BR_CODE_COMPONENT_VERSION: {
> +			struct bootrec_comp_ver *desc =
> +				(struct bootrec_comp_ver *)bootrec->data;
> +
> +			if (len < 6) {
> +				wiphy_err(priv->hw->wiphy,
> +					  "firmware component version is too short\n");
> +				return -EINVAL;
> +			}
> +
>   			/* 24 bytes should be enough for all firmwares */
> -			if (strnlen((unsigned char *) bootrec->data, 24) < 24)
> -				fw_version = (unsigned char *) bootrec->data;
> +			if (strnlen(desc->fw_version, sizeof(desc->fw_version)) <
> +			    sizeof(desc->fw_version))
> +				fw_version = (unsigned char *)desc->fw_version;
>   			break;
> +		}
>   		case BR_CODE_DESCR: {
>   			struct bootrec_desc *desc =
>   				(struct bootrec_desc *)bootrec->data;
> -			priv->rx_start = le32_to_cpu(desc->rx_start);
> -			/* FIXME add sanity checking */
> -			priv->rx_end = le32_to_cpu(desc->rx_end) - 0x3500;
> +			u32 rx_start, rx_end;
> +
> +			/* 0xa is the shortest descriptor in supported firmware. */
> +			if (len < 0xa) {
> +				wiphy_err(priv->hw->wiphy,
> +					  "firmware descriptor is too short\n");
> +				return -EINVAL;
> +			}
> +
> +			rx_start = le32_to_cpu(desc->rx_start);
> +			rx_end = le32_to_cpu(desc->rx_end);
> +			if (rx_end < 0x3500 || rx_end - 0x3500 <= rx_start) {
> +				wiphy_err(priv->hw->wiphy,
> +					  "firmware descriptor has invalid RX range\n");
> +				return -EINVAL;
> +			}
> +
> +			priv->rx_start = rx_start;
> +			priv->rx_end = rx_end - 0x3500;
>   			priv->headroom = desc->headroom;
>   			priv->tailroom = desc->tailroom;
>   			priv->privacy_caps = desc->privacy_caps;