Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Incomplete payload size validation causes wait-completion handlers to read stale request data, allowing response spoofing and invalid state. --
commit ffe1d9057aa46275f644c2e152d0e7a9e8837c3b Author: Berkant Koc <[email protected]> drm/hyperv: validate VMBus packet size in receive callback This commit introduces bytes_recvd into hyperv_receive_sub() to validate packet sizes. It restricts the memcpy length to bytes_recvd and checks payload size for feature change messages. Cc: [email protected] # 5.14+ > diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c > b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c > --- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c > +++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c [ ... ] > @@ -420,26 +420,35 @@ static int hyperv_get_supported_resolution(struct > hv_device *hdev) > static void hyperv_receive_sub(struct hv_device *hdev, u32 bytes_recvd) > { > struct hyperv_drm_device *hv = hv_get_drvdata(hdev); > struct synthvid_msg *msg; > + size_t hdr_size; > > if (!hv) > return; > > + hdr_size = sizeof(struct pipe_msg_hdr) + > + sizeof(struct synthvid_msg_hdr); > + if (bytes_recvd < hdr_size || bytes_recvd > VMBUS_MAX_PACKET_SIZE) > + return; > + > msg = (struct synthvid_msg *)hv->recv_buf; > > /* Complete the wait event */ > if (msg->vid_hdr.type == SYNTHVID_VERSION_RESPONSE || > msg->vid_hdr.type == SYNTHVID_RESOLUTION_RESPONSE || > msg->vid_hdr.type == SYNTHVID_VRAM_LOCATION_ACK) { > - memcpy(hv->init_buf, msg, VMBUS_MAX_PACKET_SIZE); > + memcpy(hv->init_buf, msg, bytes_recvd); Could this change introduce a regression where a short packet leaves stale request data in hv->init_buf? The patch bounds the memcpy to bytes_recvd, but it doesn't appear to validate that bytes_recvd is large enough to contain the expected response payloads before calling complete(&hv->wait). Because hv->init_buf is used for both sending requests and receiving responses, a short response might only partially overwrite hv->init_buf. The payload portion would retain the outgoing request's data or zeroes from the pre-send initialization. For example, when sending a VRAM location update, the driver sets msg->vram.user_ctx = vram_pp. If a short response arrives, the payload is not overwritten. The waiter checks if (msg->vram_ack.user_ctx != vram_pp). Since vram and vram_ack share a union in struct synthvid_msg, this check might pass against the driver's own request data, allowing a truncated packet to spoof a successful ACK. Similarly, in hyperv_get_supported_resolution(), if a truncated response includes the resolution_count but omits the supported_resolution array, the uncopied array memory remains zero. The driver could parse zeroed dimensions, setting hv->preferred_width and height to 0, which could propagate to the DRM core. Would it be safer to have the receive handler validate the specific payload sizes for these wait-completion response types, similar to how it validates SYNTHVID_FEATURE_CHANGE below? > complete(&hv->wait); > return; > } > > if (msg->vid_hdr.type == SYNTHVID_FEATURE_CHANGE) { > + if (bytes_recvd < hdr_size + > + sizeof(struct synthvid_feature_change)) > + return; > hv->dirt_needed = msg->feature_chg.is_dirt_needed; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
