Re: [PATCH v8 17/21] drm/bridge: synopsys: dw-dp: Drop useless reservation of first slot

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Critical] Missing bounds check and integer underflow in DP AUX read allows a malicious DP sink to overflow the kernel stack.

Pre-existing issues:
- [High] Infinite loop in `dw_dp_link_clock_recovery()` triggered by oscillating sink requests.
--

--- Patch [17]: [PATCH v8 17/21] drm/bridge: synopsys: dw-dp: Drop useless reservation of first slot ---
commit 1a405fda22c71ea438c7181332eedd3948880f18
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Drop useless reservation of first slot

This commit drops the useless reservation of the first SDP slot in the
atomic_enable code. It resolves a potential race condition with audio SDP
reservation once that feature is added.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 4d4af6a388b19..193816126e44e 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -1697,9 +1697,6 @@ static void dw_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>  		return;
>  	}
>  
> -	scoped_guard(mutex, &dp->sdp_lock)
> -		set_bit(0, dp->sdp_reg_bank);
> -
>  	ret = dw_dp_link_enable(dp);
>  	if (ret < 0) {
>  		dev_err(dp->dev, "failed to enable link: %d\n", ret);

[ ... ]

> static int dw_dp_link_clock_recovery(struct dw_dp *dp)
> {
[ ... ]
> 	for (;;) {
[ ... ]
> 		adj_changed = dw_dp_link_get_adjustments(link, status);
> 		if (!adj_changed)
> 			tries++;
> 		else
> 			tries = 0;
> 
> 		if (tries == 5)
> 			break;
> 	}
> 
> 	return 0;
> }

[Severity: High]
This is a pre-existing issue, but does this code allow an infinite loop if a
non-compliant DP sink continuously alternates its requested voltage or
pre-emphasis levels?

Since there is no hard iteration limit, if the sink changes requests without
achieving clock recovery, adj_changed remains true and resets tries to 0.

Could this trap the kernel thread executing the atomic commit in an infinite
loop, leading to a permanent hung task?

[ ... ]

> @@ -1513,7 +1513,7 @@ static ssize_t dw_dp_aux_transfer(struct drm_dp_aux *aux,
>  		if (msg->request & DP_AUX_I2C_READ) {
>  			size_t count = FIELD_GET(AUX_BYTES_READ, value) - 1;
>  
> -			if (count != msg->size)
> +			if (!count)
>  				return -EBUSY;
>  
>  			ret = dw_dp_aux_read_data(dp, msg->buffer, count);

[Severity: Critical]
Does this change remove the bounds check against msg->size, introducing an
integer underflow and a potential buffer overflow?

If the hardware reports 0 bytes read, count = 0 - 1 underflows to SIZE_MAX.
This would cause !count to evaluate to false and dw_dp_aux_read_data() to
execute an effectively infinite write loop.

Even without underflow, if the hardware reports a size larger than msg->size,
could it write past the end of msg->buffer?

Since msg->buffer is frequently allocated on the stack by the DRM core for
small reads, this might grant an external device the ability to corrupt
kernel stack memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-synopsys-dw-dp-improvements-v8-0-ac1e6a75782f@collabora.com?part=17