* [PATCH v4 0/2] drm/hyperv: harden host message parsing
@ 2026-05-21 20:41 Berkant Koc
2026-05-19 20:08 ` [PATCH v4 1/2] drm/hyperv: validate resolution_count and fix WIN8 fallback Berkant Koc
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Berkant Koc @ 2026-05-21 20:41 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 and carries Michael Kelley's Reviewed-by.
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 | 76 ++++++++++++++++++++---
1 file changed, 69 insertions(+), 7 deletions(-)
base-commit: 4bf5d3da79c48e1df4bab82c9680c53adeff7820
--
2.47.3
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v4 1/2] drm/hyperv: validate resolution_count and fix WIN8 fallback 2026-05-21 20:41 [PATCH v4 0/2] drm/hyperv: harden host message parsing Berkant Koc @ 2026-05-19 20:08 ` Berkant Koc 2026-05-25 9:37 ` Claude review: " Claude Code Review Bot 2026-05-19 20:08 ` [PATCH v4 2/2] drm/hyperv: validate VMBus packet size in receive callback Berkant Koc 2026-05-25 9:37 ` Claude review: drm/hyperv: harden host message parsing Claude Code Review Bot 2 siblings, 1 reply; 7+ 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] 7+ messages in thread
* Claude review: drm/hyperv: validate resolution_count and fix WIN8 fallback 2026-05-19 20:08 ` [PATCH v4 1/2] drm/hyperv: validate resolution_count and fix WIN8 fallback Berkant Koc @ 2026-05-25 9:37 ` Claude Code Review Bot 0 siblings, 0 replies; 7+ messages in thread From: Claude Code Review Bot @ 2026-05-25 9:37 UTC (permalink / raw) To: dri-devel-reviews Patch Review **Verdict: Good, minor nit.** **Resolution count bounds check** — Correct and necessary. The existing code only checked for zero: ```c if (msg->resolution_resp.resolution_count == 0 || msg->resolution_resp.resolution_count > SYNTHVID_MAX_RESOLUTION_COUNT) { ``` Without this, a malicious or buggy host sending `resolution_count > 64` would cause the loop at line 405 to read past `supported_resolution[SYNTHVID_MAX_RESOLUTION_COUNT]`, which is an OOB read on `hv->init_buf`. Since `init_buf` is a flat `u8[0x4000]` inside `struct hyperv_drm_device`, this reads adjacent struct members rather than unmapped memory, but it's still a data-integrity bug that can corrupt `screen_width_max`/`screen_height_max` with garbage. **WIN8 fallback** — Clean refactoring. The change from: ```c } else { hv->screen_width_max = SYNTHVID_WIDTH_WIN8; hv->screen_height_max = SYNTHVID_HEIGHT_WIN8; } ``` to: ```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 correctly converges three previously-distinct failure paths (pre-WIN10, WIN10 probe failure, WIN10 invalid count) into a single fallback. It also fixes the pre-WIN10 branch that never populated `preferred_width`/`preferred_height`, which would leave them at 0. **Nit:** The error message uses `%d` for `resolution_count` which is `u8`: ```c drm_err(dev, "Invalid resolution count: %d\n", msg->resolution_resp.resolution_count); ``` `%u` would be more semantically precise for an unsigned type, though `%d` works correctly for `u8` values and is common in the kernel. Not worth a respin. --- --- Generated by Claude Code Patch Reviewer ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4 2/2] drm/hyperv: validate VMBus packet size in receive callback 2026-05-21 20:41 [PATCH v4 0/2] drm/hyperv: harden host message parsing Berkant Koc 2026-05-19 20:08 ` [PATCH v4 1/2] drm/hyperv: validate resolution_count and fix WIN8 fallback Berkant Koc @ 2026-05-19 20:08 ` Berkant Koc 2026-05-22 21:09 ` Michael Kelley 2026-05-25 9:37 ` Claude review: " Claude Code Review Bot 2026-05-25 9:37 ` Claude review: drm/hyperv: harden host message parsing Claude Code Review Bot 2 siblings, 2 replies; 7+ 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 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 the three completion-driving types (SYNTHVID_VERSION_RESPONSE, SYNTHVID_RESOLUTION_RESPONSE, SYNTHVID_VRAM_LOCATION_ACK) require the type-specific payload before memcpy/complete, and apply the same rule to SYNTHVID_FEATURE_CHANGE before reading is_dirt_needed. 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. 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 | 63 +++++++++++++++++++++-- 1 file changed, 59 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c index c3d0ff229..48054b607 100644 --- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c +++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c @@ -420,26 +420,81 @@ 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: + /* + * 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; + } + 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, + "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); @@ -464,9 +519,9 @@ 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 && + if (!ret && bytes_recvd > 0 && recv_buf->pipe_hdr.type == PIPE_MSG_DATA) - hyperv_receive_sub(hdev); + hyperv_receive_sub(hdev, bytes_recvd); } while (bytes_recvd > 0 && ret == 0); } -- 2.47.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH v4 2/2] drm/hyperv: validate VMBus packet size in receive callback 2026-05-19 20:08 ` [PATCH v4 2/2] drm/hyperv: validate VMBus packet size in receive callback Berkant Koc @ 2026-05-22 21:09 ` Michael Kelley 2026-05-25 9:37 ` Claude review: " Claude Code Review Bot 1 sibling, 0 replies; 7+ messages in thread From: Michael Kelley @ 2026-05-22 21:09 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: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 the three > completion-driving types (SYNTHVID_VERSION_RESPONSE, > SYNTHVID_RESOLUTION_RESPONSE, SYNTHVID_VRAM_LOCATION_ACK) require the > type-specific payload before memcpy/complete, and apply the same rule > to SYNTHVID_FEATURE_CHANGE before reading is_dirt_needed. > > 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. > > 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 | 63 +++++++++++++++++++++-- > 1 file changed, 59 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c > b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c > index c3d0ff229..48054b607 100644 > --- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c > +++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c > @@ -420,26 +420,81 @@ 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) { Having the combination of the above "if" tests followed by the switch statement on the same value is logical duplication that suggests to me that some code reorganization is appropriate. Consider the following approach: 1) The drop the big "if" statement and make the switch and cases be the main decision point. Include SYNTHVID_FEATURE_CHANGE as a case in the switch statement. 2) The check against "need" followed by the memcpy() and complete() are used by the original three cases in the switch. Make that the normal exit path for the function so that the "break" statements for those three cases still do what you want. 3) For the SYNTHVID_FEATURE_CHANGE case, just do a "return" when done since that case doesn't want the check against "need" or the memcpy()/complete() operations. I haven't coded this up, but I think it should be cleaner and be fewer lines of code. > + 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; > + } > + 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, > + "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); > @@ -464,9 +519,9 @@ 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 && > + if (!ret && bytes_recvd > 0 && This patch is all about detecting malformed messages from Hyper-V, and the ret != 0 case is another example of a malformed message because the message is too big. Since malformed messages are no longer being silently ignored, output a rate limited error message in that case, just like in the other malformed message cases. As Sashiko pointed out, a message that's too big will block receipt of any further messages on this channel, but I don't think it's worth trying to code any kind of recovery. The channel is broken, presumably due to some bug in Hyper-V. The only recovery is for a sysadmin to manually unbind the driver from the synthetic device (which should close the channel), then manually rebind and try to start over again. And then report the problem to the Hyper-V team. :-) > recv_buf->pipe_hdr.type == PIPE_MSG_DATA) > - hyperv_receive_sub(hdev); > + hyperv_receive_sub(hdev, bytes_recvd); > } while (bytes_recvd > 0 && ret == 0); > } > > -- > 2.47.3 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Claude review: drm/hyperv: validate VMBus packet size in receive callback 2026-05-19 20:08 ` [PATCH v4 2/2] drm/hyperv: validate VMBus packet size in receive callback Berkant Koc 2026-05-22 21:09 ` Michael Kelley @ 2026-05-25 9:37 ` Claude Code Review Bot 1 sibling, 0 replies; 7+ messages in thread From: Claude Code Review Bot @ 2026-05-25 9:37 UTC (permalink / raw) To: dri-devel-reviews Patch Review **Verdict: Good. Well-reasoned defense-in-depth.** **Header size check** — Correct. The minimum `hdr_size` of `sizeof(struct pipe_msg_hdr) + sizeof(struct synthvid_msg_hdr)` = 16 bytes ensures `msg->vid_hdr.type` is safe to read before the type dispatch: ```c 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; } ``` **Per-type payload validation** — The switch inside the completion block is the right structure. Each response type gets its own minimum size: - `SYNTHVID_VERSION_RESPONSE`: `hdr_size + sizeof(struct synthvid_version_resp)` = 22 bytes - `SYNTHVID_VRAM_LOCATION_ACK`: `hdr_size + sizeof(struct synthvid_vram_location_ack)` = 24 bytes - `SYNTHVID_FEATURE_CHANGE`: `hdr_size + sizeof(struct synthvid_feature_change)` = 20 bytes **Variable-length SYNTHVID_RESOLUTION_RESPONSE** — This is the most complex case and it's handled well. The two-step validation is correct: ```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(...); return; } need += msg->resolution_resp.resolution_count * sizeof(struct hvd_screen_info); break; ``` First validates the fixed prefix (EDID block + count/index/is_standard = 131 bytes), then reads `resolution_count`, bounds it, and requires exactly the count-sized array. This avoids the v3 bug where requiring the full `sizeof(struct synthvid_supported_resolution_resp)` (= 387 bytes) would reject the shorter responses the host legitimately sends when fewer than 64 resolutions are reported. **memcpy size change** — Critical improvement: ```c // old: copies full 16 KiB regardless of actual data memcpy(hv->init_buf, msg, VMBUS_MAX_PACKET_SIZE); // new: copies only received data memcpy(hv->init_buf, msg, bytes_recvd); ``` This eliminates the stale-data leak where `hyperv_get_supported_resolution()` and other consumers could read residual data from a prior message in `init_buf` beyond `bytes_recvd`. Combined with patch 1's bounds check on `resolution_count`, this closes the window where a short response combined with stale `init_buf` content could produce corrupt resolution data. **`!ret` gating** — Essential with the new `memcpy(... bytes_recvd)`: ```c if (!ret && bytes_recvd > 0 && recv_buf->pipe_hdr.type == PIPE_MSG_DATA) hyperv_receive_sub(hdev, bytes_recvd); ``` On `vmbus_recvpacket()` returning `-ENOBUFS`, `bytes_recvd` contains the *required* buffer size, which can exceed `VMBUS_MAX_PACKET_SIZE`. Without the `!ret` gate, the old `VMBUS_MAX_PACKET_SIZE` memcpy was safe (both buffers are that size), but the new `bytes_recvd` memcpy would overflow both `recv_buf` and `init_buf`. The cover letter's analysis of this is accurate. **Observation on completion timeout behavior:** When `hyperv_receive_sub()` rejects a packet (e.g., resolution count too large), it returns without calling `complete()`. The corresponding `wait_for_completion_timeout()` in `hyperv_get_supported_resolution()` / `hyperv_negotiate_version()` / `hyperv_update_vram_location()` will eventually time out and return `-ETIMEDOUT`. This is the correct degraded behavior — a corrupt response should not be treated as valid. Patch 1's fallback ensures the driver still gets usable WIN8 defaults. **One subtlety worth noting:** The `need` variable is declared as `size_t` inside the `if` block, and `bytes_recvd` is `u32`. The comparison `bytes_recvd < need` promotes `bytes_recvd` to `size_t`, which is safe since `u32` → `size_t` is a widening conversion. No issue here. Overall, this is a solid hardening series that follows good defensive coding patterns. Both patches are ready for merge. --- Generated by Claude Code Patch Reviewer ^ permalink raw reply [flat|nested] 7+ messages in thread
* Claude review: drm/hyperv: harden host message parsing 2026-05-21 20:41 [PATCH v4 0/2] drm/hyperv: harden host message parsing Berkant Koc 2026-05-19 20:08 ` [PATCH v4 1/2] drm/hyperv: validate resolution_count and fix WIN8 fallback Berkant Koc 2026-05-19 20:08 ` [PATCH v4 2/2] drm/hyperv: validate VMBus packet size in receive callback Berkant Koc @ 2026-05-25 9:37 ` Claude Code Review Bot 2 siblings, 0 replies; 7+ 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] 7+ messages in thread
end of thread, other threads:[~2026-05-25 9:37 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-05-21 20:41 [PATCH v4 0/2] drm/hyperv: harden host message parsing Berkant Koc 2026-05-19 20:08 ` [PATCH v4 1/2] drm/hyperv: validate resolution_count and fix WIN8 fallback Berkant Koc 2026-05-25 9:37 ` Claude review: " Claude Code Review Bot 2026-05-19 20:08 ` [PATCH v4 2/2] drm/hyperv: validate VMBus packet size in receive callback Berkant Koc 2026-05-22 21:09 ` Michael Kelley 2026-05-25 9:37 ` Claude review: " Claude Code Review Bot 2026-05-25 9:37 ` Claude review: drm/hyperv: harden host message parsing 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