public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v5 1/2] drm/hyperv: validate resolution_count and fix WIN8 fallback
  2026-05-23 13:27 [PATCH v5 0/2] drm/hyperv: harden host message parsing Berkant Koc
@ 2026-05-19 20:08 ` Berkant Koc
  2026-05-23 15:16   ` Michael Kelley
  2026-05-25  7:45   ` Claude review: " Claude Code Review Bot
  2026-05-23 13:27 ` [PATCH v5 2/2] drm/hyperv: validate VMBus packet size in receive callback Berkant Koc
  2026-05-25  7:45 ` Claude review: drm/hyperv: harden host message parsing Claude Code Review Bot
  2 siblings, 2 replies; 9+ messages in thread
From: Berkant Koc @ 2026-05-19 20:08 UTC (permalink / raw)
  To: Saurabh Sengar, Dexuan Cui, Long Li
  Cc: linux-hyperv, dri-devel, linux-kernel, K. Y. Srinivasan,
	Haiyang Zhang, Wei Liu, Michael Kelley, Thomas Zimmermann,
	Maarten Lankhorst, Maxime Ripard, Deepak Rawat

A SYNTHVID_RESOLUTION_RESPONSE with resolution_count > 64 walks past
the supported_resolution[SYNTHVID_MAX_RESOLUTION_COUNT] array in the
parse loop. Bound resolution_count against the array size, folded
into the existing zero-check.

When the WIN10 resolution probe fails, the caller in
hyperv_connect_vsp() left hv->screen_*_max / preferred_* unpopulated,
which sets mode_config.max_width / max_height to 0 and makes
drm_internal_framebuffer_create() reject every userspace framebuffer
with -EINVAL. The pre-WIN10 branch had the same gap for
preferred_width / preferred_height. Use a single post-probe fallback
guarded by screen_width_max == 0 so both paths converge on the WIN8
defaults.

Signed-off-by: Berkant Koc <me@berkoc.com>
Assisted-by: Claude:claude-opus-4-7 berkoc-pipeline
Fixes: 76c56a5affeb ("drm/hyperv: Add DRM driver for hyperv synthetic video device")
Cc: stable@vger.kernel.org # 5.14+
Reviewed-by: Michael Kelley <mhklinux@outlook.com>
---
 drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
index 051ecc526..c3d0ff229 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;
 	}
 
-	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) {
+		drm_err(dev, "Invalid resolution count: %d\n",
+			msg->resolution_resp.resolution_count);
 		return -ENODEV;
 	}
 
@@ -508,9 +511,13 @@ int hyperv_connect_vsp(struct hv_device *hdev)
 		ret = hyperv_get_supported_resolution(hdev);
 		if (ret)
 			drm_err(dev, "Failed to get supported resolution from host, use default\n");
-	} else {
+	}
+
+	if (!hv->screen_width_max) {
 		hv->screen_width_max = SYNTHVID_WIDTH_WIN8;
 		hv->screen_height_max = SYNTHVID_HEIGHT_WIN8;
+		hv->preferred_width = SYNTHVID_WIDTH_WIN8;
+		hv->preferred_height = SYNTHVID_HEIGHT_WIN8;
 	}
 
 	hv->mmio_megabytes = hdev->channel->offermsg.offer.mmio_megabytes;
-- 
2.47.3


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v5 2/2] drm/hyperv: validate VMBus packet size in receive callback
  2026-05-23 13:27 [PATCH v5 0/2] drm/hyperv: harden host message parsing Berkant Koc
  2026-05-19 20:08 ` [PATCH v5 1/2] drm/hyperv: validate resolution_count and fix WIN8 fallback Berkant Koc
@ 2026-05-23 13:27 ` Berkant Koc
  2026-05-23 15:17   ` Michael Kelley
  2026-05-25  7:45   ` Claude review: " Claude Code Review Bot
  2026-05-25  7:45 ` Claude review: drm/hyperv: harden host message parsing Claude Code Review Bot
  2 siblings, 2 replies; 9+ messages in thread
