Re: [PATCH v4 1/2] drm/hyperv: validate resolution_count and fix WIN8 fallback

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing Completion Reinitialization causes state machine desynchronization.
- [High] Missing Message Type Validation allows parsing of incorrect response payloads.
--

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

drm/hyperv: validate resolution_count and fix WIN8 fallback

This commit bounds resolution_count against the array size to prevent walking
past the supported_resolution array in the parse loop. It also provides a
single post-probe fallback to ensure userspace framebuffers are not rejected
when the WIN10 resolution probe fails.

> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> index 051ecc5268321..c3d0ff229e3d0 100644
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> @@ -391,8 +391,11 @@ static int hyperv_get_supported_resolution(struct hv_device *hdev)
>  		return -ETIMEDOUT;
>  	}

[Severity: High]
This is a pre-existing issue, but is it possible for the state machine to
become desynchronized here if a timeout occurs?

The driver uses wait_for_completion_timeout() for synchronous VMBUS requests.
If the host is slow and the wait times out, the hv->wait completion counter is
not decremented since it returns early.

If the host responds after the timeout, hyperv_receive_sub() calls
complete(&hv->wait), incrementing the counter. Because the driver never calls
reinit_completion(&hv->wait) before sending a new request, the next transaction
could consume the late completion from the previous request immediately.

For example, a timeout during hyperv_get_supported_resolution() followed by a
late response could cause a subsequent hyperv_update_vram_location() to
complete immediately before the host has actually acknowledged the VRAM
location, which might lead to memory corruption or VM crashes.

>  
> -	if (msg->resolution_resp.resolution_count == 0) {
> -		drm_err(dev, "No supported resolutions\n");
> +	if (msg->resolution_resp.resolution_count == 0 ||
> +	    msg->resolution_resp.resolution_count >
> +	    SYNTHVID_MAX_RESOLUTION_COUNT) {

[Severity: High]
This isn't a bug introduced by this patch, but should the code verify the
message type before accessing the payload?

The driver uses a single wait completion (hv->wait) and a single response
buffer (hv->init_buf) for multiple VMBUS message types. In
hyperv_receive_sub(), receiving any valid response type (e.g.,
SYNTHVID_VERSION_RESPONSE, SYNTHVID_RESOLUTION_RESPONSE,
SYNTHVID_VRAM_LOCATION_ACK) will trigger complete(&hv->wait).

Functions waiting for responses, like hyperv_get_supported_resolution(), appear
to assume the payload in hv->init_buf is the correct response type without
verifying msg->vid_hdr.type.

If the state machine is desynchronized (e.g., due to a previous timeout) or a
buggy host sends an unsolicited response, could the driver misinterpret the
payload by reading fields like resolution_count from stale memory or data
belonging to a different response type?

> +		drm_err(dev, "Invalid resolution count: %d\n",
> +			msg->resolution_resp.resolution_count);
>  		return -ENODEV;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1779396074.git.me@berkoc.com?part=1