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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Missing array bounds checks when reading Voltage Swing/Pre-emphasis (VS/PE) override tables from the untrusted Video BIOS Table (VBT) firmware.
- [Medium] Incorrect VBT table selection for Elkhart Lake (EHL) eDP without low vswing, conflicting with the commit message specification.

Pre-existing issues:
- [High] Missing validation for empty VBT `BDB_VSWING_PREEMPH` block allows zero-sized override tables to break downstream DP link training with out-of-bounds array access.
--

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

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

Add accessor function for Combo to read requested table from VBT #57.
Parse the requested table and transform data into port's buffer.

> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index be7554c483ad2..11d2cbb78eabf 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -3971,6 +3971,99 @@ intel_bios_get_c10_vspeo(const struct intel_bios_encoder_data *devdata,
> [ ... ]
> +const struct intel_ddi_buf_trans *
> +intel_bios_get_ehl_combo_vspeo(const struct intel_bios_encoder_data *devdata,
> +			       bool has_dp, int port_clock, bool low_vswing_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;

[Severity: High]
This isn't a bug introduced by this patch, but does validate_vspeo() properly
validate the VS/PE override block?

If the VBT sets use_vbt_vswing = 1 but omits the BDB_VSWING_PREEMPH block,
parse_vswing_preemph_override() leaves num_rows = 0. This results in
vspeo->num_entries being 0.

Because validate_vspeo() only checks if the table struct was allocated and
ignores num_rows == 0, it returns true:

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;
...

Later, downstream callers like icl_mg_phy_set_signal_levels() compute
level = n_entries - 1 (which becomes -1) and perform an out-of-bounds array
access on trans->entries[-1]. Could we add a check for
vspeo->num_entries > 0?

> +
> +	if (low_vswing_edp)
> +		idx = port_clock > 540000 ? 2 : 1;
> +	else
> +		idx = port_clock > 270000 ? 1 : 0;

[Severity: Medium]
Could this table selection logic conflict with the commit message?

The commit message specifies: "Otherwise use 3rd table for eDP not using
low vswing."

When eDP does not use low vswing, low_vswing_edp is false, and this logic
assigns idx = 0 (1st table) for port_clock <= 270000, instead of idx = 2
(3rd table) as specified. This correctly applies the 1st table for
external DP, but incorrectly applies the 1st table for eDP panels without
low vswing.

> +
> +	offset += idx * num_rows * num_columns;
> +
> +	for (level = 0; level < num_rows; level++) {
> +		u32 dw2_swing_sel = tables[offset];
> +		u32 dw7_n_scalar = tables[offset + 1];
> +		u32 dw4_cursor_coeff = tables[offset + 2];
> +		u32 dw4_post_cursor_2 = tables[offset + 3];
> +		u32 dw4_post_cursor_1 = tables[offset + 4];

[Severity: High]
Can this sequence lead to an out-of-bounds read from the VBT firmware data?

This unconditionally accesses up to tables[offset + 4] without checking if
the VBT block's num_columns is at least 5, or if the overall block contains
sufficient bounds.

Because VBT data is untrusted firmware input, if num_columns is less than
5, or if idx relies on a num_tables value smaller than the hardcoded logic
assumes, this can read past the bounds of the dynamically sized
bdb_vswing_preemph block.

> [ ... ]
> +const struct intel_ddi_buf_trans *
> +intel_bios_get_jsl_combo_vspeo(const struct intel_bios_encoder_data *devdata,
> +			       bool has_dp, int port_clock, bool low_vswing_edp)
> +{
> [ ... ]
> +	for (level = 0; level < num_rows; level++) {
> +		u32 dw2_swing_sel = tables[offset];
> +		u32 dw7_n_scalar = tables[offset + 1];
> +		u32 dw4_cursor_coeff = tables[offset + 2];
> +		u32 dw4_post_cursor_2 = tables[offset + 3];
> +		u32 dw4_post_cursor_1 = tables[offset + 4];

[Severity: High]
Does this have the same out-of-bounds read risk for the JSL combo PHY
tables as noted above? We should ensure num_columns >= 5 and the offset is
within valid bounds.

> [ ... ]
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c b/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c
> index 92c0d0f933ab4..5ad8ad2d0b8e5 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c
> @@ -1784,6 +1784,40 @@ xe3plpd_get_lt_buf_trans(struct intel_encoder *encoder,
> [ ... ]
> +static const struct intel_ddi_buf_trans *
> +ehl_get_combo_buf_trans_override(struct intel_encoder *encoder,
> +				 const struct intel_crtc_state *crtc_state,
> +				 int *n_entries)
> +{
> +	const struct intel_bios_encoder_data *devdata = encoder->devdata;
> +	bool has_edp, has_dp;
> +	int port_clock;
> +
> +	has_edp = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP);
> +	has_dp = intel_crtc_has_dp_encoder(crtc_state);
> +	port_clock = crtc_state->port_clock;
> +
> +	return intel_bios_get_ehl_combo_vspeo(devdata, has_dp, port_clock,
> +					      has_edp && use_edp_low_vswing(encoder));

[Severity: Medium]
Does this discard the has_edp distinction for non-low-vswing eDP?

Passing has_edp && use_edp_low_vswing(encoder) conflates external DP and
eDP without low vswing into a single low_vswing_edp boolean parameter.

This causes intel_bios_get_ehl_combo_vspeo() to treat eDP panels without
low vswing exactly like external DP, leading to the incorrect table
selection pointed out earlier.

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