From: Berkant Koc @ 2026-05-23 13:27 UTC (permalink / raw)
  To: Saurabh Sengar, Dexuan Cui, Long Li
  Cc: linux-hyperv, dri-devel, linux-kernel, K. Y. Srinivasan,
	Haiyang Zhang, Wei Liu, Michael Kelley, Thomas Zimmermann,
	Maarten Lankhorst, Maxime Ripard, Deepak Rawat

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@vger.kernel.org # 5.14+
Signed-off-by: Berkant Koc <me@berkoc.com>
Assisted-by: Claude:claude-opus-4-7 berkoc-pipeline
---
 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v5 0/2] drm/hyperv: harden host message parsing
@ 2026-05-23 13:27 Berkant Koc
  2026-05-19 20:08 ` [PATCH v5 1/2] drm/hyperv: validate resolution_count and fix WIN8 fallback Berkant Koc
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Berkant Koc @ 2026-05-23 13:27 UTC (permalink / raw)
  To: Saurabh Sengar, Dexuan Cui, Long Li
  Cc: linux-hyperv, dri-devel, linux-kernel, K. Y. Srinivasan,
	Haiyang Zhang, Wei Liu, Michael Kelley, Thomas Zimmermann,
	Maarten Lankhorst, Maxime Ripard, Deepak Rawat

Two independent issues in the synthetic video driver that both stem
from trusting unvalidated host data.

1/2 bounds resolution_count from SYNTHVID_RESOLUTION_RESPONSE against
the supported_resolution[] array, and populates WIN8 defaults for
hv->screen_*_max / hv->preferred_* in both the WIN10-probe-failure
path and the pre-WIN10 path, so a failed or pre-WIN10 probe yields a
usable display instead of having drm_internal_framebuffer_create()
reject every userspace framebuffer with -EINVAL.

2/2 forwards bytes_recvd from vmbus_recvpacket() into the sub-handler,
rejects packets that do not cover the synthvid header, and requires
the type-specific payload size before memcpy/complete or before
reading the feature-change byte. Rejected packets are logged via
drm_err_ratelimited() instead of being silently dropped, matching the
CoCo-hardened pattern in hv_kvp_onchannelcallback().

1/2 is unchanged from v3/v4 and carries Michael Kelley's Reviewed-by.

Changes since v4 (per review by Michael Kelley):

  2/2: collapsed the leading "if (type == ... ) { ... switch ... }"
       plus the separate "if (type == FEATURE_CHANGE)" into a single
       switch on msg->vid_hdr.type. The three completion-driving cases
       compute their per-type size and break to a shared exit that does
       the size check + memcpy(init_buf) + complete(); FEATURE_CHANGE is
       its own case that validates its payload and returns without
       falling through; unknown types hit default and return. No
       functional change, fewer lines.

  2/2: the vmbus_recvpacket() nonzero-return path (e.g. -ENOBUFS for a
       too-big packet) is itself a malformed-message case. It is now
       logged via drm_err_ratelimited(), consistent with the
       sub-handler's other reject paths, instead of being silently
       skipped. No channel recovery is attempted, as that is not worth
       the added code for this rare host-side condition.

Changes since v3 (per review by Michael Kelley):

  2/2: validate SYNTHVID_RESOLUTION_RESPONSE against its actual
       variable length. The response carries resolution_count entries,
       not the full SYNTHVID_MAX_RESOLUTION_COUNT array, so requiring
       sizeof(struct synthvid_supported_resolution_resp) rejected the
       shorter responses the host legitimately sends and broke
       resolution probing. Require the fixed prefix, read and bound
       resolution_count, then require only the count-sized array.

  2/2: only run hyperv_receive_sub() when vmbus_recvpacket() returned
       success. v3 dropped the bytes_recvd upper bound as redundant,
       which holds only on a successful receive: on -ENOBUFS
       vmbus_recvpacket() reports the required length in bytes_recvd,
       which can exceed the 16 KiB hv->recv_buf, and the subsequent
       memcpy(hv->init_buf, msg, bytes_recvd) would read and write
       past both buffers. Gating on the success return restores the
       invariant that made the bound redundant, so an oversized host
       packet is dropped rather than copied.

Changes since v2 (per review by Michael Kelley):

  1/2: dropped the reinit_completion() change; the stale completion can
       only outlive its request in hyperv_vmbus_resume() after a
       get_supported_resolution() timeout, which is a narrower fix that
       belongs in a separate patch against the resume path. Pre-WIN10
       branch now also populates hv->preferred_*. The else branch is
       gone; a single screen_width_max == 0 check covers both the
       pre-WIN10 case and a failed WIN10 probe.

  2/2: added a per-type switch for the three completion-driving message
       types so the wait-completion path validates payload size before
       memcpy/complete. Every reject path emits drm_err_ratelimited()
       rather than returning silently.

Changes since v1:

  1/2: bound resolution_count check folded into the existing zero check;
       populate WIN8 defaults when hyperv_get_supported_resolution()
       fails.
  2/2: forward bytes_recvd into hyperv_receive_sub(); enforce the pipe +
       synthvid header minimum; check synthvid_feature_change payload
       size before reading is_dirt_needed.

The shared init_buf reuse (a duplicate or late host response can
overwrite init_buf between successive request/response cycles) and the
related completion reinit are real but orthogonal to this size
validation. As discussed on v2, they are queued as a separate follow-up
against the resume/expected-type path once this series lands.

This series is verified by static analysis and code inspection against
the synthvid protocol structures and the vmbus_recvpacket() contract. I
do not currently have a Hyper-V test environment to exercise the receive
and resolution-probe paths at runtime, so confirmation from someone who
can run it in a Hyper-V VM would be welcome.

Both patches carry an Assisted-by: Claude:claude-opus-4-7 berkoc-pipeline
trailer per the kernel coding-assistants policy. Code, analysis and
review responses are mine; the model is used as a structured reviewer
under human verification.

Berkant Koc (2):
  drm/hyperv: validate resolution_count and fix WIN8 fallback
  drm/hyperv: validate VMBus packet size in receive callback

 drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 113 +++++++++++++++++++---
 1 file changed, 97 insertions(+), 16 deletions(-)


base-commit: 4bf5d3da79c48e1df4bab82c9680c53adeff7820
-- 
2.47.3


^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH v5 1/2] drm/hyperv: validate resolution_count and fix WIN8 fallback
  2026-05-19 20:08 ` [PATCH v5 1/2] drm/hyperv: validate resolution_count and fix WIN8 fallback Berkant Koc
