RE: [PATCH v3 2/2] drm/hyperv: validate VMBus packet size in receive callback
From: Berkant Koc Sent: Tuesday, May 19, 2026 1:09 PM
>
> hyperv_receive_sub() reads msg->vid_hdr.type and dispatches into one
> of four message-type branches without knowing how many bytes the host
> wrote into hv->recv_buf. The completion path then runs
> memcpy(hv->init_buf, msg, VMBUS_MAX_PACKET_SIZE), so the consumer
> that wakes on wait_for_completion_timeout() can read up to 16 KiB of
> residue from a prior message as if it were the response payload.
>
> Pass bytes_recvd into hyperv_receive_sub() and reject any packet that
> does not cover the pipe + synthvid header. For each of the three
> completion-driving types (SYNTHVID_VERSION_RESPONSE,
> SYNTHVID_RESOLUTION_RESPONSE, SYNTHVID_VRAM_LOCATION_ACK) also
> require the type-specific payload before memcpy/complete, and apply
> the same rule to SYNTHVID_FEATURE_CHANGE before reading is_dirt_needed.
> The memcpy then uses bytes_recvd, which is bounded by
> VMBUS_MAX_PACKET_SIZE through the call to vmbus_recvpacket().
>
> Rejected packets are reported via drm_err_ratelimited() rather than
> silently dropped, matching the CoCo-hardened pattern in
> hv_kvp_onchannelcallback().
We discussed several issues with this patch in the feedback
from Sashiko. But see one more issue below.
>
> Fixes: 76c56a5affeb ("drm/hyperv: Add DRM driver for hyperv synthetic video
> device")
> Cc: [email protected] # 5.14+
> Signed-off-by: Berkant Koc
> Assisted-by: Claude:claude-opus-4-7 berkoc-pipeline
> ---
> drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 42 +--
> 1 file changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> index c3d0ff229..12d3feb4f 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)
> {
> 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) {
> + drm_err_ratelimited(&hv->dev,
> + "synthvid packet too small for header:
> %u\n",
> + bytes_recvd);
> + 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);
> + 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);
I'm concerned that this might be too aggressive. The last element
of struct synthvid_supported_resolution_resp is an array, and there's
a count in the message describing how many elements of the array
are populated. But Hyper-V may not (and probably doesn't) include
unpopulated elements in the response message. So "need" is likely
calculated as too large. Are you able to test this in a Hyper-V VM to
confirm?
I think you'll find it necessary to first check that enough bytes
have arrived to read the "resolution_count" field, and then use
that value to calculate "need". There are several other places
in hardened VMBus drivers that use that same two-level
technique. It's a pain, but there's not really any alternative.
Michael
> + 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);
> complete(&hv->wait);
> return;
> }
>
> if (msg->vid_hdr.type == SYNTHVID_FEATURE_CHANGE) {
> + if (bytes_recvd < hdr_size +
> + sizeof(struct synthvid_feature_change)) {
> + drm_err_ratelimited(&hv->dev,
> +
RE: [PATCH v3 2/2] drm/hyperv: validate VMBus packet size in receive callback
From: Berkant Koc Sent: Wednesday, May 20, 2026 6:24 AM > > [email protected] wrote: > > - [Critical] Using `bytes_recvd` for `memcpy()` without checking > > `vmbus_recvpacket()` return value leads to a massive heap buffer > > overflow. > > This one is bounded on this channel. hyperv_connect_vsp() calls > vmbus_open() without setting max_pkt_size, so the inbound ring uses > VMBUS_DEFAULT_MAX_PKT_SIZE (4096) and hv_pkt_iter_first() clamps the > packet length to pkt_buffer_size. bytes_recvd therefore cannot exceed > 4096, well under the 16 KiB recv_buf and init_buf, and > vmbus_recvpacket() does not return -ENOBUFS here, so the memcpy length > stays bounded. Actually, the behavior of vmbus_recvpacket() is more subtle, and the problem pointed out by Sashiko AI is real. I had forgotten about this subtle behavior in my first reply to you on this topic. :-( The incoming message from Hyper-V has a length encoded in it. hv_ringbuffer_read() gets that message length, and if it is larger than the buflen_parameter, -ENOBUFS is returned. But the returned buffer_actual_len is also set to the incoming message length provided by Hyper-V. This allows the caller to realize it didn't provide enough buffer space, and also tells it how much buffer space is needed. hv_pci_onchannelcallback() uses this functionality to retry the receive operation with a larger buffer. I think all the other callers just treat -ENOBUFS as a fatal error, which is also fine. > > I will still gate the dispatch on a successful vmbus_recvpacket() > return in the next revision, as defense in depth, so the bound is > local instead of relying on the ring clamp. Indeed, the error check is needed, and it's not just defense in depth. > > > - [High] Strict sizeof() validation incorrectly rejects > > dynamically-sized SYNTHVID_RESOLUTION_RESPONSE packets. > > Agreed. The response carries resolution_count entries, not the full > SYNTHVID_MAX_RESOLUTION_COUNT array, so checking against > sizeof(struct synthvid_supported_resolution_resp) is too strict. The > next revision validates the fixed prefix, reads and bounds > resolution_count, then requires only the count-sized array. OK, good. > > > - [High] Concurrent lockless write to `hv->init_buf` from VMBus > > callback allows a malicious host to overwrite data while the guest > > is validating it. I don't think this actually happens. In hv_pkt_iter_first(), the message is copied out of the ring buffer into a temporary buffer that is not explicitly shared with the host. This temporary copy prevents a malicious host from modifying the data while the guest is validating it. > > - [High] Missing `reinit_completion()` before reusing the shared > > `hv->wait` completion object. > > Both pre-existing. On v2 Michael Kelley suggested splitting the > completion reinit into a separate patch on the resume path. The > init_buf reuse sits in the same area, so I plan to send the reinit and > the related response-type handling as a separate follow-up rather than > fold them into this size-validation change. Handling a timeout, and then the host providing a belated response is a really messy problem. There's some mechanism using request IDs in hv_ringbuffer_write() and vmbus_sendpacket_getid() to help match up requests and responses, but my recollection is that even this extra machinery is not 100% foolproof. You may or may not want to go down the path of trying to fix it. :-) A process comment: The emails for v2 and v3 of your patch set are being threaded in an unexpected way. They are showing up as replies under the original v1. See https://lore.kernel.org/linux-hyperv/. The preferred approach is for each version to start a new email thread. The cover letter should start the new thread, and the patches should show up as threaded under the cover letter. Also, don't post a new version more frequently than every 24 hours at a minimum, and there's no problem with waiting 2 to 3 days. The idea is to give people a chance to review and provide comments so that you can batch any changes in response to the feedback. Michael
Re: [PATCH v3 2/2] drm/hyperv: validate VMBus packet size in receive callback
[email protected] wrote: > - [Critical] Using `bytes_recvd` for `memcpy()` without checking > `vmbus_recvpacket()` return value leads to a massive heap buffer > overflow. This one is bounded on this channel. hyperv_connect_vsp() calls vmbus_open() without setting max_pkt_size, so the inbound ring uses VMBUS_DEFAULT_MAX_PKT_SIZE (4096) and hv_pkt_iter_first() clamps the packet length to pkt_buffer_size. bytes_recvd therefore cannot exceed 4096, well under the 16 KiB recv_buf and init_buf, and vmbus_recvpacket() does not return -ENOBUFS here, so the memcpy length stays bounded. I will still gate the dispatch on a successful vmbus_recvpacket() return in the next revision, as defense in depth, so the bound is local instead of relying on the ring clamp. > - [High] Strict sizeof() validation incorrectly rejects > dynamically-sized SYNTHVID_RESOLUTION_RESPONSE packets. Agreed. The response carries resolution_count entries, not the full SYNTHVID_MAX_RESOLUTION_COUNT array, so checking against sizeof(struct synthvid_supported_resolution_resp) is too strict. The next revision validates the fixed prefix, reads and bounds resolution_count, then requires only the count-sized array. > - [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. Both pre-existing. On v2 Michael Kelley suggested splitting the completion reinit into a separate patch on the resume path. The init_buf reuse sits in the same area, so I plan to send the reinit and the related response-type handling as a separate follow-up rather than fold them into this size-validation change. Thanks for the review. Berkant
