[PATCH v2] p54: validate firmware record lengths

[PATCH v2] 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
Signed-off-by: Heyang Tan <thy15333007817@163.com>
---
 drivers/net/wireless/intersil/p54/fwio.c | 42 +++++++++++++++++++++---
 1 file changed, 37 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/intersil/p54/fwio.c b/drivers/net/wireless/intersil/p54/fwio.c
index a3d9053f043c..f028a7bfa734 100644
--- a/drivers/net/wireless/intersil/p54/fwio.c
+++ b/drivers/net/wireless/intersil/p54/fwio.c
@@ -52,6 +52,13 @@ 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 < sizeof(struct bootrec_comp_id) /
+				sizeof(*bootrec->data)) {
+				wiphy_err(priv->hw->wiphy,
+					  "firmware component ID is too short\n");
+				return -EINVAL;
+			}
+
 			priv->fw_interface = be32_to_cpup((__be32 *)
 					     bootrec->data);
 			switch (priv->fw_interface) {
@@ -72,16 +79,41 @@ int p54_parse_firmware(struct ieee80211_hw *dev, const struct firmware *fw)
 			}
 			break;
 		case BR_CODE_COMPONENT_VERSION:
+			if (len < DIV_ROUND_UP(sizeof(struct bootrec_comp_ver),
+					       sizeof(*bootrec->data))) {
+				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((unsigned char *)bootrec->data,
+				    sizeof(struct bootrec_comp_ver)) <
+				    sizeof(struct bootrec_comp_ver))
+				fw_version = (unsigned char *)bootrec->data;
 			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 v2] p54: validate firmware record lengths

[Christian Lamparter]

On 9/18/26 10:33 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
> Signed-off-by: Heyang Tan <thy15333007817@163.com>

yeah, both p54's pci and usb devices work.

| usb 3-1: reset high-speed USB device number 7 using xhci_hcd
| usb 3-1: Loading firmware file isl3887usb
| ieee80211 phy2: p54 detected a LM87 firmware
| p54: rx_mtu reduced from 3240 to 2384
| ieee80211 phy2: FW rev 2.13.25.0 - Softmac protocol 5.9
| ieee80211 phy2: cryptographic accelerator WEP:YES, TKIP:YES, CCMP:YES
| usbcore: registered new interface driver p54usb
| ieee80211 phy2: hwaddr 00:14:a5:35:e9:89, MAC:isl3892 RF:Xbow
| ieee80211 phy2: Selected rate control algorithm 'minstrel_ht'
| p54usb 3-1:1.0 wlx0014a535e989: renamed from wlan0
| usb 3-1: is registered as 'phy2'
| ieee80211 phy3: p54 detected a LM86 firmware
| p54: rx_mtu reduced from 3240 to 2376
| ieee80211 phy3: FW rev 2.13.25.0 - Softmac protocol 5.9
| ieee80211 phy3: cryptographic accelerator WEP:YES, TKIP:YES, CCMP:YES
| ieee80211 phy3: hwaddr 00:01:36:0d:d2:39, MAC:isl3880 RF:Frisbee
| ieee80211 phy3: Selected rate control algorithm 'minstrel_ht'
| p54pci 0000:06:02.0 wlp6s2: renamed from wlan0
| p54pci 0000:06:02.0: is registered as 'phy3'
(also tested down to the 2.5.2.0 firmwares)

Tested-by: Christian Lamparter <chunkeey@gmail.com>


