Re: [PATCH v5 07/15] drm/i915/alpm: table based establishment period

[sashiko-bot@kernel.org]

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 <animesh.manna@intel.com>
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/20260825054536.3455704-1-animesh.manna@intel.com?part=7