Re: [PATCH v1 04/16] drm/i915/bios: prepare for parsing VBT #57

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing NULL check on `kzalloc` causing NULL pointer dereference.
- [High] Unconditional memory leak of `bufs_table` and its elements.
--

--- Patch [4]: [PATCH v1 04/16] drm/i915/bios: prepare for parsing VBT #57 ---
commit a6241aaaf22342914933a8b58de0747a6e01cf2a
Author: Michał Grzelak <[email protected]>

drm/i915/bios: prepare for parsing VBT #57

This commit adds parsing code for VBT #57 and a structure for storing VS/PE
data. It also allocates a matrix of intel_ddi_buf_trans_entry to store the
deparsed values and handles its deallocation on driver removal.

> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index b6fe87c29aa7c..778584c59fc6f 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c

[ ... ]

> @@ -2183,6 +2184,38 @@ parse_compression_parameters(struct intel_display *display)
>  	}
>  }
>  
> +static void
> +parse_vswing_preemph_override(struct intel_display *display)
> +{
> +	union intel_ddi_buf_trans_entry **bufs_table;
> +	const struct bdb_vswing_preemph *block;
> +	u8 num_rows;
> +
> +	if (display->vbt.version < 218)
> +		return;
> +
> +	block = bdb_find_section(display, BDB_VSWING_PREEMPH);
> +
> +	/* pre-ICL GOP don't have VBT #57 */
> +	if (!block)
> +		return;
> +
> +	num_rows = DISPLAY_VER(display) >= 14 ? 16 : 10;
> +
> +	bufs_table = kzalloc(block->num_tables * sizeof(*bufs_table), GFP_KERNEL);
> +
> +	for (int idx = 0; idx < block->num_tables; idx++)
> +		bufs_table[idx] = kzalloc(num_rows * sizeof(**bufs_table), GFP_KERNEL);

[Severity: High]
Is a NULL check missing here?

If kzalloc() fails to allocate bufs_table, the loop will immediately attempt
to assign to bufs_table[idx] and cause a NULL pointer dereference.

Additionally, if the inner kzalloc() for individual rows fails, it leaves
NULL pointers in the table without any error handling.

> +
> +	drm_dbg_kms(display->drm, "Vswing / Preemph Override not yet supported on the platform\n");
> +	bufs_table = NULL;

[Severity: High]
Does this cause an unconditional memory leak?

The memory just allocated for bufs_table and its elements is explicitly
overwritten with NULL here.

This causes display->vbt.vswing_preemph.bufs_table to receive NULL, and the
original allocated pointers are completely lost, preventing them from being
freed during intel_bios_driver_remove().

> +
> +	display->vbt.vswing_preemph.bufs_table = bufs_table;
> +	display->vbt.vswing_preemph.num_tables = block->num_tables;
> +	display->vbt.vswing_preemph.num_rows = num_rows;
> +	display->vbt.vswing_preemph.num_cols = block->num_columns;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4