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

[Christian Lamparter <chunkeey@gmail.com>]

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;