Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Unchecked negative error codes from drm_dp_lttpr_count() cause an out-of-bounds array read and integer underflow. Pre-existing issues: - [High] 32-bit signed integer overflow in `tml_phy_lock` calculation corrupts ALPM wake time. -- --- Patch [7]: [PATCH v5 07/15] drm/i915/alpm: table based establishment period --- commit 39688e03d0da05abea5d53e9441ff2410c77a69c Author: Animesh Manna <[email protected]> Subject: drm/i915/alpm: table based establishment period This patch introduces a table-based lookup for the establishment period in intel_alpm.c. It uses the link rate and LTTPR count to find the period from a 2D array, falling back to a computed value if not found. > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c > b/drivers/gpu/drm/i915/display/intel_alpm.c > index af2733805505e..078e04bb69d29 100644 > --- a/drivers/gpu/drm/i915/display/intel_alpm.c > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c [ ... ] > @@ -129,8 +215,17 @@ static int get_establishment_period(struct intel_dp > *intel_dp, > if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP)) { > tcds = (7 + DIV_ROUND_UP(6500, tml_phy_lock) + 1) * > tml_phy_lock; [Severity: High] This isn't a bug introduced by this patch, but does the calculation for tml_phy_lock suffer from a 32-bit signed integer overflow? Looking earlier in get_establishment_period(): int tml_phy_lock = 1000 * 1000 * tps4 / crtc_state->port_clock / 10; The expression 1000 * 1000 * tps4 evaluates using 32-bit signed integer math. For both DP (tps4 = 2520) and UHBR (tps4 = 12672), the intermediate result exceeds INT_MAX (2,147,483,647) and overflows (to -212643840 for UHBR). This negative value propagates into tml_phy_lock and causes tcds to subtract time instead of adding it, which could corrupt the ALPM wake time. > } else { > + int idx = get_link_rate_index(crtc_state->port_clock); > + > tcds = 7 * tml_phy_lock; > lttpr_count = drm_dp_lttpr_count(intel_dp->lttpr_common_caps); > + > + if (idx != LINK_RATE_INVALID && > + lttpr_count < DEFAULT_MAX_LTTPR_COUNT && [Severity: High] If a buggy or malicious DP sink returns an invalid LTTPR configuration, drm_dp_lttpr_count() can return a negative error code (e.g., -ERANGE or -EINVAL). Since lttpr_count is signed, will it incorrectly pass this lttpr_count < DEFAULT_MAX_LTTPR_COUNT check? > + establishment_periods[lttpr_count][idx]) { > + establishment_period = > establishment_periods[lttpr_count][idx]; Could this negative lttpr_count then be used as an array index on this line, causing an out-of-bounds memory read? If the out-of-bounds read accesses an unmapped page it could trigger a kernel oops. Furthermore, if the lookup is bypassed, the negative lttpr_count could cause severe integer underflow in the fallback calculation later in this function: int tdcs = (SILENCE_PERIOD_TIME + t1 + tcs + (lttpr_count - 1) * (tlw + tlfps_period)); > + return establishment_period; > + } > } > > if (lttpr_count) { -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
