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