Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Unsynchronized access to the shared `hv->init_buf` introduces Double Fetch/TOCTOU vulnerabilities, allowing a malicious host to bypass bounds checks and leak kernel memory. - [High] The driver reuses the `hv->wait` completion for synchronous messages without re-initializing it, which can cause spurious wakeups and synchronization bypass. -- commit dde0fd11b79967ca7df11b1049abebec363d25a5 Author: Berkant Koc <[email protected]> drm/hyperv: validate resolution_count and fix WIN8 fallback Adds bounds checking for resolution_count to prevent walking past the supported_resolution array. It also adds fallback handling to ensure WIN8 defaults are used when the 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 > @@ -387,12 +387,15 @@ static int hyperv_get_supported_resolution(struct > hv_device *hdev) > hyperv_sendpacket(hdev, msg); > > t = wait_for_completion_timeout(&hv->wait, VMBUS_VSP_TIMEOUT); [Severity: High] This isn't a bug introduced by this patch, but does the driver need to call reinit_completion(&hv->wait) before sending requests? If a delayed or unsolicited response arrives, hyperv_receive_sub() calls complete(&hv->wait), incrementing its internal counter. When this function is called later, wait_for_completion_timeout() will return immediately without waiting for the actual response. Could this cause the driver to erroneously parse its own outgoing request buffer as a response? > if (!t) { > drm_err(dev, "Time out on waiting resolution response\n"); > return -ETIMEDOUT; > } > > - 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: Critical] This is a pre-existing issue, but does reading directly from the shared hv->init_buf introduce a TOCTOU or Double Fetch problem here? Since hv->init_buf is also used by the asynchronous hyperv_receive_sub() callback, could a malicious host send a valid count to pass this check, and then immediately overwrite hv->init_buf with a large value before the subsequent for-loop evaluates i < msg->resolution_resp.resolution_count? Additionally, when the driver prepares requests earlier in this function (e.g., setting msg->vid_hdr.size), an unsolicited response could overwrite hv->init_buf. Could this trick vmbus_sendpacket() into using a host-controlled size and leaking kernel memory? > + drm_err(dev, "Invalid resolution count: %d\n", > + msg->resolution_resp.resolution_count); > return -ENODEV; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
