[PATCH] drm/i915/edp: Check supported link rates DPCD read

[PATCH] drm/i915/edp: Check supported link rates DPCD read

[Nikita Zhandarovich]

intel_edp_set_sink_rates() reads DP_SUPPORTED_LINK_RATES into a local
stack array and then parses the array unconditionally. If the read
fails or returns less data than requested, the array contents are not
valid and may result in bogus sink link rates being used.

Check that the full DPCD block was read before parsing it. If not, fall
back to the default sink rate handling.

Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.

Fixes: 68f357cb7347 ("drm/i915/dp: generate and cache sink rate array for all DP, not just eDP 1.4")
Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 6ef2a0043cda..b6650a12ca54 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4678,10 +4678,16 @@ intel_edp_set_sink_rates(struct intel_dp *intel_dp)
 
 	if (intel_dp->edp_dpcd[0] >= DP_EDP_14) {
 		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
+		ssize_t ret;
 		int i;
 
-		drm_dp_dpcd_read(&intel_dp->aux, DP_SUPPORTED_LINK_RATES,
-				 sink_rates, sizeof(sink_rates));
+		ret = drm_dp_dpcd_read(&intel_dp->aux, DP_SUPPORTED_LINK_RATES,
+				       sink_rates, sizeof(sink_rates));
+		if (ret != sizeof(sink_rates)) {
+			drm_dbg_kms(display->drm,
+				    "Unable to read eDP supported link rates, using default rates\n");
+			goto use_default_rates;
+		}
 
 		for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
 			int rate;
@@ -4715,6 +4721,7 @@ intel_edp_set_sink_rates(struct intel_dp *intel_dp)
 	 * Use DP_LINK_RATE_SET if DP_SUPPORTED_LINK_RATES are available,
 	 * default to DP_MAX_LINK_RATE and DP_LINK_BW_SET otherwise.
 	 */
+use_default_rates:
 	if (intel_dp->num_sink_rates)
 		intel_dp->use_rate_select = true;
 	else

Re: [PATCH] drm/i915/edp: Check supported link rates DPCD read

[Jani Nikula]

On Fri, 29 May 2026, Nikita Zhandarovich <n.zhandarovich@fintech.ru> wrote:
> intel_edp_set_sink_rates() reads DP_SUPPORTED_LINK_RATES into a local
> stack array and then parses the array unconditionally. If the read
> fails or returns less data than requested, the array contents are not
> valid and may result in bogus sink link rates being used.
>
> Check that the full DPCD block was read before parsing it. If not, fall
> back to the default sink rate handling.
>
> Found by Linux Verification Center (linuxtesting.org) with static
> analysis tool SVACE.

Thanks for the patch.

>
> Fixes: 68f357cb7347 ("drm/i915/dp: generate and cache sink rate array for all DP, not just eDP 1.4")
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 6ef2a0043cda..b6650a12ca54 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4678,10 +4678,16 @@ intel_edp_set_sink_rates(struct intel_dp *intel_dp)
>  
>  	if (intel_dp->edp_dpcd[0] >= DP_EDP_14) {
>  		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
> +		ssize_t ret;
>  		int i;
>  
> -		drm_dp_dpcd_read(&intel_dp->aux, DP_SUPPORTED_LINK_RATES,
> -				 sink_rates, sizeof(sink_rates));
> +		ret = drm_dp_dpcd_read(&intel_dp->aux, DP_SUPPORTED_LINK_RATES,
> +				       sink_rates, sizeof(sink_rates));

Please switch to drm_dp_dpcd_read_data(). It'll return < 0 for errors,
and is easier to deal with.

> +		if (ret != sizeof(sink_rates)) {
> +			drm_dbg_kms(display->drm,
> +				    "Unable to read eDP supported link rates, using default rates\n");
> +			goto use_default_rates;

I think I'd avoid the goto and use:

	memset(sink_rates, 0, sizeof(sink_rates));

BR,
Jani.

> +		}
>  
>  		for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
>  			int rate;
> @@ -4715,6 +4721,7 @@ intel_edp_set_sink_rates(struct intel_dp *intel_dp)
>  	 * Use DP_LINK_RATE_SET if DP_SUPPORTED_LINK_RATES are available,
>  	 * default to DP_MAX_LINK_RATE and DP_LINK_BW_SET otherwise.
>  	 */
> +use_default_rates:
>  	if (intel_dp->num_sink_rates)
>  		intel_dp->use_rate_select = true;
>  	else

-- 
Jani Nikula, Intel

Claude review: drm/i915/edp: Check supported link rates DPCD read

[Claude Code Review Bot]

Overall Series Review

Subject: drm/i915/edp: Check supported link rates DPCD read
Author: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
Patches: 2
Reviewed: 2026-06-04T16:37:23.909532

---

This is a single-patch series that adds error checking to the `drm_dp_dpcd_read()` call in `intel_edp_set_sink_rates()`. The motivation is reasonable — the existing code reads DPCD supported link rates into a stack-allocated array and parses it unconditionally, which could use uninitialized stack data if the read fails or is short. The fix approach is sensible: check the return value and fall back to default rate handling on failure.

However, the patch has a notable mismatch with the current upstream code and some minor issues worth addressing.

**Verdict**: Reasonable defensive fix with minor issues. Needs a small update to match the current kernel tree.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/i915/edp: Check supported link rates DPCD read

[Claude Code Review Bot]

Patch Review

**Correctness of the core fix — Good**

The approach of checking the `drm_dp_dpcd_read()` return value is correct. Looking at the `drm_dp_dpcd_read()` implementation at `drm_dp_helper.c:748`, it returns the number of bytes transferred on success or a negative error code on failure. The docstring also confirms that short reads return `-EPROTO`:

> Returns the number of bytes transferred on success, or a negative error code on failure. ... If not all bytes were transferred, this function returns -EPROTO.

Given that `drm_dp_dpcd_read()` already returns `-EPROTO` for partial reads, the comparison `ret != sizeof(sink_rates)` correctly handles both errors (negative) and short reads (would be -EPROTO anyway, but belt-and-suspenders is fine here).

**Issue 1 — `ssize_t` type mismatch with current `drm_dp_dpcd_read()` return type**

The patch declares:
```c
ssize_t ret;
```

But looking at the current `drm_dp_dpcd_read()` definition in `drm_dp_helper.c:748-751`:
```c
ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
                          void *buffer, size_t size)
{
    int ret;
```

The function signature returns `ssize_t`, so this is technically correct. However, I note the function internally uses `int ret`. This is fine — `ssize_t` for the caller's variable matches the declared return type.

**Issue 2 — `goto` label placement creates a subtle semantic change**

The `use_default_rates:` label is placed at:
```c
use_default_rates:
	if (intel_dp->num_sink_rates)
		intel_dp->use_rate_select = true;
	else
		intel_dp_set_sink_rates(intel_dp);
```

When the `goto` is taken, `intel_dp->num_sink_rates` will be 0 (set at the top of the function on line 4680 of the original), so the `else` branch runs `intel_dp_set_sink_rates()`. This is correct behavior — on DPCD read failure, we fall back to the default rate path. The label name and debug message are clear about the intent.

**Issue 3 — Patch may not apply to current drm-next**

The patch is based on context showing the code at line 4678, but in the current tree this function starts at line 4806. The tree also contains additional code (the `QUIRK_EDP_LIMIT_RATE_HBR2` block at lines 4834-4841) that isn't in the patch context. This means the patch was likely written against an older tree. It should apply with fuzz or can be trivially rebased.

**Issue 4 — Missing `intel_edp_set_data_override_rates` on error path**

Looking at the full function in the current tree at `intel_dp.c:4856`:
```c
	intel_edp_set_data_override_rates(intel_dp);
```

This call happens at the end of the function. Since the `goto use_default_rates` jumps past the rate parsing loop but still hits this final call (it's after the label), the data override rates are still applied on the error path. This is correct — the `goto` doesn't skip that call.

**Issue 5 — Uninitialized stack data concern is somewhat overstated**

The commit message says the array "may result in bogus sink link rates being used." While technically true in theory, `drm_dp_dpcd_read()` already returns `-EPROTO` on partial reads per its documentation, so a short read (returning less than requested but positive) shouldn't actually happen with this API. The realistic failure mode is a negative error code. That said, the fix is still good defensive programming — the current code silently ignores read errors and would parse whatever garbage happened to be on the stack.

**Nit — Commit message title grammar**

The subject line `"drm/i915/edp: Check supported link rates DPCD read"` reads awkwardly. Something like `"drm/i915/edp: Check return value of supported link rates DPCD read"` would be clearer.

**Summary**: The patch is a reasonable defensive fix. The code change is correct and the fallback behavior is appropriate. The main feedback would be to rebase onto current drm-next and possibly clarify the commit subject line.

---
Generated by Claude Code Patch Reviewer