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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing completion reinitialization allows unsolicited host responses to prematurely satisfy future waits.
- [High] Lack of message type verification after waiting causes type confusion on mismatched responses.
- [High] Concurrent overwrites of the shared `hv->init_buf` by the receive handler cause a TOCTOU data race and data corruption.
--

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

drm/hyperv: validate VMBus packet size in receive callback

This patch adds validation for the received VMBus packet size to ensure
it covers the pipe and synthvid headers. It also computes the type-specific
payload size and ensures the copy length is properly bounded.

> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> index c3d0ff229e3d0..4e6f703a1b334 100644
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
[ ... ]
> @@ -420,30 +420,92 @@ static int hyperv_get_supported_resolution(struct hv_device *hdev)
>  	return 0;
>  }
>  
> -static void hyperv_receive_sub(struct hv_device *hdev)
> +static void hyperv_receive_sub(struct hv_device *hdev, u32 bytes_recvd)
>  {
[ ... ]
> +	/*
> +	 * Shared completion path for the wait-event responses
> +	 * (VERSION_RESPONSE, RESOLUTION_RESPONSE, VRAM_LOCATION_ACK):
> +	 * require the type-specific payload before handing the buffer to
> +	 * the waiter.
> +	 */
> +	if (bytes_recvd < need) {
> +		drm_err_ratelimited(&hv->dev,
> +				    "synthvid packet too small for type %u: %u < %zu\n",
> +				    msg->vid_hdr.type, bytes_recvd, need);
> +		return;
>  	}
> +	memcpy(hv->init_buf, msg, bytes_recvd);

[Severity: High]
This is a pre-existing issue, but does the shared use of hv->init_buf create a
data race?

The driver uses the lockless hv->init_buf for both preparing outgoing requests
and receiving incoming responses.

If a malicious host sends an unsolicited response packet while the driver
thread is actively reading from or writing to hv->init_buf, could this
Time-of-Check to Time-of-Use (TOCTOU) overwrite cause the host to bypass
validations?

For example, could the host change resolution_count to exceed bounds after it
was checked in hyperv_get_supported_resolution()?

[Severity: High]
This isn't a bug introduced by this patch, but does the driver verify the
message type after waiting for the response?

Because hyperv_receive_sub() copies any of the three wait-driving message
types into hv->init_buf and completes the wait, a malicious host could send
a different response type (like SYNTHVID_VERSION_RESPONSE) than the one
requested.

The consumer (like hyperv_get_supported_resolution()) will then parse the
buffer as the expected type without checking msg->vid_hdr.type, reading
mismatched fields and bypassing the newly added length validations.

> +	complete(&hv->wait);

[Severity: High]
This isn't a bug introduced by this patch, but does the driver fail to
reinitialize the completion before waiting?

If a compromised or misbehaving host sends an unsolicited completion-driving
message, complete() will increment the internal counter.

When the driver subsequently sends its next request and calls
wait_for_completion_timeout(), it will return immediately without waiting
for the actual response, allowing the driver to proceed with mismatched or
leftover data.

>  }

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