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

[Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@intel.com>]

On 9/18/2026 12:12 AM, Golani, Mitulkumar Ajitkumar wrote:
>
>> -----Original Message-----
>> From: Dibin Moolakadan Subrahmanian
>> <dibin.moolakadan.subrahmanian@intel.com>
>> Sent: 07 September 2026 22:49
>> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
>> Cc: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>;
>> Nikula, Jani <jani.nikula@intel.com>
>> Subject: [PATCH v4 1/2] 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.
>>
>> BSpec: 69671, 68378
>> Fixes: 3d5928a168a9 ("drm/i915/xelpd: Pipe A DMC plugging")
> if available, please add Closes: <url> (with Reported-by:) to link it.

Thanks for the review.
Reported by internal tool , Closes not available.

>
>> Cc: stable@vger.kernel.org
>> Assisted-by: Claude-Code:Sonnet-5
>> Signed-off-by: Dibin Moolakadan Subrahmanian
>> <dibin.moolakadan.subrahmanian@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dmc.c      | 79 ++++++++++++++++---
>>   drivers/gpu/drm/i915/display/intel_dmc_regs.h | 12 +++
>>   2 files changed, 80 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c
>> b/drivers/gpu/drm/i915/display/intel_dmc.c
>> index a191eee240d9..051b721a0895 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 (header_ver != 3)
>> +		return true;
>> +
>> +	switch (dmc_id) {
>> +	case DMC_FW_MAIN:
>> +		start_range = DMC_MAIN_PROGRAM_BASE_START;
>> +		end_range = DMC_MAIN_PROGRAM_BASE_END;
>> +		break;
>> +	case DMC_FW_PIPEA:
>> +		start_range = DMC_PIPEA_PROGRAM_BASE_START;
>> +		end_range = DMC_PIPEA_PROGRAM_BASE_END;
>> +		break;
>> +	case DMC_FW_PIPEB:
>> +		start_range = DMC_PIPEB_PROGRAM_BASE_START;
>> +		end_range = DMC_PIPEB_PROGRAM_BASE_END;
>> +		break;
>> +	case DMC_FW_PIPEC:
>> +		start_range = DMC_PIPEC_PROGRAM_BASE_START;
>> +		end_range = DMC_PIPEC_PROGRAM_BASE_END;
>> +		break;
>> +	case DMC_FW_PIPED:
>> +		start_range = DMC_PIPED_PROGRAM_BASE_START;
>> +		end_range = DMC_PIPED_PROGRAM_BASE_END;
>> +		break;
>> +	default:
>> +		drm_warn(display->drm, "Unknown dmc_id %d for load
>> address sanity check\n", dmc_id);
>> +		return false;
> (nit) The switch already covers all five valid enum intel_dmc_id values, and callers only pass validated ids. The default is dead defensive code; returning false is safe but If the goal is to force a compile-time reminder, consider using "MISSING_CASE(dmc_id)" for consistency.

Will useMISSING_CASE(dmc_id)in next version.

>
>> +	}
>> +
>> +	if (payload_size == 0)
>> +		end_addr = start_addr;
> Bounds are computed on payload_size = fw_size * 4, which is 32-bit and can wrap. For fw_size >= 0x40000000, payload_size wraps small/zero, so both the max_fw_size cap and dmc_load_addr_sanity_check pass, while dmc_load_program still loops the raw dmc_fw_size and defeating the new check and causing OOB reads. Please validate fw_size (reject 0 and oversized) before the multiply.

Will fix the 32-bit wrap in next version.

>> +	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;
>> +}
>> +
>>   static bool dmc_mmio_addr_sanity_check(struct intel_dmc *dmc,
>>   				       const u32 *mmioaddr, u32 mmio_count,
>>   				       int header_ver, enum intel_dmc_id
>> dmc_id) @@ -1125,6 +1175,24 @@ static u32 parse_dmc_fw_header(struct
>> intel_dmc *dmc,
>>   		return 0;
>>   	}
>>
>> +	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;
>> +	}
>> +
>> +	if (!dmc_load_addr_sanity_check(dmc, start_mmioaddr, payload_size,
>> +					dmc_header->header_ver, dmc_id)) {
>> +		drm_err(display->drm, "DMC %d: firmware has wrong load
>> address\n", dmc_id);
>> +		return 0;
>> +	}
>> +
>>   	if (!dmc_mmio_addr_sanity_check(dmc, mmioaddr, mmio_count,
>>   					dmc_header->header_ver, dmc_id)) {
>>   		drm_err(display->drm, "DMC firmware has Wrong MMIO
>> Addresses\n"); @@ -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;
>> -	}
> (nit) Mention the code motion in the commit message. A single line "move payload_size computation earlier so the load-address check can use it" would save the next reviewer the diff archaeology.
>
>>   	dmc_info->dmc_fw_size = dmc_header->fw_size;
>>
>>   	dmc_info->payload = kmalloc(payload_size, GFP_KERNEL); diff --git
>> a/drivers/gpu/drm/i915/display/intel_dmc_regs.h
>> b/drivers/gpu/drm/i915/display/intel_dmc_regs.h
>> index 6b7978fb8986..324320afad58 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dmc_regs.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dmc_regs.h
>> @@ -521,6 +521,18 @@ enum pipedmc_event_id {
>>   #define TGL_PIPE_MMIO_END(dmc_id)	_PICK_EVEN(((dmc_id) - 1),
>> _TGL_PIPEA_MMIO_END,\
>>   					      _TGL_PIPEB_MMIO_END)
>>
>> +/* For DMC header version v3*/
>> +#define DMC_MAIN_PROGRAM_BASE_START	0x80000
>> +#define DMC_MAIN_PROGRAM_BASE_END	0x86fff
>> +#define DMC_PIPEA_PROGRAM_BASE_START	0x90000
>> +#define DMC_PIPEA_PROGRAM_BASE_END	0x96fff
>> +#define DMC_PIPEB_PROGRAM_BASE_START	0x98000
>> +#define DMC_PIPEB_PROGRAM_BASE_END	0x9efff
>> +#define DMC_PIPEC_PROGRAM_BASE_START	0x52000
>> +#define DMC_PIPEC_PROGRAM_BASE_END	0x53fff
>> +#define DMC_PIPED_PROGRAM_BASE_START	0x59000
>> +#define DMC_PIPED_PROGRAM_BASE_END	0x5afff
>> +
>>   #define SKL_DMC_DC3_DC5_COUNT	_MMIO(0x80030)
>>   #define SKL_DMC_DC5_DC6_COUNT	_MMIO(0x8002C)
>>   #define BXT_DMC_DC3_DC5_COUNT	_MMIO(0x80038)
>> --
>> 2.43.0