@ 2026-05-23 15:16   ` Michael Kelley
  2026-05-25  7:45   ` Claude review: " Claude Code Review Bot
  1 sibling, 0 replies; 9+ messages in thread
From: Michael Kelley @ 2026-05-23 15:16 UTC (permalink / raw)
  To: Berkant Koc, Saurabh Sengar, Dexuan Cui, Long Li
  Cc: linux-hyperv@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, K. Y. Srinivasan, Haiyang Zhang,
	Wei Liu, Michael Kelley, Thomas Zimmermann, Maarten Lankhorst,
	Maxime Ripard, Deepak Rawat

From: Berkant Koc <me@berkoc.com> Sent: Tuesday, May 19, 2026 1:08 PM
> 
> A SYNTHVID_RESOLUTION_RESPONSE with resolution_count > 64 walks past
> the supported_resolution[SYNTHVID_MAX_RESOLUTION_COUNT] array in the
> parse loop. Bound resolution_count against the array size, folded
> into the existing zero-check.
> 
> When the WIN10 resolution probe fails, the caller in
> hyperv_connect_vsp() left hv->screen_*_max / preferred_* unpopulated,
> which sets mode_config.max_width / max_height to 0 and makes
> drm_internal_framebuffer_create() reject every userspace framebuffer
> with -EINVAL. The pre-WIN10 branch had the same gap for
> preferred_width / preferred_height. Use a single post-probe fallback
> guarded by screen_width_max == 0 so both paths converge on the WIN8
> defaults.
> 
> Signed-off-by: Berkant Koc <me@berkoc.com>
> Assisted-by: Claude:claude-opus-4-7 berkoc-pipeline
> Fixes: 76c56a5affeb ("drm/hyperv: Add DRM driver for hyperv synthetic video device")
> Cc: stable@vger.kernel.org # 5.14+
> Reviewed-by: Michael Kelley <mhklinux@outlook.com>

I 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 resolution_count from Hyper-V is
valid. I did not simulate a bogus resolution_count and ensure
it is detected.

Tested-by: Michael Kelley <mhklinux@outlook.com>  