> ---
>   drivers/net/wireless/intersil/p54/fwio.c | 42 +++++++++++++++++++++---
>   1 file changed, 37 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/intersil/p54/fwio.c b/drivers/net/wireless/intersil/p54/fwio.c
> index a3d9053f043c..f028a7bfa734 100644
> --- a/drivers/net/wireless/intersil/p54/fwio.c
> +++ b/drivers/net/wireless/intersil/p54/fwio.c
> @@ -52,6 +52,13 @@ 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 < sizeof(struct bootrec_comp_id) /
> +				sizeof(*bootrec->data)) {
> +				wiphy_err(priv->hw->wiphy,
> +					  "firmware component ID is too short\n");
> +				return -EINVAL;
> +			}
> +
>   			priv->fw_interface = be32_to_cpup((__be32 *)
>   					     bootrec->data);
>   			switch (priv->fw_interface) {

Ok? So, from what I know 100 characters per line is now "fine". But can you ask the AI why it
went for
+			if (len < sizeof(struct bootrec_comp_id) / sizeof(*bootrec->data)) {

yes, this is technically correct. That said, only an AI would take the effort to do this in a
such round about way. A "if (len != 1)" check would suffice (with an adjusted error message),
since all this descriptor can ever hold is a __le32 fw_variant according to p54.h (hex for
LM86/LM87/LM88/LM20). It's was very likely that __le32 chosen here because its "easier" to do
a case / switch / if on that. No idea if the type of the fw_variant should be changed to
__be32. I guess future AIs will/can tell us.

Maybe this all happens because unit of the len variable is a u32 / 4 bytes. If this is
cumbersome to deal with, I have no problem with refactoring this whole function to use u8/char
as len's unit. It would also be possible to initialize a struct for bootrec_comp_id *desc in
a similar fashion that BR_CODE_DESCR does.

> @@ -72,16 +79,41 @@ int p54_parse_firmware(struct ieee80211_hw *dev, const struct firmware *fw)
>   			}
>   			break;
>   		case BR_CODE_COMPONENT_VERSION:
> +			if (len < DIV_ROUND_UP(sizeof(struct bootrec_comp_ver),
> +					       sizeof(*bootrec->data))) {

same as above. Looking up what stuctures are involved and how big they are gets confusing fast.

> +
>   			/* 24 bytes should be enough for all firmwares */
> -			if (strnlen((unsigned char *) bootrec->data, 24) < 24)
> -				fw_version = (unsigned char *) bootrec->data;
> +			if (strnlen((unsigned char *)bootrec->data,
> +				    sizeof(struct bootrec_comp_ver)) <
> +				    sizeof(struct bootrec_comp_ver))
> +				fw_version = (unsigned char *)bootrec->data;
Hmm, for readability. couldn't you just define a struct bootrec_comp_ver* desc = bootrec->data;
(similar to what BR_CODE_DESCR) does and use that for everything in the function?

Any other thoughts?

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

[Heyang Tan]

Hi Christian,

A short follow-up with the additional firmware evidence:

- The historical daemonizer archive sample fw-softmac/2.13.1.0.arm is
  available locally. Its published MD5 is
  8ff41cff31c9323330d6170b54735477; local SHA-256 is
  190aa5174c6c08632bf0c452c95509f04ae93705efb7dd88444938fd871d2308.
- Its BR_CODE_DESCR record is 0xb 32-bit words, with rx_start=0x20200 and
  rx_end=0x27c60, so the 0xa minimum descriptor check remains compatible.
- I also generated a private truncated-descriptor fixture for parser-boundary
  testing (SHA-256
  8dc5f14057d46ae0a08f2406d0119991c645365d2dcf8c71c6f5de6fe0f8ad53).

The fixture is not an official linux-firmware image. I have not yet performed
kernel/KASAN execution with it or loaded it on real hardware, so this does not
claim a completed dynamic reproduction or a confirmed CVE.

Regards,
Heyang Tan

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

[Heyang Tan]

Hi Christian,

Thanks for the PCI/USB testing and the review comments. I will simplify the
component-ID length check and the component-version handling in the next
revision.

Could you also help with one focused parser-only validation on Linux, if
convenient? The goal is to distinguish the malformed-record boundary from the
normal firmware compatibility testing:

1. Run the private truncated-descriptor fixture through the unpatched parser
   under a KASAN-enabled kernel (a small KUnit/test-module harness is fine;
   physical p54 hardware should not be required).
2. Record whether KASAN reports a read beyond the allocated firmware buffer
   (`fw->data + fw->size`), rather than merely crossing into a following
   record.
3. Run the same fixture with the revised patch and confirm it returns
   `-EINVAL` without a KASAN report.

Fixture SHA-256:
8dc5f14057d46ae0a08f2406d0119991c645365d2dcf8c71c6f5de6fe0f8ad53

This is a private derived parser-boundary fixture, not an official
linux-firmware image. I am not claiming a CVE or a completed dynamic
reproduction until this comparison is available. If the harness is too much
work, a brief statement that the test cannot be run is also useful.

Regards,
Heyang Tan

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

[Heyang Tan]

Hi Christian,

Thanks for the detailed review and for testing both the PCI and USB devices,
including the older firmware versions.

You are right that the v2 checks are more roundabout than necessary. I used
the structure-size expressions to avoid hard-coding the record sizes while
accounting for the fact that `len` counts 32-bit words, but the result is less
readable than the format warrants.

For the next revision I will:

- use the direct one-word check for the component-ID record (`len != 1`),
  with an error message that describes the required record size;
- define a typed local `struct bootrec_comp_ver *` for the component-version
  record, similar to the existing descriptor handling, and use it to keep the
  size and string access easy to follow;
- keep `len` in 32-bit-word units and avoid a broader byte-unit refactoring in
  this patch;
- leave the existing firmware-variant endianness interpretation unchanged,
  since I do not have evidence that changing the field type is required.

I agree that these changes make the parser easier to review. I have no other
substantive changes to propose at this point, and will send a v3 after the
compile check.

Thanks again for the testing and the `Tested-by` tag.

Regards,
Heyang Tan

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

[Heyang Tan]

Hi Christian,

Thank you for testing both the p54 PCI and USB devices and for providing
the Tested-by tag. I also appreciate your detailed comments on the record
length units and code readability.

I have revised the patch based on your suggestions. In v3, the
BR_CODE_COMPONENT_ID check directly requires its one-word record format,
and BR_CODE_COMPONENT_VERSION uses a local struct bootrec_comp_ver pointer.
The version record check is also expressed directly as six 32-bit words,
which reflects the unit used by bootrec->len.

I will post v3 as a new thread to the linux-wireless mailing list and keep
the further discussion on the list.

Thank you again for your careful testing and helpful review.

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