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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Out-of-bounds Read in VBT VSPEO parsing
- [High] Data Race when writing to shared devdata->entries array
- [High] Missing VBT block causes ZERO_SIZE_PTR allocation and out-of-bounds read
- [Medium] Contradiction of Intent: VBT override bypass for non-DP outputs
- [Medium] Memory leak of devdata->entries
--

--- Patch [6]: [PATCH v11 6/8] drm/i915: override Snps's VS/PE when requested ---
commit 70006d8778b43948ec0e561395199a65420366c2
Author: Michał Grzelak <michal.grzelak@intel.com>
Date:   Thu Aug 13 16:09:16 2026 +0000

    drm/i915: override Snps's VS/PE when requested
    
    This commit adds accessor functions to read requested VBT tables for
    Synopsys PHYs and transform the data for the port's buffer. It chooses
    the appropriate accessor based on display version and PHY type (C10/C20)
    and handles table indexing based on link rates.

> For C10, 1st table is used for external DP with modes below HBR2
> (exclusive). 1st table is also used as a fallback for non-DPs. 2nd
> table is used for external DP with modes higher than HBR2 (inclusive).

[Severity: Medium]
Is this description accurate?

The commit message states the 1st table is used as a fallback for non-DPs, but
looking at validate_vspeo() and intel_bios_get_c10_vspeo(), the code returns
false/NULL if has_dp is false. This completely aborts the VBT override path
for non-DP outputs rather than falling back to the 1st table.

Could the commit message be more precise, or should the code be adjusted to
allow this fallback?

> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index a491b85006117..56b4aacff7763 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
[ ... ]
> @@ -3881,6 +3881,99 @@ 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;
> +
> +	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;
> +
> +	for (level = 0; level < num_rows; level++) {
> +		u32 vswing = tables[offset];
> +		u32 pre_cursor = tables[offset + 1];
> +		u32 post_cursor = tables[offset + 2];

[Severity: High]
Does this code adequately protect against out-of-bounds memory reads from the
VBT tables array?

There is no validation that the chosen idx (which can be up to 5) is less
than display->vbt.vspeo.num_tables, or that num_columns is at least 3. If a
malformed or older VBT provides fewer tables or columns, could the driver
read past the end of the VBT block here?

> +
> +		entries[level].snps.vswing = vswing;
> +		entries[level].snps.pre_cursor = pre_cursor;
> +		entries[level].snps.post_cursor = post_cursor;

[Severity: High]
Can writing to the shared devdata->entries array inside this getter function
cause a data race?

This getter is called during PHY programming and link training (e.g., from
commit_tail and intel_dp_link_check). If the asynchronous modeset worker and
hotplug link retraining worker run concurrently, wouldn't they race to
overwrite the exact same devdata->entries array without synchronization,
potentially leading to the hardware being programmed with corrupted values?

> +
> +		offset += num_columns;
> +	}
> +
> +	vspeo->entries = entries;
> +	vspeo->num_entries = num_rows;
> +
> +	return vspeo;
> +}
> +
> +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;
> +
> +	for (level = 0; level < num_rows; level++) {
> +		u32 vswing = tables[offset];
> +		u32 pre_cursor = tables[offset + 1];
> +		u32 post_cursor = tables[offset + 2];

[Severity: High]
Is it possible to read past the end of the VBT block here?

Similar to the C20 function, there appears to be no validation that idx is
less than display->vbt.vspeo.num_tables or that num_columns is at least 3,
which might lead to out-of-bounds reads if the VBT is older or malformed.

> +
> +		entries[level].snps.vswing = vswing;
> +		entries[level].snps.pre_cursor = pre_cursor;
> +		entries[level].snps.post_cursor = post_cursor;

[Severity: High]
Does overwriting the shared devdata->entries array here introduce a race
condition when accessed concurrently?

If concurrent accesses occur, such as a hotplug event while a modeset is
active, they might race to write to this shared state without locking, leading
to corrupt PHY programming.

> +
> +		offset += num_columns;
> +	}
> +
> +	vspeo->entries = entries;
> +	vspeo->num_entries = num_rows;
> +
> +	return vspeo;
> +}


[Severity: High]
Could a missing VBT block lead to a ZERO_SIZE_PTR allocation and subsequent
out-of-bounds read?

If a system's VBT version is >= 218 and use_vbt_vswing is true, but VBT block
57 is omitted, parse_vswing_preemph_override() returns early leaving num_rows
as 0.

This would lead to allocating with size 0:

allocate_vswing_preemph_override() {
    ...
	entries = kzalloc_objs(*entries, num_rows);
	if (!entries) {
    ...
}

Because kzalloc_objs returns ZERO_SIZE_PTR for size 0, the !entries check
passes. When intel_ddi_buf_trans_get() returns this table with num_entries = 0,
the bounds check in intel_ddi_level() behaves incorrectly:

intel_ddi_level() {
    ...
	if (drm_WARN_ON_ONCE(display->drm, level >= n_entries))
		level = n_entries - 1;
    ...
}

This sets level to -1, and the PHY code then reads from trans->entries[-1],
which is an invalid memory address (ZERO_SIZE_PTR - sizeof(entry)), potentially
causing a kernel panic.


[Severity: Medium]
Will this sequence leak the devdata->entries memory on driver removal?

In allocate_vswing_preemph_override(), memory is allocated and assigned to
devdata->entries, but devdata->vspeo->entries remains NULL until one of the
accessor functions (like intel_bios_get_c20_vspeo) is called.

If a port is never used and the accessor is never invoked, the teardown path
frees the wrong pointer:

intel_bios_driver_remove() {
    ...
		if (devdata->vspeo)
			kfree(devdata->vspeo->entries);
    ...
}

Because devdata->vspeo->entries is still NULL in the unused case, the
underlying devdata->entries allocation appears to be permanently leaked.

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