> ---
>  drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> index 051ecc526..c3d0ff229 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;
>  	}
> 
> -	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) {
> +		drm_err(dev, "Invalid resolution count: %d\n",
> +			msg->resolution_resp.resolution_count);
>  		return -ENODEV;
>  	}
> 
> @@ -508,9 +511,13 @@ int hyperv_connect_vsp(struct hv_device *hdev)
>  		ret = hyperv_get_supported_resolution(hdev);
>  		if (ret)
>  			drm_err(dev, "Failed to get supported resolution from host, use default\n");
> -	} else {
> +	}
> +
> +	if (!hv->screen_width_max) {
>  		hv->screen_width_max = SYNTHVID_WIDTH_WIN8;
>  		hv->screen_height_max = SYNTHVID_HEIGHT_WIN8;
> +		hv->preferred_width = SYNTHVID_WIDTH_WIN8;
> +		hv->preferred_height = SYNTHVID_HEIGHT_WIN8;
>  	}
> 
>  	hv->mmio_megabytes = hdev->channel->offermsg.offer.mmio_megabytes;
> --
> 2.47.3
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH v5 2/2] drm/hyperv: validate VMBus packet size in receive callback
  2026-05-23 13:27 ` [PATCH v5 2/2] drm/hyperv: validate VMBus packet size in receive callback Berkant Koc
@ 2026-05-23 15:17   ` Michael Kelley
  2026-05-25  7:45   ` Claude review: " Claude Code Review Bot
  1 sibling, 0 replies; 9+ messages in thread
From: Michael Kelley @ 2026-05-23 15:17 UTC (permalink / raw)
  To: Berkant Koc, Saurabh Sengar, Dexuan Cui, Long Li
  Cc: linux-hyperv@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, K. Y. Srinivasan, Haiyang Zhang,
	Wei Liu, Michael Kelley, Thomas Zimmermann, Maarten Lankhorst,
	Maxime Ripard, Deepak Rawat

From: Berkant Koc <me@berkoc.com> 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@vger.kernel.org # 5.14+
> Signed-off-by: Berkant Koc <me@berkoc.com>
> 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@outlook.com>
Tested-by: Michael Kelley <mhklinux@outlook.com>  

> ---
>  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
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Claude review: drm/hyperv: harden host message parsing
  2026-05-23 13:27 [PATCH v5 0/2] drm/hyperv: harden host message parsing Berkant Koc
  2026-05-19 20:08 ` [PATCH v5 1/2] drm/hyperv: validate resolution_count and fix WIN8 fallback Berkant Koc
  2026-05-23 13:27 ` [PATCH v5 2/2] drm/hyperv: validate VMBus packet size in receive callback Berkant Koc
@ 2026-05-25  7:45 ` Claude Code Review Bot
  2 siblings, 0 replies; 9+ messages in thread
From: Claude Code Review Bot @ 2026-05-25  7:45 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/hyperv: harden host message parsing
Author: Berkant Koc <me@berkoc.com>
Patches: 5
Reviewed: 2026-05-25T17:45:30.405059

---

This is a well-crafted v5 security-hardening series for the Hyper-V synthetic video (synthvid) driver. It addresses two real vulnerabilities stemming from blind trust of host-supplied data: an unbounded `resolution_count` that can walk past a fixed-size array, and a receive callback that processes message payloads without verifying the received byte count covers the fields being read.

Both patches are architecturally sound. Patch 1 is a straightforward bounds check plus fallback fix. Patch 2 is more involved, restructuring the receive handler into a switch-based dispatch with per-type size validation and a shared completion path — the v5 refactor into a single switch is cleaner than the earlier approach. The variable-length handling for `SYNTHVID_RESOLUTION_RESPONSE` is correctly implemented with a two-stage validation (fixed prefix first, then count-sized array). The `memcpy` change from the full 16 KiB constant to `bytes_recvd` is a good improvement that eliminates stale residue leaking into the waiter's view.

The series is suitable for merging. I have a few minor observations but no blocking issues.

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Claude review: drm/hyperv: validate resolution_count and fix WIN8 fallback
  2026-05-19 20:08 ` [PATCH v5 1/2] drm/hyperv: validate resolution_count and fix WIN8 fallback Berkant Koc
  2026-05-23 15:16   ` Michael Kelley
