[PATCH v2 0/2] Add validation for DMC firmware header parsing

[PATCH v2 0/2] Add validation for DMC firmware header parsing

[Dibin Moolakadan Subrahmanian]

The DMC firmware parser does not validate the firmware load address
stored in version 3 headers before it is used by dmc_load_program().

The firmware size conversion from dwords to bytes can overflow while
calculating the payload size.

Fix both by validating the firmware load address against the expected
range and using check_mul_overflow() for the firmware size conversion.

v2:
Added Fixes tag in commit messages for both patches.

Dibin Moolakadan Subrahmanian (2):
  drm/i915/dmc: Add sanity check for DMC load address
  drm/i915/dmc: Prevent fw_size multiplication overflow

 drivers/gpu/drm/i915/display/intel_dmc.c      | 84 ++++++++++++++++---
 drivers/gpu/drm/i915/display/intel_dmc_regs.h | 12 +++
 2 files changed, 85 insertions(+), 11 deletions(-)

-- 
2.43.0

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

[Dibin Moolakadan Subrahmanian]

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")
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;
+	}
+
+	if (payload_size == 0)
+		end_addr = start_addr;
+	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;
-	}
 	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

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

[sashiko-bot]

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

[PATCH v2 2/2] drm/i915/dmc: Prevent fw_size multiplication overflow

[Dibin Moolakadan Subrahmanian]

Validate the conversion of the DMC firmware size from dwords to bytes
using check_mul_overflow() before calculating the firmware payload size.

Fixes: eb805623d8b1 ("drm/i915/skl: Add support to load SKL CSR firmware.")
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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
index 051b721a0895..228a1b296733 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
@@ -1178,7 +1178,12 @@ static u32 parse_dmc_fw_header(struct intel_dmc *dmc,
 	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 (check_mul_overflow(dmc_header->fw_size, 4u, &payload_size)) {
+		drm_err(display->drm, "DMC fw_size too large (%u dwords)\n",
+			dmc_header->fw_size);
+		return 0;
+	}
+
 	if (rem_size < payload_size)
 		goto error_truncated;
 
-- 
2.43.0

Re: [PATCH v2 2/2] drm/i915/dmc: Prevent fw_size multiplication overflow

[sashiko-bot]

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

Pre-existing issues:
- [High] Allocating a 0-sized firmware payload results in a `ZERO_SIZE_PTR` dereference, causing a kernel crash.
- [High] Integer underflow in MMIO count loop causes a massive out-of-bounds read and potential crash.
--

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

drm/i915/dmc: Prevent fw_size multiplication overflow

This commit validates the conversion of the DMC firmware size from dwords to
bytes using check_mul_overflow(). This prevents a potential integer overflow
when calculating the firmware payload size.

Fixes: eb805623d8b1 ("drm/i915/skl: Add support to load SKL CSR firmware.")
Cc: stable@vger.kernel.org
Assisted-by: Claude-Code:Sonnet-5
Signed-off-by: Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@intel.com>

> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
> index 051b721a08959..228a1b2967336 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> @@ -1178,7 +1178,12 @@ static u32 parse_dmc_fw_header(struct intel_dmc *dmc,
>  	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 (check_mul_overflow(dmc_header->fw_size, 4u, &payload_size)) {
> +		drm_err(display->drm, "DMC fw_size too large (%u dwords)\n",
> +			dmc_header->fw_size);
> +		return 0;
> +	}
> +

[Severity: High]
This is a pre-existing issue, but does this code handle a payload_size of
0 safely?

If a firmware file has fw_size == 0, payload_size will be 0. Later in
parse_dmc_fw_header() in drivers/gpu/drm/i915/display/intel_dmc.c, this
results in:

    dmc_info->payload = kmalloc(payload_size, GFP_KERNEL);

When payload_size is 0, kmalloc() returns ZERO_SIZE_PTR (address 16),
not NULL. 

Then, assert_dmc_loaded() unconditionally reads from the payload pointer:

    expected = dmc->dmc_info[dmc_id].payload[0];

Can this cause a kernel crash when dereferencing the ZERO_SIZE_PTR?

[Severity: High]
This isn't a bug introduced by this patch, but does parse_dmc_fw_header()
safely handle an mmio_count of 0?

In parse_dmc_fw_header() in drivers/gpu/drm/i915/display/intel_dmc.c, the
loop for processing mmio entries looks like:

    for (i = 0; i < mmio_count - 1; i++) {
        u32 orig_mmiodata[2] = {
            dmc_info->mmiodata[i],
            dmc_info->mmiodata[i+1],
        };
        ...

If the parsed firmware header has mmio_count == 0, the loop condition
underflows to UINT_MAX. Could this cause a massive out-of-bounds read
when accessing dmc_info->mmiodata[i]?

>  	if (rem_size < payload_size)
>  		goto error_truncated;
>

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

✓ i915.CI.BAT: success for Add validation for DMC firmware header parsing (rev3)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 2036 bytes --]

== Series Details ==

Series: Add validation for DMC firmware header parsing (rev3)
URL   : https://patchwork.freedesktop.org/series/170409/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_19095 -> Patchwork_170409v3
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_170409v3/index.html

Participating hosts (40 -> 38)
------------------------------

  Missing    (2): bat-dg2-13 fi-snb-2520m 

Known issues
------------

  Here are the changes found in Patchwork_170409v3 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@late_gt_pm:
    - fi-cfl-8109u:       [PASS][1] -> [DMESG-WARN][2] ([i915#13735]) +80 other tests dmesg-warn
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19095/fi-cfl-8109u/igt@i915_selftest@live@late_gt_pm.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_170409v3/fi-cfl-8109u/igt@i915_selftest@live@late_gt_pm.html

  * igt@kms_pipe_crc_basic@read-crc:
    - fi-cfl-8109u:       [PASS][3] -> [DMESG-WARN][4] ([i915#13735] / [i915#15673]) +49 other tests dmesg-warn
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19095/fi-cfl-8109u/igt@kms_pipe_crc_basic@read-crc.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_170409v3/fi-cfl-8109u/igt@kms_pipe_crc_basic@read-crc.html

  
  [i915#13735]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13735
  [i915#15673]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/15673


Build changes
-------------

  * Linux: CI_DRM_19095 -> Patchwork_170409v3

  CI-20190529: 20190529
  CI_DRM_19095: c60540b13d7bacc21848ed94fd368a968454d5db @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_9084: 9084
  Patchwork_170409v3: c60540b13d7bacc21848ed94fd368a968454d5db @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_170409v3/index.html

[-- Attachment #2: Type: text/html, Size: 2732 bytes --]