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

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

[Heyang Tan]

Hi Christian,

Thank you very much for reviewing the v3 patch and for providing your
Acked-by and Tested-by tags. I sincerely appreciate your careful testing
and guidance throughout the revisions.

May I also ask for your technical opinion on the security classification
of this issue? The malformed firmware records could cause out-of-bounds
reads in the kernel firmware parser, but I understand that the practical
security impact may depend on how the firmware can be supplied or replaced.

In your opinion, would this issue be appropriate for consideration by the
Linux kernel CVE team after the fix is merged and included in a released
stable kernel, or would it be more appropriate to regard it as a regular
firmware-parser robustness fix?

I understand that CVE assignment is handled by the Linux kernel CVE team,
and I am not asking you to make that decision or take any action on my
behalf. I would simply be very grateful for your technical advice on
whether contacting cve@kernel.org after the fix is released would be
reasonable.

Thank you again for your time, testing, and helpful review.

Best regards,
Heyang Tan
School of Software
Taiyuan University of Technology

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;

[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