RE: [PATCH v5 2/2] drm/hyperv: validate VMBus packet size in receive callback

[Michael Kelley]

From: Berkant Koc <me@xxxxxxxxxx> Sent: Saturday, May 23, 2026 6:28 AM
> 
> 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. A single switch on
> msg->vid_hdr.type then computes the type-specific payload size: the
> three completion-driving types (SYNTHVID_VERSION_RESPONSE,
> SYNTHVID_RESOLUTION_RESPONSE, SYNTHVID_VRAM_LOCATION_ACK) fall through
> to a shared exit that requires that size before memcpy/complete, while
> SYNTHVID_FEATURE_CHANGE validates its own payload and returns before
> reading is_dirt_needed. Unknown types are dropped.
> 
> SYNTHVID_RESOLUTION_RESPONSE is variable length: the host fills
> resolution_count entries, not the full SYNTHVID_MAX_RESOLUTION_COUNT
> array. Validate the fixed prefix first so resolution_count can be
> read, bound it against the array, then require only the count-sized
> array, so the shorter responses the host actually sends are accepted.
> 
> Only run the sub-handler when vmbus_recvpacket() returned success. The
> memcpy length is bytes_recvd, which is bounded by VMBUS_MAX_PACKET_SIZE
> only on a successful receive; on -ENOBUFS vmbus_recvpacket() instead
> reports the required length, which can exceed hv->recv_buf, so copying
> bytes_recvd would read and write past the 16 KiB buffers. Gating on the
> success return keeps the copy bounded. The nonzero-return path is itself
> a malformed-message case and is now logged rather than silently skipped;
> channel recovery is not attempted.
> 
> Rejected packets are reported via drm_err_ratelimited() rather than
> silently dropped, matching the CoCo-hardened pattern in
> hv_kvp_onchannelcallback().
> 
> Fixes: 76c56a5affeb ("drm/hyperv: Add DRM driver for hyperv synthetic video device")
> Cc: stable@xxxxxxxxxxxxxxx # 5.14+
> Signed-off-by: Berkant Koc <me@xxxxxxxxxx>
> Assisted-by: Claude:claude-opus-4-7 berkoc-pipeline

This looks good now. The error checking and reporting is robust
and the code is well-structured. Thanks for putting up with my
sometimes picky feedback. :-)

I also ran a basic smoke-test on my local Hyper-V instance. I
can confirm that no error messages or failure were generated
in the "good" case where the messages from Hyper-V are
properly formatted and sized. I did not simulate bad messages
and ensure they are detected.

Reviewed-by: Michael Kelley <mhklinux@xxxxxxxxxxx>
Tested-by: Michael Kelley <mhklinux@xxxxxxxxxxx>  

> ---
>  drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 100 +++++++++++++++++++---
>  1 file changed, 87 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> index c3d0ff229..4e6f703a1 100644
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> @@ -420,30 +420,92 @@ 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;
> +	size_t need;
> 
>  	if (!hv)
>  		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);
> -		complete(&hv->wait);
> +	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;
>  	}
> 
> -	if (msg->vid_hdr.type == SYNTHVID_FEATURE_CHANGE) {
> +	msg = (struct synthvid_msg *)hv->recv_buf;
> +	need = hdr_size;
> +
> +	switch (msg->vid_hdr.type) {
> +	case SYNTHVID_VERSION_RESPONSE:
> +		need += sizeof(struct synthvid_version_resp);
> +		break;
> +	case SYNTHVID_RESOLUTION_RESPONSE:
> +		/*
> +		 * The resolution response is variable length: the host
> +		 * fills resolution_count entries, not the full
> +		 * SYNTHVID_MAX_RESOLUTION_COUNT array. Require the fixed
> +		 * prefix first so resolution_count can be read, then
> +		 * demand exactly the count-sized array.
> +		 */
> +		need += offsetof(struct synthvid_supported_resolution_resp,
> +				 supported_resolution);
> +		if (bytes_recvd < need)
> +			break;
> +		if (msg->resolution_resp.resolution_count >
> +		    SYNTHVID_MAX_RESOLUTION_COUNT) {
> +			drm_err_ratelimited(&hv->dev,
> +					    "synthvid resolution count too large: %u\n",
> +					    msg->resolution_resp.resolution_count);
> +			return;
> +		}
> +		need += msg->resolution_resp.resolution_count *
> +			sizeof(struct hvd_screen_info);
> +		break;
> +	case SYNTHVID_VRAM_LOCATION_ACK:
> +		need += sizeof(struct synthvid_vram_location_ack);
> +		break;
> +	case SYNTHVID_FEATURE_CHANGE:
> +		/*
> +		 * Not a completion-driving message: validate its own payload
> +		 * and consume it here rather than falling through to the
> +		 * memcpy/complete shared by the wait-event responses.
> +		 */
> +		if (bytes_recvd < need +
> +		    sizeof(struct synthvid_feature_change)) {
> +			drm_err_ratelimited(&hv->dev,
> +					    "synthvid feature change packet too small: %u\n",
> +					    bytes_recvd);
> +			return;
> +		}
>  		hv->dirt_needed = msg->feature_chg.is_dirt_needed;
>  		if (hv->dirt_needed)
>  			hyperv_hide_hw_ptr(hv->hdev);
> +		return;
> +	default:
> +		return;
> +	}
> +
> +	/*
> +	 * Shared completion path for the wait-event responses
> +	 * (VERSION_RESPONSE, RESOLUTION_RESPONSE, VRAM_LOCATION_ACK):
> +	 * require the type-specific payload before handing the buffer to
> +	 * the waiter.
> +	 */
> +	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);
>  }
> 
>  static void hyperv_receive(void *ctx)
> @@ -464,9 +526,21 @@ static void hyperv_receive(void *ctx)
>  		ret = vmbus_recvpacket(hdev->channel, recv_buf,
>  				       VMBUS_MAX_PACKET_SIZE,
>  				       &bytes_recvd, &req_id);
> -		if (bytes_recvd > 0 &&
> -		    recv_buf->pipe_hdr.type == PIPE_MSG_DATA)
> -			hyperv_receive_sub(hdev);
> +		if (ret) {
> +			/*
> +			 * A nonzero return (e.g. -ENOBUFS for an oversized
> +			 * packet) is itself a malformed message: bytes_recvd
> +			 * then reports the required length rather than a copied
> +			 * payload, so it must not be forwarded to the
> +			 * sub-handler. Channel recovery is not attempted.
> +			 */
> +			drm_err_ratelimited(&hv->dev,
> +					    "vmbus_recvpacket failed: %d (need %u)\n",
> +					    ret, bytes_recvd);
> +		} else if (bytes_recvd > 0 &&
> +			   recv_buf->pipe_hdr.type == PIPE_MSG_DATA) {
> +			hyperv_receive_sub(hdev, bytes_recvd);
> +		}
>  	} while (bytes_recvd > 0 && ret == 0);
>  }
> 
> --
> 2.47.3
>