@ 2026-05-25  7:45   ` Claude Code Review Bot
  1 sibling, 0 replies; 9+ messages in thread
From: Claude Code Review Bot @ 2026-05-25  7:45 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Status: Good — no issues found.**

The `resolution_count` bounds check is correct. The parse loop at lines 405–410 of the original iterates `resolution_count` times over `supported_resolution[SYNTHVID_MAX_RESOLUTION_COUNT]` (64 entries), so an unchecked count from the host can cause an out-of-bounds read:

```c
+	if (msg->resolution_resp.resolution_count == 0 ||
+	    msg->resolution_resp.resolution_count >
+	    SYNTHVID_MAX_RESOLUTION_COUNT) {
+		drm_err(dev, "Invalid resolution count: %d\n",
+			msg->resolution_resp.resolution_count);
 		return -ENODEV;
```

Folding the upper bound into the existing zero-check is clean. The `%d` format for a `u8` is fine (promoted to `int`).

The fallback refactor is also correct. The original `else` branch only covered pre-WIN10, leaving `screen_width_max` / `screen_height_max` at zero on a failed WIN10 probe, which cascades into `mode_config.max_width = 0` and breaks framebuffer creation. The new guard:

```c
+	if (!hv->screen_width_max) {
 		hv->screen_width_max = SYNTHVID_WIDTH_WIN8;
 		hv->screen_height_max = SYNTHVID_HEIGHT_WIN8;
+		hv->preferred_width = SYNTHVID_WIDTH_WIN8;
+		hv->preferred_height = SYNTHVID_HEIGHT_WIN8;
 	}
```

This works because `screen_width_max` is zero-initialized (the device struct comes from `devm_drm_dev_alloc` → `kzalloc`), and `hyperv_get_supported_resolution()` only sets it to nonzero on success. The pre-WIN10 path also benefits from gaining the `preferred_width`/`preferred_height` defaults that were previously missing. No concerns.

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Claude review: drm/hyperv: validate VMBus packet size in receive callback
  2026-05-23 13:27 ` [PATCH v5 2/2] drm/hyperv: validate VMBus packet size in receive callback Berkant Koc
  2026-05-23 15:17   ` Michael Kelley
@ 2026-05-25  7:45   ` Claude Code Review Bot
  1 sibling, 0 replies; 9+ messages in thread
From: Claude Code Review Bot @ 2026-05-25  7:45 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Status: Good — minor observations, no blocking issues.**

**Header minimum check:** Correct. The header size computation matches the offset to the union payload in `struct synthvid_msg`:

```c
+	hdr_size = sizeof(struct pipe_msg_hdr) +
+		   sizeof(struct synthvid_msg_hdr);
+	if (bytes_recvd < hdr_size) {
```

`pipe_msg_hdr` is 8 bytes, `synthvid_msg_hdr` is 8 bytes, so `hdr_size = 16`. The cast to `struct synthvid_msg *` is correctly moved after this validation, ensuring `msg->vid_hdr.type` isn't read from an undersized buffer.

**SYNTHVID_VERSION_RESPONSE and SYNTHVID_VRAM_LOCATION_ACK:** Straightforward fixed-size checks. Both correctly add the inner struct size to `need` and fall through to the shared completion path.

**SYNTHVID_RESOLUTION_RESPONSE:** The two-stage validation is well thought out:

```c
+	case SYNTHVID_RESOLUTION_RESPONSE:
+		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, ...);
+			return;
+		}
+		need += msg->resolution_resp.resolution_count *
+			sizeof(struct hvd_screen_info);
+		break;
```

The `offsetof` correctly gives 131 bytes (128 EDID + 1 `resolution_count` + 1 `default_resolution_index` + 1 `is_standard`). The early `break` on undersized packets falls through to the shared path where `bytes_recvd < need` catches and logs it. The `return` on oversized `resolution_count` prevents the completion from firing, which is correct — the waiter will time out and report the failure. The duplicate bounds check with Patch 1's consumer-side check provides defense-in-depth.

**SYNTHVID_FEATURE_CHANGE:** Correctly handled as a self-contained case that validates and returns without falling through to the completion path:

```c
+	case SYNTHVID_FEATURE_CHANGE:
+		if (bytes_recvd < need +
+		    sizeof(struct synthvid_feature_change)) {
```

At this point `need == hdr_size` (16 bytes), plus `sizeof(synthvid_feature_change)` = 4 bytes, for a total of 20 bytes minimum. Correct.

**memcpy change:** The switch from `memcpy(hv->init_buf, msg, VMBUS_MAX_PACKET_SIZE)` to `memcpy(hv->init_buf, msg, bytes_recvd)` is a good improvement. Both buffers are `u8[VMBUS_MAX_PACKET_SIZE]` (0x4000 = 16 KiB), and `bytes_recvd <= VMBUS_MAX_PACKET_SIZE` is guaranteed on a successful `vmbus_recvpacket()` return since that's the buffer size passed. The consumers (`hyperv_negotiate_version`, `hyperv_get_supported_resolution`, `hyperv_update_vram_location`) only access fields within the validated payload extent, so stale bytes beyond `bytes_recvd` in `init_buf` are not a concern.

**vmbus_recvpacket error handling in hyperv_receive():**

```c
+		if (ret) {
+			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);
+		}
```

This fixes a real bug in the original code: the `do { } while` loop body would execute before the loop condition `ret == 0` is tested, so on an error return (e.g., `-ENOBUFS`), the original code would still enter the `if (bytes_recvd > 0 ...)` body, potentially calling `hyperv_receive_sub()` with stale buffer contents and a `bytes_recvd` value representing the *required* length (which could exceed the 16 KiB buffer). The new gating on `ret` prevents this.

**Minor observations (non-blocking):**

1. The `default:` case silently drops unknown message types. A `drm_dbg_ratelimited()` could aid debugging during development, but silent drop is reasonable for production to avoid log noise from potential future message types.

2. The error message `"vmbus_recvpacket failed: %d (need %u)\n"` uses "need" to describe `bytes_recvd` — this is correct on `-ENOBUFS` where `bytes_recvd` reports the required length, but could be slightly confusing for other error codes. Minor nit; the context makes it clear enough.

3. The loop exit on error means any valid packets queued behind a bad one won't be processed in this callback invocation. This is acceptable — on `-ENOBUFS` the oversized packet is stuck in the ring and retrying would just fail again; the callback will be re-triggered by the next vmbus interrupt.

Overall both patches are correct, well-documented, and ready to merge. The commit messages are thorough and the changelog clearly tracks the evolution across versions.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Claude review: drm/hyperv: harden host message parsing
  2026-05-21 20:41 [PATCH v4 0/2] " Berkant Koc
@ 2026-05-25  9:37 ` Claude Code Review Bot
  0 siblings, 0 replies; 9+ messages in thread
From: Claude Code Review Bot @ 2026-05-25  9:37 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/hyperv: harden host message parsing
Author: Berkant Koc <me@berkoc.com>
Patches: 4
Reviewed: 2026-05-25T19:37:10.016122

---

This is a well-crafted v4 defensive hardening series for the Hyper-V synthetic video driver. It addresses two real classes of trust-boundary bugs: (1) an unbounded array walk from an unvalidated host-supplied `resolution_count`, and (2) blind consumption of vmbus packets without checking how many bytes the host actually delivered. The patches are logically independent, well-ordered (patch 1 fixes the consumer, patch 2 fixes the transport layer), and the cover letter demonstrates thorough iteration history addressing previous reviewer feedback.

The series is in good shape. Patch 1 already carries a Reviewed-by from Michael Kelley. Patch 2 has grown significantly across versions but the v4 changes (variable-length resolution validation, `!ret` gating) are important correctness improvements. Both patches are suitable for stable backport.

One minor nit in patch 1 and a few observations in patch 2, but no blocking issues.

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-05-25  9:37 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-23 13:27 [PATCH v5 0/2] drm/hyperv: harden host message parsing Berkant Koc
2026-05-19 20:08 ` [PATCH v5 1/2] drm/hyperv: validate resolution_count and fix WIN8 fallback Berkant Koc
2026-05-23 15:16   ` Michael Kelley
2026-05-25  7:45   ` Claude review: " Claude Code Review Bot
2026-05-23 13:27 ` [PATCH v5 2/2] drm/hyperv: validate VMBus packet size in receive callback Berkant Koc
2026-05-23 15:17   ` Michael Kelley
2026-05-25  7:45   ` Claude review: " Claude Code Review Bot
2026-05-25  7:45 ` Claude review: drm/hyperv: harden host message parsing Claude Code Review Bot
  -- strict thread matches above, loose matches on Subject: below --
2026-05-21 20:41 [PATCH v4 0/2] " Berkant Koc
2026-05-25  9:37 ` Claude review: " Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox