Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [Critical] Using `bytes_recvd` for `memcpy()` without checking `vmbus_recvpacket()` return value leads to a massive heap buffer overflow. - [High] Strict sizeof() validation incorrectly rejects dynamically-sized SYNTHVID_RESOLUTION_RESPONSE packets. Pre-existing issues: - [High] Concurrent lockless write to `hv->init_buf` from VMBus callback allows a malicious host to overwrite data while the guest is validating it. - [High] Missing `reinit_completion()` before reusing the shared `hv->wait` completion object. -- commit 8035423b95c9b24007478019421c5f94fe1179ce Author: Berkant Koc <[email protected]> Subject: drm/hyperv: validate VMBus packet size in receive callback This patch passes bytes_recvd into hyperv_receive_sub() to validate that packets cover the pipe and synthvid headers as well as the type-specific payload. The memcpy operation is also modified to use bytes_recvd. 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 > index c3d0ff229e3d0..12d3feb4f88ac 100644 > --- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c > +++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c [ ... ] > @@ -420,26 +420,62 @@ 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) > { [ ... ] > 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); > + size_t need = hdr_size; > + > + switch (msg->vid_hdr.type) { > + case SYNTHVID_VERSION_RESPONSE: > + need += sizeof(struct synthvid_version_resp); > + break; > + case SYNTHVID_RESOLUTION_RESPONSE: > + need += sizeof(struct > synthvid_supported_resolution_resp); [Severity: High] Does this strict sizeof() check correctly handle dynamically-sized packets? If the host sends a response sized only for the active elements (up to resolution_count) rather than the maximum 64 elements, bytes_recvd will be less than need. Will this cause valid packets to be falsely rejected and break the driver initialization sequence? > + break; > + case SYNTHVID_VRAM_LOCATION_ACK: > + need += sizeof(struct synthvid_vram_location_ack); > + break; > + } > + 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 this pattern allow a malicious host to overwrite data while the guest is validating it? The callback unconditionally copies incoming response packets into hv->init_buf via memcpy() and calls complete(). Since the waiting thread processes hv->init_buf in process context without copying it to isolated local memory, can an untrusted host send a second packet immediately after validation to trigger a time-of-check to time-of-use (TOCTOU) data race? > complete(&hv->wait); [Severity: High] This isn't a bug introduced by this patch, but does the driver need to call reinit_completion(&hv->wait) before sending a new request? If a prior request times out and its response arrives late, the completion counter is incremented. Will the next wait immediately consume the completion counter and process stale data left in hv->init_buf? > return; > } [ ... ] > @@ -466,7 +502,7 @@ static void hyperv_receive(void *ctx) > &bytes_recvd, &req_id); > if (bytes_recvd > 0 && > recv_buf->pipe_hdr.type == PIPE_MSG_DATA) > - hyperv_receive_sub(hdev); > + hyperv_receive_sub(hdev, bytes_recvd); [Severity: Critical] If vmbus_recvpacket() encounters a packet larger than VMBUS_MAX_PACKET_SIZE (16KB), it returns -ENOBUFS and sets bytes_recvd to the required packet length. Since ret == 0 is only checked in the while condition, does this pass an unbounded bytes_recvd into the callback? If recv_buf still contains the header from a previous successful iteration, it looks like memcpy(hv->init_buf, msg, bytes_recvd) will copy more than 16KB into hv->init_buf, causing a heap buffer overflow. > } while (bytes_recvd > 0 && ret == 0); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
