Re: [PATCH v12 6/8] drm/i915: override Snps's VS/PE when requested

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing bounds checks on VBT-provided 'num_tables' and 'num_columns' when calculating array offsets leads to out-of-bounds read vulnerabilities when parsing VS/PE override tables.
- [High] Returning an empty buffer translation table when the VBT block is missing causes callers to access negative array indices (e.g., `entries[-1]`).
--

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

drm/i915: override Snps's VS/PE when requested

This commit adds accessor functions to read the requested Snps VS/PE
table from VBT block 57. It parses the requested table and transforms
the data into the port's buffer for C10 and C20 PHYs.

> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 1a09f7933e491..be7554c483ad2 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -3880,6 +3880,97 @@ bool intel_bios_encoder_supports_tbt(const struct intel_bios_encoder_data *devda
>  	return devdata->display->vbt.version >= 209 && devdata->child.tbt;
>  }
>  
> +static bool
> +validate_vspeo(const struct intel_bios_encoder_data *devdata, bool has_dp)
> +{
> +	struct intel_ddi_buf_trans *vspeo;
> +
> +	vspeo = devdata->vspeo;
> +	if (!vspeo)
> +		return false;

[Severity: High]
Does this validation need to ensure the VBT block data was actually parsed?

If VBT block 57 is absent, vspeo might be allocated with num_entries
set to 0, which would pass this check. If intel_bios_get_c20_vspeo returns
this empty vspeo, downstream callers like intel_ddi_buf_trans_get will
return n_entries = 0.

When n_entries = 0 reaches intel_ddi_level(), could it result in level
becoming -1:

drivers/gpu/drm/i915/display/intel_ddi.c:intel_ddi_level() {
    ...
    if (drm_WARN_ON_ONCE(display->drm, level >= n_entries))
        level = n_entries - 1;

    return level;
}

This would cause callers to access a negative array index on the entries
pointer, leading to a potential crash.

> +
> +	if (!has_dp)
> +		return false;
> +
> +	return true;
> +}
> +
> +const struct intel_ddi_buf_trans *
> +intel_bios_get_c20_vspeo(const struct intel_bios_encoder_data *devdata,
> +			 bool has_dp, bool is_uhbr)
> +{
> +	struct intel_display *display = devdata->display;
> +	union intel_ddi_buf_trans_entry *entries = devdata->entries;
> +	struct intel_ddi_buf_trans *vspeo = devdata->vspeo;
> +	const u32 *tables = display->vbt.vspeo.tables;
> +	int num_columns = display->vbt.vspeo.num_columns;
> +	int num_rows = display->vbt.vspeo.num_rows;
> +	int idx = is_uhbr ? 5 : 4;
> +	size_t offset = 0;
> +	int level;
> +
> +	if (!validate_vspeo(devdata, has_dp))
> +		return NULL;
> +
> +	offset += idx * num_rows * num_columns;

[Severity: High]
Should there be bounds checking on idx and num_columns here?

Since idx is computed dynamically (up to 5 for C20, and up to 3 for C10)
and multiplied by num_rows and num_columns, the resulting offset
could potentially exceed the size of the VBT block 57 data if num_columns
is unexpectedly large or the block is truncated.

Could this lead to an out-of-bounds read when accessing tables[offset + 2]
below, potentially reading unmapped memory?

> +
> +	for (level = 0; level < num_rows; level++) {
> +		u32 vswing = tables[offset];
> +		u32 pre_cursor = tables[offset + 1];
> +		u32 post_cursor = tables[offset + 2];

[ ... ]

> +const struct intel_ddi_buf_trans *
> +intel_bios_get_c10_vspeo(const struct intel_bios_encoder_data *devdata,
> +			 bool has_dp, int port_clock, bool has_edp)
> +{
> +	struct intel_display *display = devdata->display;
> +	union intel_ddi_buf_trans_entry *entries = devdata->entries;
> +	struct intel_ddi_buf_trans *vspeo = devdata->vspeo;
> +	const u32 *tables = display->vbt.vspeo.tables;
> +	int num_columns = display->vbt.vspeo.num_columns;
> +	int num_rows = display->vbt.vspeo.num_rows;
> +	size_t offset = 0;
> +	int level, idx;
> +
> +	if (!validate_vspeo(devdata, has_dp))
> +		return NULL;
> +
> +	if (has_edp)
> +		idx = port_clock > 540000 ? 3 : 2;
> +	else
> +		idx = port_clock > 270000 ? 1 : 0;
> +
> +	offset += idx * num_rows * num_columns;

[Severity: High]
Similar to the C20 function above, is it possible for this computed
offset to fall outside the bounds of the tables array if num_columns
or the selected idx is too large for the provided VBT data?

> +
> +	for (level = 0; level < num_rows; level++) {
> +		u32 vswing = tables[offset];

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