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 <[email protected]> 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/[email protected]?part=2
