Re: [PATCH v3 2/2] drm/i915/dmc: Harden DMC firmware payload parsing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Partial initialization of `dmc_info` on payload allocation failure leaves a stale `mmio_count`, leading to unintended hardware MMIO programming for missing firmwares.
--

commit bbb6b21d216798e993d4045cdf8b46c4ba305c9b
Author: Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@intel.com>

drm/i915/dmc: Harden DMC firmware payload parsing

This commit validates the DMC firmware size conversion using
check_mul_overflow() to prevent integer overflow. It also rejects zero-length
payloads and zero MMIO counts, and delays setting dmc_fw_size until after the
payload allocation succeeds.

> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
> --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c

[ ... ]

> @@ -1237,12 +1247,12 @@ static u32 parse_dmc_fw_header(struct intel_dmc *dmc,
>  	dmc_info->mmio_count = mmio_count;
>  	dmc_info->start_mmioaddr = start_mmioaddr;
>  
> -	dmc_info->dmc_fw_size = dmc_header->fw_size;
> -
>  	dmc_info->payload = kmalloc(payload_size, GFP_KERNEL);
>  	if (!dmc_info->payload)
>  		return 0;

[Severity: High]
This is a pre-existing issue, but partial initialization of dmc_info here
leaves a stale mmio_count on allocation failure.

Since this patch already moves the dmc_fw_size assignment below the kmalloc
check to avoid leaving stale state, should mmio_count and start_mmioaddr be
moved down as well?

If kmalloc() fails, parse_dmc_fw_header() returns 0 but leaves
dmc_info->mmio_count set to a non-zero value. Later in the firmware loading
sequence, intel_dmc_load_program() will proceed to call dmc_load_program()
for the failed secondary DMC, where it calls:

drivers/gpu/drm/i915/display/intel_dmc.c:dmc_load_program() {
    ...
    dmc_load_mmio(display, dmc_id);
    ...
}

Does this cause dmc_load_mmio() to iterate up to the stale mmio_count and
write the partially initialized mmiodata to hardware registers for a
firmware payload that was never loaded?

> +	dmc_info->dmc_fw_size = dmc_header->fw_size;
> +
>  	payload = (u8 *)(dmc_header) + header_len_bytes;
>  	memcpy(dmc_info->payload, payload, payload_size);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907135048.1403587-4-dibin.moolakadan.subrahmanian@intel.com?part=2