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

[Christian Lamparter <chunkeey@gmail.com>]

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?