Re: [PATCH v2 2/2] drm/dp_mst: Track allocated_pbn from ALLOCATE_PAYLOAD down-reply

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/dp_mst: Data race and logical flaw in allocated_pbn causes spurious -ENOSPC
- [High] drm/dp_mst: Unvalidated allocated_pbn from device causes permanent DoS
--

commit 4a7c9a56c428ab20bf4027cd5c4e8a720688111a
Author: Xiao Lu <xiaolu.xie@intel.com>

drm/dp_mst: Track allocated_pbn from ALLOCATE_PAYLOAD down-reply

This commit stores the allocated_pbn returned by the ALLOCATE_PAYLOAD
down-reply sideband message. It uses the larger of requested PBN and
allocated PBN for the MST bandwidth limit checks, carrying the value across
dependent atomic commits.

> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 7ce9e212770ad..b0e4cf8458eda 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -3166,10 +3167,14 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr,
>  	 */
>  	ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
>  	if (ret > 0) {
> -		if (txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK)
> +		if (txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK) {
>  			ret = -EINVAL;
> -		else
> +		} else {
> +			if (payload)
> +				payload->allocated_pbn =
> +					txmsg->reply.u.allocate_payload.allocated_pbn;

[Severity: High]
Is it safe to write to the active atomic state payload from an asynchronous
worker here without modeset locks?

If drm_dp_add_payload_part2() locklessly mutates allocated_pbn concurrently
with drm_dp_mst_duplicate_state(), could this result in a data race?

[Severity: High]
Is this untrusted 16-bit value from the branch device saved into the payload
state here without bounds checking?

If a malicious or malfunctioning branch device replies to the
ALLOCATE_PAYLOAD message with a spoofed allocated_pbn value (e.g., 65535),
could it be stored directly without validation?

>  			ret = 0;
> +		}
>  	}
>  	kfree(txmsg);
>  fail_put:
[ ... ]
> @@ -4670,6 +4678,7 @@ void drm_dp_mst_atomic_wait_for_dependencies(struct drm_atomic_commit *state)
>  			new_payload = drm_atomic_get_mst_payload_state(new_mst_state,
>  								       old_payload->port);
>  			new_payload->vc_start_slot = old_payload->vc_start_slot;
> +			new_payload->allocated_pbn = old_payload->allocated_pbn;

[Severity: High]
Could this unconditional restore overwrite the 0 set by
drm_dp_atomic_find_time_slots() during the atomic check phase?

If it does, could it inflate the perceived bandwidth for concurrent checks?

>  			new_payload->payload_allocation_status =
>  							old_payload->payload_allocation_status;
>  		}
[ ... ]
> @@ -5352,7 +5361,7 @@ drm_dp_mst_atomic_check_port_bw_limit(struct drm_dp_mst_port *port,
>  			return -EINVAL;
>  		}
>  
> -		pbn_used = payload->pbn;
> +		pbn_used = max(payload->pbn, payload->allocated_pbn);

[Severity: High]
Does evaluating this max() with a potentially stale or artificially inflated
value concurrently produce incorrect bandwidth checks?

If so, could it cause spurious -ENOSPC failures in bandwidth validation,
causing display configurations to fail randomly?

Also, if a maliciously huge value was evaluated here, could it force
pbn_used > port->full_pbn, failing the check?

Since the topology check iterates over all payloads, could this permanently
prevent any future display configurations on this topology?

>  	} else {
>  		pbn_used = drm_dp_mst_atomic_check_mstb_bw_limit(port->mstb,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922092647.2263962-1-xiaolu.xie@intel.com?part=2