Re: [PATCH] drm/dp/mst: fix OOB reads on 2-byte fields in sideband reply parsers

[lyude]

Comments down below

On Fri, 2026-04-10 at 04:00 +0000, Ashutosh Desai wrote:
> Three sideband reply parsers read 16-bit fields as:
> 
>   val = (raw->msg[idx] << 8) | (raw->msg[idx+1]);
> 
> and only check bounds after the fact (idx += 2, then
> if (idx > raw->curlen)).  When idx == raw->curlen at the
> point of the read, raw->msg[idx+1] is one byte past the
> received message data, reading into the following struct
> fields (curchunk_len, curchunk_idx, curlen).
> 
> Affected functions:
>  - drm_dp_sideband_parse_enum_path_resources_ack()
>    full_payload_bw_number and avail_payload_bw_number fields
>  - drm_dp_sideband_parse_allocate_payload_ack()
>    allocated_pbn field
>  - drm_dp_sideband_parse_query_payload_ack()
>    allocated_pbn field
> 
> Move the bounds check to immediately before each 2-byte read,
> using idx + 2 > raw->curlen, consistent with the fix pattern
> used elsewhere in this file.
> 
> Signed-off-by: Ashutosh Desai <ashutoshdesai993@xxxxxxxxx>
> ---
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 9cef6c576..f2a7dbc5e 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -931,14 +931,13 @@ static bool
> drm_dp_sideband_parse_enum_path_resources_ack(struct drm_dp_sideband
>  	idx++;
>  	if (idx > raw->curlen)
>  		goto fail_len;
> +	if (idx + 2 > raw->curlen)
> +		goto fail_len;

Same feedback as the other patch - we can just remove the idx check
above and replace it with the idx + 2 > raw->curlen check.

>  	repmsg->u.path_resources.full_payload_bw_number = (raw-
> >msg[idx] << 8) | (raw->msg[idx+1]);
>  	idx += 2;
> -	if (idx > raw->curlen)
> +	if (idx + 2 > raw->curlen)
>  		goto fail_len;
>  	repmsg->u.path_resources.avail_payload_bw_number = (raw-
> >msg[idx] << 8) | (raw->msg[idx+1]);
> -	idx += 2;
> -	if (idx > raw->curlen)
> -		goto fail_len;
>  	return true;
>  fail_len:
>  	DRM_DEBUG_KMS("enum resource parse length fail %d %d\n",
> idx, raw->curlen);
> @@ -958,10 +957,9 @@ static bool
> drm_dp_sideband_parse_allocate_payload_ack(struct drm_dp_sideband_ms
>  	idx++;
>  	if (idx > raw->curlen)
>  		goto fail_len;
> -	repmsg->u.allocate_payload.allocated_pbn = (raw->msg[idx] <<
> 8) | (raw->msg[idx+1]);
> -	idx += 2;
> -	if (idx > raw->curlen)
> +	if (idx + 2 > raw->curlen)
>  		goto fail_len;
> +	repmsg->u.allocate_payload.allocated_pbn = (raw->msg[idx] <<
> 8) | (raw->msg[idx+1]);
>  	return true;
>  fail_len:
>  	DRM_DEBUG_KMS("allocate payload parse length fail %d %d\n",
> idx, raw->curlen);
> @@ -977,10 +975,9 @@ static bool
> drm_dp_sideband_parse_query_payload_ack(struct drm_dp_sideband_msg_r
>  	idx++;
>  	if (idx > raw->curlen)
>  		goto fail_len;
> -	repmsg->u.query_payload.allocated_pbn = (raw->msg[idx] << 8)
> | (raw->msg[idx + 1]);
> -	idx += 2;
> -	if (idx > raw->curlen)
> +	if (idx + 2 > raw->curlen)
>  		goto fail_len;
> +	repmsg->u.query_payload.allocated_pbn = (raw->msg[idx] << 8)
> | (raw->msg[idx + 1]);
>  	return true;
>  fail_len:
>  	DRM_DEBUG_KMS("query payload parse length fail %d %d\n",
> idx, raw->curlen);