Re: [PATCH v12 2/8] drm/i915/bios: store VBT #57's metadata in intel_vbt_data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing bounds validation for VBT block 57 dimensions leading to out-of-bounds read
- [High] Unaligned pointer assignment drops __packed attribute, causing undefined behavior
- [Medium] Hardware assumption violation by proxying C20 PHY presence with DISPLAY_VER >= 14
--

commit ab5a1ab3ad655d1164dc5ca45a58ad0f45df01fb
Author: Michał Grzelak <michal.grzelak@intel.com>

drm/i915/bios: store VBT #57's metadata in intel_vbt_data
    
Store tables, number of tables, number of rows and number of columns in
intel_vbt_data when search for the VBT #57 has succeeded. Structurize
all VS/PE-O relevant metadata inside anonymous struct named as vspeo.

> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index ece4df4a9d62a..06a8dd7581bb8 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
[ ... ]
> +static int vswing_preemph_num_rows(struct intel_display *display)
> +{
> +	return DISPLAY_VER(display) >= 14 ? 16 : 10;

[Severity: Medium]
Does this heuristic correctly identify C20 PHY presence across all variants?

The commit message states that the presence of C20 or newer PHY causes each
table to contain 16 rows, and proxies this check with DISPLAY_VER >= 14.
However, platform families can encompass multiple variants (for instance,
Meteor Lake can have both C10 and C20 PHYs).

If a hardware variant covered by DISPLAY_VER >= 14 does not contain a C20 PHY,
could the VBT provide a 10-row table instead, causing the 16-row assumption
to misinterpret the data?

> +}
> +
>  static void
>  parse_vswing_preemph_override(struct intel_display *display)
>  {
[ ... ]
> @@ -2218,6 +2223,11 @@ parse_vswing_preemph_override(struct intel_display *display)
>  	/* pre-ICL GOPs don't have VBT #57 */
>  	if (!block)
>  		return;
> +
> +	display->vbt.vspeo.tables = block->tables;

[Severity: High]
Does this assignment drop the __packed attribute and cause unaligned pointer
dereferences?

Since struct bdb_vswing_preemph is marked __packed, its tables flexible array
can be at an unaligned offset. Assigning block->tables to const u32 *tables
(which assumes a standard 4-byte alignment) drops the packed attribute.
Could this lead to alignment faults or undefined behavior when elements of
vspeo.tables are accessed?

[Severity: High]
Is it possible for a malformed VBT to cause an out-of-bounds read here?

The fields num_tables and num_columns are accepted directly from the untrusted
VBT block and stored without validating that the actual block size (via
get_blocksize(block)) is large enough to contain the calculated number of
elements in the flexible tables array.

Could this lead to reading past the end of the heap allocation when the driver
later indexes into vspeo.tables?

> +	display->vbt.vspeo.num_tables = block->num_tables;
> +	display->vbt.vspeo.num_columns = block->num_columns;
> +	display->vbt.vspeo.num_rows = vswing_preemph_num_rows(display);
>  }
>  
>  static u8 translate_iboost(struct intel_display *display, u8 val)
> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
> index 7e988b7b1fe7a..c80b4a2f74f9e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
[ ... ]
> @@ -244,6 +244,13 @@ struct intel_vbt_data {
>  	struct list_head display_devices;
>  	struct list_head bdb_blocks;
>  
> +	struct {
> +		const u32 *tables;
> +		int num_tables;
> +		int num_columns;
> +		int num_rows;
> +	} vspeo;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904123148.2165596-1-michal.grzelak@intel.com?part=2