* Re: [PATCH] drm/i915/bios: validate VBT vswing/preemphasis override table
2026-11-08 22:29 [PATCH] drm/i915/bios: validate VBT vswing/preemphasis override table Noah Weinberger
@ 2026-11-08 22:46 ` sashiko-bot
2026-11-10 1:12 ` ✗ LGCI.VerificationFailed: failure for " Patchwork
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-11-08 22:46 UTC (permalink / raw)
To: Noah Weinberger; +Cc: intel-gfx@lists.freedesktop.org
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 <NoahWeinberger2003@proton.me>
> 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 <noahweinberger2003@proton.me>
[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
^ permalink raw reply [flat|nested] 3+ messages in thread