Re: [PATCH v2 1/2] drm/i915/dmc: Add sanity check for DMC load address

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] NULL pointer dereference in dmc_load_program() when firmware payload allocation fails for secondary pipes.
- [High] Firmware with fw_size == 0 bypasses sanity checks and causes a ZERO_SIZE_PTR dereference in assert_dmc_loaded().
--

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

drm/i915/dmc: Add sanity check for DMC load address
    
For DMC firmware header version 3, the firmware load address is stored
in dmc_info->start_mmioaddr and later used by dmc_load_program().
Unlike the MMIO address table, the firmware load address is not
validated. Add a sanity check to ensure it is within the valid range.

> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
> index a191eee240d9f..051b721a08959 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> @@ -1023,6 +1023,56 @@ static void dmc_set_fw_offset(struct intel_dmc *dmc,
>  	}
>  }
>  
> +/*
> + * Check if the load address is within the valid range for the given DMC ID.
> + */
> +static bool dmc_load_addr_sanity_check(struct intel_dmc *dmc,
> +				       u32 start_addr, u32 payload_size,
> +				       int header_ver, enum intel_dmc_id dmc_id)
> +{
> +	struct intel_display *display = dmc->display;
> +	u32 start_range, end_range, end_addr;
[ ... ]
> +	if (payload_size == 0)
> +		end_addr = start_addr;

[Severity: High]
This is a pre-existing issue, but this patch explicitly allows a 0-byte
payload to bypass the sanity check. If a firmware file has fw_size == 0,
kmalloc(0) will return ZERO_SIZE_PTR, which is not NULL.

Because payload is not NULL, has_dmc_id_fw() will return true, and
assert_dmc_loaded() will try to dereference it:

intel_dmc.c:assert_dmc_loaded() {
    ...
	found = intel_de_read(display, DMC_PROGRAM(dmc->dmc_info[dmc_id].start_mmioaddr, 0));
	expected = dmc->dmc_info[dmc_id].payload[0];
    ...
}

Could this cause a kernel panic due to a ZERO_SIZE_PTR dereference?
Should the sanity check instead reject firmware with a 0-byte payload?

> +	else if (check_add_overflow(start_addr, payload_size - 1, &end_addr))
> +		return false;
> +
> +	if (start_addr < start_range || end_addr > end_range)
> +		return false;
> +
> +	return true;
> +}
[ ... ]
> @@ -1169,17 +1237,6 @@ static u32 parse_dmc_fw_header(struct intel_dmc *dmc,
>  	dmc_info->mmio_count = mmio_count;
>  	dmc_info->start_mmioaddr = start_mmioaddr;
>  
> -	rem_size -= header_len_bytes;
> -
> -	/* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
> -	payload_size = dmc_header->fw_size * 4;
> -	if (rem_size < payload_size)
> -		goto error_truncated;
> -
> -	if (payload_size > dmc->max_fw_size) {
> -		drm_err(display->drm, "DMC FW too big (%u bytes)\n", payload_size);
> -		return 0;
> -	}
>  	dmc_info->dmc_fw_size = dmc_header->fw_size;
>  
>  	dmc_info->payload = kmalloc(payload_size, GFP_KERNEL);

[Severity: High]
This isn't a bug introduced by this patch, but if kmalloc() fails here
for a secondary pipe (like DMC_FW_PIPEA), does this leave the struct in
an inconsistent state?

If kmalloc() fails, the function returns 0, but dmc_info->dmc_fw_size is
already updated. Since parse_dmc_fw() ignores the return value for
secondary pipes, could this cause a NULL pointer dereference later in
dmc_load_program()?

intel_dmc_load_program() will call dmc_load_program() which iterates
based on dmc_fw_size:

intel_dmc.c:dmc_load_program() {
	for (i = 0; i < dmc->dmc_info[dmc_id].dmc_fw_size; i++) {
		intel_de_write_fw(display,
				  DMC_PROGRAM(dmc->dmc_info[dmc_id].start_mmioaddr, i),
				  dmc->dmc_info[dmc_id].payload[i]);
	}
}

Would it be safer to only update dmc_fw_size after payload allocation
succeeds?

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