On Thu, Aug 13, 2026 at 04:29:11PM GMT, [email protected] wrote:
> 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 <[email protected]>
> 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?
Yes, commit message is outdated.
>
> > 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.
Duplicate of memory leak from PATCH 4/8.
BR,
Michał
>
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/[email protected]?part=6
