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 <[email protected]> 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/[email protected]?part=1
