Re: [PATCH 2/2] drm/hyperv: validate VMBus packet size in receive callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incomplete payload size validation causes wait-completion handlers to read stale request data, allowing response spoofing and invalid state.
--

commit ffe1d9057aa46275f644c2e152d0e7a9e8837c3b
Author: Berkant Koc <me@berkoc.com>

drm/hyperv: validate VMBus packet size in receive callback

This commit introduces bytes_recvd into hyperv_receive_sub() to validate
packet sizes. It restricts the memcpy length to bytes_recvd and checks
payload size for feature change messages.

Cc: stable@vger.kernel.org # 5.14+

> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c

[ ... ]

> @@ -420,26 +420,35 @@ static int hyperv_get_supported_resolution(struct hv_device *hdev)
>  static void hyperv_receive_sub(struct hv_device *hdev, u32 bytes_recvd)
>  {
>  	struct hyperv_drm_device *hv = hv_get_drvdata(hdev);
>  	struct synthvid_msg *msg;
> +	size_t hdr_size;
>  
>  	if (!hv)
>  		return;
>  
> +	hdr_size = sizeof(struct pipe_msg_hdr) +
> +		   sizeof(struct synthvid_msg_hdr);
> +	if (bytes_recvd < hdr_size || bytes_recvd > VMBUS_MAX_PACKET_SIZE)
> +		return;
> +
>  	msg = (struct synthvid_msg *)hv->recv_buf;
>  
>  	/* Complete the wait event */
>  	if (msg->vid_hdr.type == SYNTHVID_VERSION_RESPONSE ||
>  	    msg->vid_hdr.type == SYNTHVID_RESOLUTION_RESPONSE ||
>  	    msg->vid_hdr.type == SYNTHVID_VRAM_LOCATION_ACK) {
> -		memcpy(hv->init_buf, msg, VMBUS_MAX_PACKET_SIZE);
> +		memcpy(hv->init_buf, msg, bytes_recvd);

Could this change introduce a regression where a short packet leaves stale
request data in hv->init_buf?

The patch bounds the memcpy to bytes_recvd, but it doesn't appear to validate
that bytes_recvd is large enough to contain the expected response payloads
before calling complete(&hv->wait).

Because hv->init_buf is used for both sending requests and receiving responses,
a short response might only partially overwrite hv->init_buf. The payload
portion would retain the outgoing request's data or zeroes from the pre-send
initialization.

For example, when sending a VRAM location update, the driver sets
msg->vram.user_ctx = vram_pp. If a short response arrives, the payload is not
overwritten. The waiter checks if (msg->vram_ack.user_ctx != vram_pp). Since
vram and vram_ack share a union in struct synthvid_msg, this check might pass
against the driver's own request data, allowing a truncated packet to spoof a
successful ACK.

Similarly, in hyperv_get_supported_resolution(), if a truncated response
includes the resolution_count but omits the supported_resolution array, the
uncopied array memory remains zero. The driver could parse zeroed dimensions,
setting hv->preferred_width and height to 0, which could propagate to the DRM
core.

Would it be safer to have the receive handler validate the specific payload
sizes for these wait-completion response types, similar to how it validates
SYNTHVID_FEATURE_CHANGE below?

>  		complete(&hv->wait);
>  		return;
>  	}
>  
>  	if (msg->vid_hdr.type == SYNTHVID_FEATURE_CHANGE) {
> +		if (bytes_recvd < hdr_size +
> +		    sizeof(struct synthvid_feature_change))
> +			return;
>  		hv->dirt_needed = msg->feature_chg.is_dirt_needed;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260517-drm-hyperv-cover@berkoc.com?part=2