Thank you for your contribution! Sashiko AI review found 4 potential issues to consider: - [Medium] drm/i915/bios: missing BDB block size check in parse_vswing_preemph_override() - [Medium] drm/i915/bios: out-of-bounds read in combo PHY vspeo due to validate_vspeo() - [Medium] drm/i915/bios: validate_vspeo() rejects valid C10 and combo PHY VBT tables - [Low] drm/i915/bios: missing Fixes/Signed-off-by trailers and inaccurate changelog --
commit e60dd76fbe3fb6893876e4ed2f372a4da1a69f33 Author: Noah Weinberger <[email protected]> > drm/i915/bios: validate VBT vswing/preemphasis override table > > Hello, > > Proposed-fix limitation: the patch below does not validate the full > VBT table extent against the section size. The malformed/short-table > out-of-bounds case described below remains unresolved by these > existence and minimum-count checks. This is an incomplete proposal for > maintainer review; a complete fix still needs section-extent bounds > validation. > > parse_vswing_preemph_override() stores the raw pointer and count fields > from VBT block 57 without validating that the block actually contains > the announced table, and allocate_vswing_preemph_override() allocates > the consumer structures whenever a child device sets use_vbt_vswing - > even when the block is absent entirely. validate_vspeo() only checks > that devdata->vspeo is non-NULL and that the port has DP. > > Two failure modes follow at DDI buffer-translation time: > - block absent: tables == NULL, num_columns == 0, so the consumer > dereferences tables[0] (NULL). > - block short or malformed: the table walk reads up to > (5+1)*num_rows*num_columns u32s from a block whose enforced minimum > size is 2 bytes - with num_columns = 255 that is ~98 KB past a small > heap allocation, and the values are programmed into PHY registers. > > The patch gates the consumer on the table existing with sane counts. > A complete fix should also validate the table extent against the > section size, which is not currently plumbed through; happy to rework > if there is a preferred way to carry the section size. > > Affected version/commit: mainline fa4ad798f6f681031775266044ba1b0cf3c670bd > (v7.4-rc1, 2026-11-07), drivers/gpu/drm/i915/display/intel_bios.c:2216-2234 > (parse), 2654-2676 (allocate), 3874-3923 (validate/consume). > Introduced in the v7.4 merge window. > > This was found during an assisted (AI-supported) static source review of > mainline fa4ad798f6f681031775266044ba1b0cf3c670bd (v7.4-rc1, 2026-11-07); > every code path cited below was re-verified by the > reporter against that tree. The patch below has NOT been compiled and has > NOT been runtime tested, and no reproducer was produced - the triggering > conditions are described above instead. Please treat this as a report for > verification rather than a battle-tested fix. > > Reported-by: Noah Weinberger <[email protected]> [Severity: Low] This commit fixes bugs in VBT block 57 handling, so it needs a Signed-off-by: trailer and Fixes: tags for the introducing commits: Fixes: a138a81d7805 ("drm/i915/bios: de/allocate VS/PE-O buffers for each port") Fixes: e8fe5008ae3d ("drm/i915: override Snps's VS/PE when requested") Fixes: 7fdf86e530c7 ("drm/i915: override Combo's VS/PE when requested") The greeting and testing disclaimers also belong below the "---" separator rather than in the changelog. In addition, is the description of the absent-block failure mode accurate? When block 57 is absent, parse_vswing_preemph_override() returns before initializing display->vbt.vspeo.num_rows, so num_rows is 0; the loops in intel_bios_get_*_vspeo() do not iterate or dereference tables[0], and instead allocate_vswing_preemph_override() passes 0 to kzalloc_objs(), leaving devdata->entries set to ZERO_SIZE_PTR for callers to dereference. Finally, in validate_vspeo(), the blank line between the local variable declarations of vspeo and display is unneeded, and the !display->vbt.vspeo.tables check is redundant once allocate_vswing_preemph_override() skips allocating devdata->vspeo when tables is NULL. > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c > b/drivers/gpu/drm/i915/display/intel_bios.c > index 7b6a6f8c3cf19..d99a6756d68be 100644 > --- a/drivers/gpu/drm/i915/display/intel_bios.c > +++ b/drivers/gpu/drm/i915/display/intel_bios.c [ ... ] > @@ -3883,6 +3886,11 @@ validate_vspeo(const struct intel_bios_encoder_data > *devdata, bool has_dp) > if (!has_dp) > return false; > > + if (!display->vbt.vspeo.tables || [Severity: Medium] Can parse_vswing_preemph_override() validate the section size directly with get_blocksize(block) before populating display->vbt.vspeo? Whenever BDB_VSWING_PREEMPH is present, block->tables is a non-NULL flexible array address inside the block allocation, whose enforced minimum size in bdb_blocks[] is only 2 bytes. If a malformed or truncated VBT has num_columns >= 3 and num_tables >= 6 with a smaller payload, intel_bios_get_*_vspeo() will still read up to 6 * 16 * 255 * sizeof(u32) bytes past the heap buffer and program arbitrary slab memory into PHY registers. get_blocksize() is already available in intel_bios.c for any pointer returned by bdb_find_section(): drivers/gpu/drm/i915/display/intel_bios.c:get_blocksize() { static u32 get_blocksize(const void *block_data) { return _get_blocksize(block_data - 3); } } Checking get_blocksize(block) against struct_size(block, tables, size_mul(num_rows, size_mul(block->num_tables, block->num_columns))) in parse_vswing_preemph_override() would prevent out-of-bounds reads without needing extra plumbing. > + display->vbt.vspeo.num_columns < 3 || [Severity: Medium] Does checking only num_columns < 3 allow out-of-bounds reads in intel_bios_get_ehl_combo_vspeo() and intel_bios_get_jsl_combo_vspeo()? While C10 and C20 PHYs read 3 columns per row, both combo PHY helpers read 5 columns per row after calling validate_vspeo(): drivers/gpu/drm/i915/display/intel_bios.c:intel_bios_get_ehl_combo_vspeo() { 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]; ... offset += num_columns; } } If VBT block 57 specifies num_columns as 3 or 4 on Elkhart Lake or Jasper Lake, validate_vspeo() succeeds, and the combo PHY loop reads across row boundaries on every level and up to 2 u32 entries past the end of the selected table on the last row. Passing the required column count into validate_vspeo() (5 for combo PHY, 3 for C10/C20) would catch this. > + display->vbt.vspeo.num_tables < 6) > + return false; [Severity: Medium] Doesn't requiring num_tables >= 6 for all callers introduce a regression on C10, EHL, and JSL platforms whose VBT block 57 has fewer than 6 tables? Only intel_bios_get_c20_vspeo() indexes tables 4 and 5; intel_bios_get_c10_vspeo() only uses table indices 0 through 3, and intel_bios_get_ehl_combo_vspeo() and intel_bios_get_jsl_combo_vspeo() only use indices 0 through 2. Passing the selected table index idx into validate_vspeo() and checking idx >= display->vbt.vspeo.num_tables would avoid rejecting valid non-C20 VBT override tables. -- Sashiko AI review · https://sashiko.dev/#/patchset/40IXMMqFQJJm26J3saK7hx09nF4FuE3ZJcU6gMIryuxuGC5VENrcgA9q4klryPNSLnMwE64szv7eg4wf__gU8OlgzlL8tw3CmV_78Kc_H14=@proton.me?part=1
