drm/hyperv: harden VMBus message parser input validation
Subsystems (6)
cover drm/hyperv: harden VMBus message parser input validation
The hyperv synthetic video driver parses VMBus messages from the host without bounding two host-controlled values that feed into fixed-size buffers. Both items are input validation, not security bugs: the Hyper-V host sits inside the trusted compute base under the default Hyper-V threat-model. The patches still trim the inputs the driver accepts at face value, matching the trajectory drivers/hv/ has followed for Confidential-VMBus work where the host is no longer fully trusted.
Patch 1 bounds resolution_count against supported_resolution[SYNTHVID_MAX_RESOLUTION_COUNT]; the existing default_resolution_index check is bypassable when both values exceed 64.
Patch 2 forwards bytes_recvd from vmbus_recvpacket() into the sub-handler so that vid_hdr.type and feature_chg.is_dirt_needed are only read once the host actually delivered enough bytes, and so that the init_buf memcpy uses the received length.
Sending as a plain patch series, not a security disclosure. Compile-tested against drm-fixes (6916d5703ddf), static-only.
Berkant Koc (2):
drm/hyperv: validate resolution_count from host VMBus message drm/hyperv: validate VMBus packet size in receive callback
drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)
base-commit: 6916d5703ddf9a38f1f6c2cc793381a24ee914c6
Signature · 1 line
2.47.3
PATCH 1/2 drm/hyperv: validate resolution_count from host VMBus message
The synthetic video device receives a SYNTHVID_RESOLUTION_RESPONSE containing a u8 resolution_count and a u8 default_resolution_index from the host. The existing check rejects resolution_count == 0 and rejects an index that is greater or equal to resolution_count, but does not bound resolution_count itself against the fixed supported_resolution[SYNTHVID_MAX_RESOLUTION_COUNT] array. A host that returns resolution_count > 64 together with an in-range default_resolution_index causes the subsequent loop to read past the array.
Reject any resolution_count that exceeds SYNTHVID_MAX_RESOLUTION_COUNT, folded into the existing zero-check for one log entry per failure. This matches the input-validation pattern used by other VMBus parsers under drivers/hv/ and trims one host-controlled value from the trusted path.
--- drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +5 -2
@@ -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 ; }
Signature · 1 line
2.47.3
[email protected]
AI reviewRe: [PATCH 1/2] drm/hyperv: validate resolution_count from host VMBus message
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Uninitialized State on Error Path Leaves Display Unusable - [High] Missing Completion…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Uninitialized State on Error Path Leaves Display Unusable
- [High] Missing Completion Re-Initialization Breaks Driver Synchronization --
commit 94c5684ae582179bbf9c456d94f6a0bd0f92785f
Author: Berkant Koc <[email protected]>
Subject: drm/hyperv: validate resolution_count from host VMBus message
This patch adds validation to ensure the host-provided resolution_count does not exceed SYNTHVID_MAX_RESOLUTION_COUNT. It folds this check into the existing zero-count check to prevent reading past the end of the supported resolution array.
drivers/gpu/drm/hyperv/hyperv_drm_proto.c static int hyperv_get_supported_resolution(struct hv_device *hdev) · line 392
Quoted 7 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +0
-0@@ -391,8 +391,11 @@ static int hyperv_get_supported_resolution(struct hv_device *hdev)return -ETIMEDOUT ; }
This isn't a bug in your patch, but looking at the wait_for_completion_timeout call just before this hunk, does the driver reuse the hv->wait completion structure for multiple sequential VMBus requests without calling reinit_completion()?
If a previous request times out but the host sends a delayed response, the receive callback might execute complete(&hv->wait). Could a subsequent request's wait_for_completion_timeout incorrectly consume this stray completion, return immediately, and parse stale data from hv->init_buf?
drivers/gpu/drm/hyperv/hyperv_drm_proto.c static int hyperv_get_supported_resolution(struct hv_device *hdev) · line 399
Quoted 9 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +5
-2-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 ;
This is also a pre-existing issue, but I noticed that returning an error here can leave the display unusable.
When hyperv_get_supported_resolution() fails due to this new bounds check, the caller hyperv_connect_vsp() logs a warning to use defaults, but it skips setting up the resolution limits. As a result, hv->screen_width_max and hv->screen_height_max remain 0.
Later, hyperv_mode_config_init() sets dev->mode_config.max_width and max_height to 0. Since the DRM core's drm_internal_framebuffer_create() strictly bounds framebuffer dimensions to dev->mode_config.max_width, would this cause all userspace attempts to create a framebuffer to fail with -EINVAL?
Quoted 2 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +0
-0}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH 2/2 drm/hyperv: validate VMBus packet size in receive callback
In reply to Berkant Kochyperv_receive() reads bytes_recvd from vmbus_recvpacket() but does not forward that value to hyperv_receive_sub(). The sub-handler reads msg->vid_hdr.type and msg->feature_chg.is_dirt_needed without knowing how many bytes the host actually wrote, so a short packet leaves the parser reading bytes that the host did not write in this packet. The unconditional memcpy(hv->init_buf, msg, VMBUS_MAX_PACKET_SIZE) on the wait-completion path also copies the full 16 KiB recv_buf regardless of bytes_recvd, including any residue from the prior message.
Pass bytes_recvd into hyperv_receive_sub() and reject any packet shorter than the pipe + synthvid header. The dirt-feature branch additionally requires the feature_change payload size before reading is_dirt_needed. The init_buf copy now uses bytes_recvd as the length argument, which keeps it bounded by VMBUS_MAX_PACKET_SIZE through the new upper check.
--- drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +12 -3
@@ -420,26 +420,35 @@ 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 || bytes_recvd >VMBUS_MAX_PACKET_SIZE ) +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 ); +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 )) +return ; hv->dirt_needed = msg->feature_chg.is_dirt_needed;if (hv->dirt_needed)hyperv_hide_hw_ptr (hv->hdev);
@@ -466,7 +475,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); }while (bytes_recvd >0 && ret ==0 ); }
Signature · 1 line
2.47.3
[email protected]
AI reviewRe: [PATCH 2/2] drm/hyperv: validate VMBus packet size in receive callback
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Incomplete payload size validation causes wait-completion handlers to read stale request data…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incomplete payload size validation causes wait-completion handlers to read stale request data, allowing response spoofing and invalid state. --
commit ffe1d9057aa46275f644c2e152d0e7a9e8837c3b
Author: Berkant Koc <[email protected]>
drm/hyperv: validate VMBus packet size in receive callback
This commit introduces bytes_recvd into hyperv_receive_sub() to validate packet sizes. It restricts the memcpy length to bytes_recvd and checks payload size for feature change messages.
Quoted 3 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +0
-0
[ ... ]
Quoted 13 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +4
-0@@ -420,26 +420,35 @@ static int hyperv_get_supported_resolution(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 || bytes_recvd >VMBUS_MAX_PACKET_SIZE )
drivers/gpu/drm/hyperv/hyperv_drm_proto.c static int hyperv_get_supported_resolution(struct hv_device *hdev) · line 440
Quoted 10 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +3
-1+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 ); +memcpy (hv->init_buf, msg, bytes_recvd);
Could this change introduce a regression where a short packet leaves stale request data in hv->init_buf?
The patch bounds the memcpy to bytes_recvd, but it doesn't appear to validate that bytes_recvd is large enough to contain the expected response payloads before calling complete(&hv->wait).
Because hv->init_buf is used for both sending requests and receiving responses, a short response might only partially overwrite hv->init_buf. The payload portion would retain the outgoing request's data or zeroes from the pre-send initialization.
For example, when sending a VRAM location update, the driver sets msg->vram.user_ctx = vram_pp. If a short response arrives, the payload is not overwritten. The waiter checks if (msg->vram_ack.user_ctx != vram_pp). Since vram and vram_ack share a union in struct synthvid_msg, this check might pass against the driver's own request data, allowing a truncated packet to spoof a successful ACK.
Similarly, in hyperv_get_supported_resolution(), if a truncated response includes the resolution_count but omits the supported_resolution array, the uncopied array memory remains zero. The driver could parse zeroed dimensions, setting hv->preferred_width and height to 0, which could propagate to the DRM core.
Would it be safer to have the receive handler validate the specific payload sizes for these wait-completion response types, similar to how it validates SYNTHVID_FEATURE_CHANGE below?
Quoted 9 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +3
-0complete (&hv->wait);return ; }if (msg->vid_hdr.type ==SYNTHVID_FEATURE_CHANGE ) { +if (bytes_recvd < hdr_size + +sizeof (struct synthvid_feature_change )) +return ; hv->dirt_needed = msg->feature_chg.is_dirt_needed;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
v2 cover drm/hyperv: harden VMBus message parser input validation
In reply to Berkant Kocv2 folds two further issues into patch 1 that the sashiko-bot review pointed out on v1:
1. The resolution_count bounds check in v1 returned -ENODEV, but hyperv_connect_vsp() only logged a warning and continued without setting hv->screen_width_max / height_max / preferred_*. That left dev->mode_config.max_width and max_height at 0, which made drm_internal_framebuffer_create() reject every userspace framebuffer with -EINVAL. v2 falls back to the WIN8 defaults on that error path, matching the pre-WIN10 branch.
2. The three sequential VSP requests in hyperv_connect_vsp() (negotiate version, update VRAM location, get supported resolution) all wait on the same hv->wait completion without calling reinit_completion() between requests. A delayed complete() after a wait_for_completion_timeout() can leak into the next request and let it parse stale data out of hv->init_buf. v2 calls reinit_completion() before each send.
Patch 2 is unchanged from v1.
v1: https://lore.kernel.org/r/[email protected]
Berkant Koc (2):
drm/hyperv: validate resolution_count and harden VSP request paths drm/hyperv: validate VMBus packet size in receive callback
drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 32 ++++++++++++++++++----- 1 file changed, 26 insertions(+), 6 deletions(-)
base-commit: 6916d5703ddf9a38f1f6c2cc793381a24ee914c6
Signature · 1 line
2.47.3
PATCH v2 1/2 drm/hyperv: validate resolution_count and harden VSP request paths
The synthetic video device parses a SYNTHVID_RESOLUTION_RESPONSE that contains a u8 resolution_count and a u8 default_resolution_index. The existing check rejects resolution_count == 0 and an index greater or equal to resolution_count, but does not bound resolution_count itself against the fixed supported_resolution[SYNTHVID_MAX_RESOLUTION_COUNT] array. A host that returns resolution_count > 64 together with an in-range default_resolution_index causes the subsequent loop to read past the array. Reject any resolution_count that exceeds the array bound, folded into the existing zero-check so a single log entry remains per failure.
When that bounds check (or any later failure in hyperv_get_supported_resolution()) returns an error, the caller in hyperv_connect_vsp() previously logged a warning and continued without populating hv->screen_width_max / hv->screen_height_max / preferred_*. hyperv_mode_config_init() then set dev->mode_config.max_width and max_height to 0, which makes drm_internal_framebuffer_create() reject every userspace framebuffer with -EINVAL. Populate the fields with the WIN8 defaults that the pre-WIN10 branch already uses so a failed resolution probe degrades to a usable display instead of disabling it.
The driver also issues three sequential VSP requests (negotiate version, update VRAM location, get supported resolution) that share a single hv->wait completion. None of the call sites call reinit_completion() between requests. If wait_for_completion_timeout() returns 0 but a delayed response later triggers complete(&hv->wait) in the receive callback, the next request's wait can consume that stale completion, return immediately, and parse stale data out of hv->init_buf. Call reinit_completion() before each send so every request waits for its own response.
--- drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +14 -3
@@ -223,6 +223,7 @@ static int hyperv_negotiate_version(struct hv_device *hdev, u32 ver) msg->vid_hdr.size =sizeof (struct synthvid_msg_hdr ) +sizeof (struct synthvid_version_req ); msg->ver_req.version = ver; +reinit_completion (&hv->wait);hyperv_sendpacket (hdev, msg); t =wait_for_completion_timeout (&hv->wait,VMBUS_VSP_TIMEOUT );
@@ -257,6 +258,7 @@ int hyperv_update_vram_location(struct hv_device *hdev, phys_addr_t vram_pp) msg->vram.user_ctx = vram_pp; msg->vram.vram_gpa = vram_pp; msg->vram.is_vram_gpa_specified =1 ; +reinit_completion (&hv->wait);hyperv_sendpacket (hdev, msg); t =wait_for_completion_timeout (&hv->wait,VMBUS_VSP_TIMEOUT );
@@ -383,6 +385,7 @@ static int hyperv_get_supported_resolution(struct hv_device *hdev)sizeof (struct synthvid_supported_resolution_req ); msg->resolution_req.maximum_resolution_count =SYNTHVID_MAX_RESOLUTION_COUNT ; +reinit_completion (&hv->wait);hyperv_sendpacket (hdev, msg); t =wait_for_completion_timeout (&hv->wait,VMBUS_VSP_TIMEOUT );
@@ -391,8 +394,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 ; }
@@ -506,8 +512,13 @@ int hyperv_connect_vsp(struct hv_device *hdev)if (hyperv_version_ge (hv->synthvid_version,SYNTHVID_VERSION_WIN10 )) { ret =hyperv_get_supported_resolution (hdev); -if (ret) +if (ret) {drm_err (dev,"Failed to get supported resolution from host, use default\n" ); + 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 ; + } }else { hv->screen_width_max =SYNTHVID_WIDTH_WIN8 ; hv->screen_height_max =SYNTHVID_HEIGHT_WIN8 ;
Signature · 1 line
2.47.3
[email protected]
AI reviewRe: [PATCH v2 1/2] drm/hyperv: validate resolution_count and harden VSP request paths
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Critical] The fix for delayed VSP responses is incomplete, leaving a critical data race on the shared…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The fix for delayed VSP responses is incomplete, leaving a critical data race on the shared hv->init_buf that allows type confusion and out-of-bounds memory reads. --
commit 3d049f39a946f7898e25e92a422ae95b066e9989
Author: Berkant Koc <[email protected]>
drm/hyperv: validate resolution_count and harden VSP request paths
This commit adds bounds checking for resolution_count when parsing SYNTHVID_RESOLUTION_RESPONSE. It degrades gracefully to default resolutions on failure, and adds reinit_completion() before VSP requests to prevent consuming stale completions.
Quoted 3 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +0
-0
[ ... ]
drivers/gpu/drm/hyperv/hyperv_drm_proto.c static int hyperv_get_supported_resolution(struct hv_device *hdev) · line 389
Quoted 6 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +1
-0@@ -383,6 +385,7 @@ static int hyperv_get_supported_resolution(struct hv_device *hdev)sizeof (struct synthvid_supported_resolution_req ); msg->resolution_req.maximum_resolution_count =SYNTHVID_MAX_RESOLUTION_COUNT ; +reinit_completion (&hv->wait);hyperv_sendpacket (hdev, msg);
Does this reinit_completion() prevent a data race on the shared hv->init_buf?
If a timed-out request's delayed response arrives while a new request is being constructed, could hyperv_receive_sub() concurrently overwrite hv->init_buf without holding any locks?
Since the buffer is used for both sending and receiving, if a delayed response arrives exactly between the memset(msg, 0) and hyperv_sendpacket(hdev, msg), could msg->vid_hdr.size be overwritten by the host's response?
If a host provides a crafted size, could this cause an integer overflow in totalbytes_towrite within hv_ringbuffer_write(), bypassing ring buffer free-space checks and causing an out-of-bounds read of guest memory into the VMBus ring buffer?
Additionally, if the delayed response triggers complete(&hv->wait), is there a chance the driver parses a mismatched response because it does not validate that msg->vid_hdr.type matches the expected response?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
RE: [PATCH v2 1/2] drm/hyperv: validate resolution_count and harden VSP request paths
In reply to Berkant KocFrom: Berkant Koc <[email protected]> Sent: Sunday, May 17, 2026 7:25 AM
Quoted 11 lines
The synthetic video device parses a
SYNTHVID_RESOLUTION_RESPONSEthat contains a u8resolution_countand a u8default_resolution_index. The existing check rejectsresolution_count== 0 and an index greater or equal toresolution_count, but does not boundresolution_countitself against the fixed supported_resolution[SYNTHVID_MAX_RESOLUTION_COUNT] array. A host that returnsresolution_count> 64 together with an in-rangedefault_resolution_indexcauses the subsequent loop to read past the array. Reject anyresolution_countthat exceeds the array bound, folded into the existing zero-check so a single log entry remains per failure.
I think this is a good fix, but let me provide some background. A core assumption has been that the Hyper-V host is not malicious, and data from the host can be trusted to be well-formed and valid. Linux code interacting with the host does *not* take a paranoid approach and does *not* validate every value received from the host. Existing validation is ad hoc and probably not comprehensive.
A few years ago with the introduction of Confidential Computing and "CoCo VMs" in in Linux, the assumption changed. (Hyper-V calls these "Confidential VMs", or sometimes "Isolated VMs" -- unfortunately, the terminology is a bit scattered.) The host is treated as potentially malicious and Linux code was hardened against bad values passed from the host. But not all drivers for Hyper-V synthetic devices were hardened because CoCo VMs on Hyper-V don't allow all the possible synthetic devices. The Hyper-V synthetic frame buffer is one such disallowed device, so the Hyper-V DRM driver was not hardened. The allowed devices can be seen in the vmbus_devs[] array where .allowed_in_isolation is "true".
All that said, I'm in favor of adding hardening, even if it is done piecemeal. There won't likely be a comprehensive effort to harden the Hyper-V DRM driver unless the frame buffer device becomes available in a CoCo VM.
Quoted 10 lines
When that bounds check (or any later failure in
hyperv_get_supported_resolution()) returns an error, the caller inhyperv_connect_vsp()previously logged a warning and continued without populatinghv->screen_width_max/hv->screen_height_max/ preferred_*.hyperv_mode_config_init()then setdev->mode_config.max_widthandmax_heightto 0, which makesdrm_internal_framebuffer_create()reject every userspace framebuffer with-EINVAL. Populate the fields with the WIN8 defaults that the pre-WIN10 branch already uses so a failed resolution probe degrades to a usable display instead of disabling it.
Yes, this also makes sense.
Quoted 10 lines
The driver also issues three sequential VSP requests (negotiate version, update VRAM location, get supported resolution) that share a single
hv->waitcompletion. None of the call sites callreinit_completion()between requests. Ifwait_for_completion_timeout()returns 0 but a delayed response later triggerscomplete(&hv->wait)in the receive callback, the next request's wait can consume that stale completion, return immediately, and parse stale data out ofhv->init_buf. Callreinit_completion()before each send so every request waits for its own response.
This change should probably be a separate patch, as it's not really related to the bounds checking issue. You could even argue that the bounds check and using the default size values in case of an error should be separate patches, but it's a judgment call and I could go either way. Combining the first two works for me, though someone else might disagree.
All that notwithstanding, I don't think your fix is needed in its current form. If wait_for_completion_timeout() times out in "negotiate version" or "update VRAM location", the code returns -ETIMEDOUT. The caller then bails out and does not send the next message in the sequence. Eventually hyperv_vmbus_probe() fails and the struct hyperv_drm_device memory is freed. A similar thing could happen with hyperv_vmbus_resume(). The memory isn't freed, but if the resume fails and is tried again later, it should be with a fresh copy of the saved memory image. In that case, the completion should be clean for the retry.
The problem case is "get supported resolution", which would leave the completion in a pending state if it times out. This could later affect hyperv_vmbus_resume(). I could see reinit'ing the completion in hyperv_vmbus_resume() to handle this case.
But double-check my thinking on all this. Maybe I'm missing something.
One other comment: In my view, your commit message is a bit too detailed. A commit message should focus on "why" the change, and less on the details of the implementation. Explaining the failure case is fine, but don't recapitulate code that is straightforward and obvious.
Quoted 56 lines
Fixes 76c56a5affeb ("drm/hyperv: Add DRM driver for hyperv synthetic video device")Cc [email protected] # 5.14+Signed-off-by Berkant Koc <[email protected]>--- drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)drivers/gpu/drm/hyperv/hyperv_drm_proto.c +8
-3@@ -223,6 +223,7 @@ static int hyperv_negotiate_version(struct hv_device *hdev, u32 ver) msg->vid_hdr.size =sizeof (struct synthvid_msg_hdr ) +sizeof (struct synthvid_version_req ); msg->ver_req.version = ver; +reinit_completion (&hv->wait);hyperv_sendpacket (hdev, msg); t =wait_for_completion_timeout (&hv->wait,VMBUS_VSP_TIMEOUT );@@ -257,6 +258,7 @@ int hyperv_update_vram_location(struct hv_device *hdev, phys_addr_t vram_pp) msg->vram.user_ctx = vram_pp; msg->vram.vram_gpa = vram_pp; msg->vram.is_vram_gpa_specified =1 ; +reinit_completion (&hv->wait);hyperv_sendpacket (hdev, msg); t =wait_for_completion_timeout (&hv->wait,VMBUS_VSP_TIMEOUT );@@ -383,6 +385,7 @@ static int hyperv_get_supported_resolution(struct hv_device *hdev)sizeof (struct synthvid_supported_resolution_req ); msg->resolution_req.maximum_resolution_count =SYNTHVID_MAX_RESOLUTION_COUNT ; +reinit_completion (&hv->wait);hyperv_sendpacket (hdev, msg); t =wait_for_completion_timeout (&hv->wait,VMBUS_VSP_TIMEOUT );@@ -391,8 +394,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 ; }@@ -506,8 +512,13 @@ int hyperv_connect_vsp(struct hv_device *hdev)if (hyperv_version_ge (hv->synthvid_version,SYNTHVID_VERSION_WIN10 )) { ret =hyperv_get_supported_resolution (hdev); -if (ret)
drivers/gpu/drm/hyperv/hyperv_drm_proto.c int hyperv_connect_vsp(struct hv_device *hdev) · line 524
Quoted 10 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +6
-0+if (ret) {drm_err (dev,"Failed to get supported resolution from host, use default\n" ); + 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 ; + } }else { hv->screen_width_max =SYNTHVID_WIDTH_WIN8 ; hv->screen_height_max =SYNTHVID_HEIGHT_WIN8 ;
Is there a separate problem here in that preferred_width and preferred_height are not set in the pre-WIN10 case? I'm not familiar enough with DRM driver code to know how these preferred values are used. It looks inconsistent to have the two fallback cases be different.
Also, having to duplicate the fallback code is distasteful. Instead of having an "else" clause, maybe have a follow-up test for screen_width_max (or any of the values) being zero as an indicator that they haven't been set, and apply the defaults in that case?
Michael
Re: [PATCH v2 1/2] drm/hyperv: validate resolution_count and harden VSP request paths
Hello Michael,
Thanks for the CoCo context, that lines up with what is in vmbus_devs[] for the framebuffer device. The piecemeal approach is what I am aiming for here.
v3 is on the list and addresses your three concrete points:
Quoted 5 lines
This change should probably be a separate patch, as it's not really related to the bounds checking issue.
[...]
All that notwithstanding, I don't think your fix is needed in its current form.
Dropped from v3. You are right that the negotiate-version and update-vram-location timeouts let hyperv_vmbus_probe() bail out and free the device, so the stale-completion path is only reachable from hyperv_vmbus_resume() after a get_supported_resolution() timeout. That is a narrower fix and belongs in a separate patch against the resume path, which I will send afterwards.
Quoted 2 lines
Is there a separate problem here in that
preferred_widthandpreferred_heightare not set in the pre-WIN10 case?
Yes, separate problem, and I missed it in v2. The pre-WIN10 branch in hyperv_connect_vsp() sets only screen_*_max and leaves preferred_* at zero, which is inconsistent with the WIN10-failure path.
Quoted 3 lines
Also, having to duplicate the fallback code is distasteful. Instead of having an "else" clause, maybe have a follow-up test for
screen_width_max[...] being zero as an indicator [...]
Adopted in v3. The else branch is gone; the WIN10 path runs the probe and the post-probe block applies the WIN8 defaults whenever screen_width_max is still zero. One source of truth, both paths converge on it.
Quoted 1 line
In my view, your commit message is a bit too detailed.
Rewritten in v3. The bounds check and the WIN8 fallback are now one short paragraph each, focused on the "why".
Series: <[email protected]>
The v3 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.
Thanks,
Berkant
PATCH v2 2/2 drm/hyperv: validate VMBus packet size in receive callback
In reply to Berkant Kochyperv_receive() reads bytes_recvd from vmbus_recvpacket() but does not forward that value to hyperv_receive_sub(). The sub-handler reads msg->vid_hdr.type and msg->feature_chg.is_dirt_needed without knowing how many bytes the host actually wrote, so a short packet leaves the parser reading bytes that the host did not write in this packet. The unconditional memcpy(hv->init_buf, msg, VMBUS_MAX_PACKET_SIZE) on the wait-completion path also copies the full 16 KiB recv_buf regardless of bytes_recvd, including any residue from the prior message.
Pass bytes_recvd into hyperv_receive_sub() and reject any packet shorter than the pipe + synthvid header. The dirt-feature branch additionally requires the feature_change payload size before reading is_dirt_needed. The init_buf copy now uses bytes_recvd as the length argument, which keeps it bounded by VMBUS_MAX_PACKET_SIZE through the new upper check.
Unchanged from v1.
--- drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +12 -3
@@ -423,26 +423,35 @@ 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 || bytes_recvd >VMBUS_MAX_PACKET_SIZE ) +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 ); +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 )) +return ; hv->dirt_needed = msg->feature_chg.is_dirt_needed;if (hv->dirt_needed)hyperv_hide_hw_ptr (hv->hdev);
@@ -469,7 +478,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); }while (bytes_recvd >0 && ret ==0 ); }
Signature · 1 line
2.47.3
[email protected]
AI reviewRe: [PATCH v2 2/2] drm/hyperv: validate VMBus packet size in receive callback
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Incomplete validation of synchronous response payload sizes causes state leakage and deterministic…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incomplete validation of synchronous response payload sizes causes state leakage and deterministic false success. --
commit 31b0cdbb563323a78bd6df8c18d507462a03a084
Author: Berkant Koc <[email protected]>
drm/hyperv: validate VMBus packet size in receive callback
This commit updates the hyperv_receive callback to validate the received packet size before processing it. It ensures that the packet contains at least the message headers, passes bytes_recvd to the sub-handler, and uses it to bound the memcpy for synchronous responses.
Quoted 4 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +0
-0
[ ... ]
Quoted 17 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +5
-1@@ -423,26 +423,35 @@ 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 || bytes_recvd >VMBUS_MAX_PACKET_SIZE )
drivers/gpu/drm/hyperv/hyperv_drm_proto.c static int hyperv_get_supported_resolution(struct hv_device *hdev) · line 446
Quoted 10 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +3
-1+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 ); +memcpy (hv->init_buf, msg, bytes_recvd);
Is it possible for a truncated response to incorrectly report success here?
Since hv->init_buf is reused for both sending requests and receiving responses, and their structures alias each other in the synthvid_msg union, a short packet would leave the payload section of hv->init_buf unmodified.
For example, if a truncated SYNTHVID_VRAM_LOCATION_ACK packet contains only headers, memcpy() will only copy the headers. The request's vram.user_ctx and the response's vram_ack.user_ctx reside at the exact same memory offset in the union. Because the payload in hv->init_buf is unmodified, msg->vram_ack.user_ctx will perfectly retain the request's vram_pp value.
The waiting thread checks this with:
if (msg->vram_ack.user_ctx != vram_pp)
Because the memory was unchanged, this check will deterministically evaluate to false, causing the driver to proceed as if the VRAM location was successfully acknowledged despite the malformed packet.
Should this code validate that bytes_recvd is large enough to contain the specific response type's payload before copying and calling complete()?
Quoted 3 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +0
-0complete (&hv->wait);return ; }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
RE: [PATCH v2 2/2] drm/hyperv: validate VMBus packet size in receive callback
In reply to Berkant KocFrom: Berkant Koc <[email protected]> Sent: Sunday, May 17, 2026 7:25 AM
Quoted 6 lines
hyperv_receive()readsbytes_recvdfromvmbus_recvpacket()but does not forward that value tohyperv_receive_sub(). The sub-handler readsmsg->vid_hdr.typeandmsg->feature_chg.is_dirt_neededwithout knowing how many bytes the host actually wrote, so a short packet leaves the parser reading bytes that the host did not write in this packet.
This is a valid concern. Similar patterns have hopefully been fixed in other drivers that were hardened for CoCo VMs.
Quoted 3 lines
The unconditional
memcpy(hv->init_buf, msg, VMBUS_MAX_PACKET_SIZE)on the wait-completion path also copies the full 16 KiBrecv_bufregardless ofbytes_recvd, including any residue from the prior message.
Does copying the full 16 KiB break anything? Or are you flagging as just wasteful activity? This gets to what I previously mentioned about commit messages -- explain "why". Being wasteful is a valid reason, but it can be helpful to future readers to be explicit about the "why".
Quoted 8 lines
Pass
bytes_recvdintohyperv_receive_sub()and reject any packet shorter than the pipe + synthvid header. The dirt-feature branch additionally requires thefeature_changepayload size before readingis_dirt_needed. Theinit_bufcopy now usesbytes_recvdas the length argument, which keeps it bounded byVMBUS_MAX_PACKET_SIZEthrough the new upper check.Unchanged from v1.
Version related comments should go below the "---" following the Signed-off line. They will be visible in the patch emails, but won't get put into the git log when the patch is finally accepted. The Linux kernel doesn't want to clutter the git log with intermediate version information that accumulates as the patch is reviewed and revised.
Quoted 20 lines
Fixes 76c56a5affeb ("drm/hyperv: Add DRM driver for hyperv synthetic video device")Cc [email protected] # 5.14+Signed-off-by Berkant Koc <[email protected]>--- drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)drivers/gpu/drm/hyperv/hyperv_drm_proto.c +1
-1@@ -423,26 +423,35 @@ 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) {
drivers/gpu/drm/hyperv/hyperv_drm_proto.c static int hyperv_get_supported_resolution(struct hv_device *hdev) · line 437
Quoted 10 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +4
-0struct 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 || bytes_recvd >VMBUS_MAX_PACKET_SIZE )
The check against VMBUS_MAX_PACKET_SIZE shouldn't be needed. vmbus_recvpacket() takes VMBUS_MAX_PACKET_SIZE as the max receive size and won't return a bytes_recvd value that is larger.
drivers/gpu/drm/hyperv/hyperv_drm_proto.c static int hyperv_get_supported_resolution(struct hv_device *hdev) · line 438
Quoted 1 line
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +1
-0+return ;
In similar cases in other drivers that have been hardened for CoCo VMs, the code outputs a rate limited error message. That should be done here so the bad received message isn't just ignored. See hv_kvp_onchannelcallback() for example.
drivers/gpu/drm/hyperv/hyperv_drm_proto.c static int hyperv_get_supported_resolution(struct hv_device *hdev) · line 446
Quoted 9 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +2
-1+ 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 ); +memcpy (hv->init_buf, msg, bytes_recvd);
Additional logic is needed here. Each of the three message types in the "if" statement has data beyond just the header. Before doing the memcpy() and complete(), the code should validate that the msg is big enough to contain that expected data. I.e., it needs to do the same as you have done for SYNTHVID_FEATURE_CHANGE. Otherwise, the code that wakes up from the completion
will think that it has valid data to work with, and it may not.
Of course, the problem already exists in the upstream code. But if you are going to make changes to fix the problem, the fix should fully handle situation and not just be a partial fix.
drivers/gpu/drm/hyperv/hyperv_drm_proto.c static int hyperv_get_supported_resolution(struct hv_device *hdev) · line 454
Quoted 8 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +3
-0complete (&hv->wait);return ; }if (msg->vid_hdr.type ==SYNTHVID_FEATURE_CHANGE ) { +if (bytes_recvd < hdr_size + +sizeof (struct synthvid_feature_change )) +return ;
Same here. Output a rate limited error message.
Quoted 15 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +1
-2hv->dirt_needed = msg->feature_chg.is_dirt_needed;if (hv->dirt_needed)hyperv_hide_hw_ptr (hv->hdev);@@ -469,7 +478,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); }while (bytes_recvd >0 && ret ==0 ); } --2.47.3
Re: [PATCH v2 2/2] drm/hyperv: validate VMBus packet size in receive callback
Hello Michael,
Thanks for the thorough review. v3 is on the list and addresses each point:
Quoted 2 lines
Does copying the full 16 KiB break anything? Or are you flagging as just wasteful activity?
It is the residue read that is the actual hazard, not the copy cost:
the consumer that wakes on complete() then reads up to 16 KiB of bytes the host did not write in this packet as if it were the response payload. The v3 commit message now leads with that and treats "wasteful" as a secondary observation.
Quoted 2 lines
Version related comments should go below the "---" following the Signed-off line.
Moved into the cover letter changelog in v3 so it stays out of git log.
Quoted 1 line
The check against
VMBUS_MAX_PACKET_SIZEshouldn't be needed.
Dropped. The v3 check is bytes_recvd < hdr_size only.
Quoted 3 lines
In similar cases in other drivers that have been hardened for CoCo VMs, the code outputs a rate limited error message. [...] See
hv_kvp_onchannelcallback()for example.
Done in v3 via drm_err_ratelimited() on every short-packet path (synthvid header underflow, type-specific payload underflow, feature change underflow). The driver already uses drm_err_ratelimited() in hyperv_sendpacket() for the corresponding send path.
Quoted 4 lines
Additional logic is needed here. Each of the three message types in the "if" statement has data beyond just the header. Before doing the
memcpy()andcomplete(), the code should validate that the msg is big enough to contain that expected data.
Fixed in v3. For the three completion types I now compute the required payload size with a switch on msg->vid_hdr.type and reject the packet before memcpy/complete:
SYNTHVID_VERSION_RESPONSE -> sizeof(struct synthvid_version_resp)
SYNTHVID_RESOLUTION_RESPONSE -> sizeof(struct synthvid_supported_resolution_resp)
SYNTHVID_VRAM_LOCATION_ACK -> sizeof(struct synthvid_vram_location_ack)
The memcpy then uses bytes_recvd, so wait_for_completion_timeout() consumers never see truncated data and never read past what the host wrote.
Series: <[email protected]>
The v3 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.
Thanks,
Berkant
v3 cover drm/hyperv: harden host message parsing
In reply to Berkant KocTwo 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 additionally 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().
Changes since v2 (per review by Michael Kelley):
1/2: dropped the reinit_completion() change. Kelley pointed out that
the negotiate-version and update-vram-location timeouts cause hyperv_vmbus_probe() to fail and free the device, so the stale completion can only outlive its request in hyperv_vmbus_resume() after a get_supported_resolution() timeout. That is a narrower fix and belongs in a separate patch against the resume path. Subject and commit message rewritten to reflect that this patch is now bounds-check + WIN8 fallback only. Pre-WIN10 branch now also populates hv->preferred_* (Kelley spotted the gap). Followed the post-probe-test refactor Kelley suggested: 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: dropped the redundant upper bound on bytes_recvd. Added a
per-type switch for the three completion-driving message types (SYNTHVID_VERSION_RESPONSE, SYNTHVID_RESOLUTION_RESPONSE, SYNTHVID_VRAM_LOCATION_ACK) so the wait-completion path validates payload size before memcpy/complete. Every reject path now emits drm_err_ratelimited() rather than returning silently. Commit message rewritten to lead with the residue read, with "wasteful copy" reframed as the secondary observation.
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.
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.
base-commit: 4bf5d3da79c48e1df4bab82c9680c53adeff7820
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 | 58 ++++++++++++++++++++--- 1 file changed, 52 insertions(+), 6 deletions(-)
--
2.47.3
PATCH v3 1/2 drm/hyperv: validate resolution_count and fix WIN8 fallback
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.
--- drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +10 -3
@@ -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;
Signature · 1 line
2.47.3
[email protected]
AI reviewRe: [PATCH v3 1/2] drm/hyperv: validate resolution_count and fix WIN8 fallback
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Unsynchronized access to the shared hv->init_buf introduces Double Fetch/TOCTOU vulnerabilities, allowing a malicious host to bypass bounds checks and leak kernel memory.
- [High] The driver reuses the hv->wait completion for synchronous messages without re-initializing it, which can cause spurious wakeups and synchronization bypass. --
commit dde0fd11b79967ca7df11b1049abebec363d25a5
Author: Berkant Koc <[email protected]>
drm/hyperv: validate resolution_count and fix WIN8 fallback
Adds bounds checking for resolution_count to prevent walking past the supported_resolution array. It also adds fallback handling to ensure WIN8 defaults are used when the resolution probe fails.
drivers/gpu/drm/hyperv/hyperv_drm_proto.c static int hyperv_get_supported_resolution(struct hv_device *hdev) · line 389
Quoted 8 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +0
-0@@ -387,12 +387,15 @@ static int hyperv_get_supported_resolution(struct hv_device *hdev)hyperv_sendpacket (hdev, msg); t =wait_for_completion_timeout (&hv->wait,VMBUS_VSP_TIMEOUT );
[Severity: High]
This isn't a bug introduced by this patch, but does the driver need to call reinit_completion(&hv->wait) before sending requests?
If a delayed or unsolicited response arrives, hyperv_receive_sub() calls complete(&hv->wait), incrementing its internal counter. When this function is called later, wait_for_completion_timeout() will return immediately without waiting for the actual response.
Could this cause the driver to erroneously parse its own outgoing request buffer as a response?
drivers/gpu/drm/hyperv/hyperv_drm_proto.c static int hyperv_get_supported_resolution(struct hv_device *hdev) · line 396
Quoted 10 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +3
-2if (!t) {drm_err (dev,"Time out on waiting resolution response\n" );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 ) {
[Severity: Critical]
This is a pre-existing issue, but does reading directly from the shared hv->init_buf introduce a TOCTOU or Double Fetch problem here?
Since hv->init_buf is also used by the asynchronous hyperv_receive_sub() callback, could a malicious host send a valid count to pass this check, and then immediately overwrite hv->init_buf with a large value before the subsequent for-loop evaluates i < msg->resolution_resp.resolution_count?
Additionally, when the driver prepares requests earlier in this function (e.g., setting msg->vid_hdr.size), an unsolicited response could overwrite hv->init_buf. Could this trick vmbus_sendpacket() into using a host-controlled size and leaking kernel memory?
Quoted 4 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +2
-0+drm_err (dev,"Invalid resolution count: %d\n" , + msg->resolution_resp.resolution_count);return -ENODEV ; }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
RE: [PATCH v3 1/2] drm/hyperv: validate resolution_count and fix WIN8 fallback
In reply to Berkant KocFrom: Berkant Koc <[email protected]> Sent: Tuesday, May 19, 2026 1:08 PM
Quoted 59 lines
A
SYNTHVID_RESOLUTION_RESPONSEwithresolution_count> 64 walks past the supported_resolution[SYNTHVID_MAX_RESOLUTION_COUNT] array in the parse loop. Boundresolution_countagainst 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_heightto 0 and makesdrm_internal_framebuffer_create()reject every userspace framebuffer with-EINVAL. The pre-WIN10 branch had the same gap forpreferred_width/preferred_height. Use a single post-probe fallback guarded byscreen_width_max== 0 so both paths converge on the WIN8 defaults.Signed-off-by Berkant Koc <[email protected]>Assisted-by Claude:claude-opus-4-7 berkoc-pipelineFixes 76c56a5affeb ("drm/hyperv: Add DRM driver for hyperv synthetic video device")Cc [email protected] # 5.14+--- drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)drivers/gpu/drm/hyperv/hyperv_drm_proto.c +10
-4@@ -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; --
Quoted 2 lines
2.47.3
Looks good to me.
PATCH v3 2/2 drm/hyperv: validate VMBus packet size in receive callback
In reply to Berkant Kochyperv_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().
--- drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 42 +++++++++++++++++++++-- 1 file changed, 39 insertions(+), 3 deletions(-)
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +39 -3
@@ -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 ); +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);
@@ -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); }while (bytes_recvd >0 && ret ==0 ); }
Signature · 1 line
2.47.3
[email protected]
AI reviewRe: [PATCH v3 2/2] drm/hyperv: validate VMBus packet size in receive callback
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
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.
Quoted 4 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +0
-0
[ ... ]
Quoted 7 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +1
-1@@ -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) {
[ ... ]
drivers/gpu/drm/hyperv/hyperv_drm_proto.c static int hyperv_get_supported_resolution(struct hv_device *hdev) · line 454
Quoted 12 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +8
-1if (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?
drivers/gpu/drm/hyperv/hyperv_drm_proto.c static int hyperv_get_supported_resolution(struct hv_device *hdev) · line 466
Quoted 12 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +12
-0+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?
Quoted 1 line
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?
Quoted 2 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +0
-0return ; }
[ ... ]
drivers/gpu/drm/hyperv/hyperv_drm_proto.c static void hyperv_receive(void *ctx) · line 505
Quoted 6 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +1
-1@@ -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.
Quoted 2 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +0
-0}while (bytes_recvd >0 && ret ==0 ); }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Re: [PATCH v3 2/2] drm/hyperv: validate VMBus packet size in receive callback
Quoted 3 lines · [email protected] wrote:
- [Critical] Using
bytes_recvdformemcpy()without checkingvmbus_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.
Quoted 2 lines
- [High] Strict
sizeof()validation incorrectly rejects dynamically-sizedSYNTHVID_RESOLUTION_RESPONSEpackets.
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.
Quoted 5 lines
- [High] Concurrent lockless write to
hv->init_buffrom VMBus callback allows a malicious host to overwrite data while the guest is validating it.
- [High] Missingreinit_completion()before reusing the sharedhv->waitcompletion 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
RE: [PATCH v3 2/2] drm/hyperv: validate VMBus packet size in receive callback
From: Berkant Koc <[email protected]> Sent: Wednesday, May 20, 2026 6:24 AM
Quoted 6 lines
Quoted 3 lines · [email protected] wrote:
- [Critical] Using
bytes_recvdformemcpy()without checkingvmbus_recvpacket()return value leads to a massive heap buffer overflow.
Quoted 7 lines
This one is bounded on this channel.
hyperv_connect_vsp()callsvmbus_open()without settingmax_pkt_size, so the inbound ring usesVMBUS_DEFAULT_MAX_PKT_SIZE(4096) andhv_pkt_iter_first()clamps the packet length topkt_buffer_size.bytes_recvdtherefore cannot exceed 4096, well under the 16 KiBrecv_bufandinit_buf, andvmbus_recvpacket()does not return-ENOBUFShere, 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.
Quoted 4 lines
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.
Quoted 9 lines
Quoted 3 lines
- [High] Strict
sizeof()validation incorrectly rejects dynamically-sizedSYNTHVID_RESOLUTION_RESPONSEpackets.Agreed. The response carries
resolution_countentries, not the fullSYNTHVID_MAX_RESOLUTION_COUNTarray, so checking againstsizeof(struct synthvid_supported_resolution_resp)is too strict. The next revision validates the fixed prefix, reads and boundsresolution_count, then requires only the count-sized array.
OK, good.
Quoted 4 lines
Quoted 4 lines
- [High] Concurrent lockless write to
hv->init_buffrom 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.
Quoted 8 lines
Quoted 2 lines
- [High] Missing
reinit_completion()before reusing the sharedhv->waitcompletion object.Both pre-existing. On v2 Michael Kelley suggested splitting the completion reinit into a separate patch on the resume path. The
init_bufreuse 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
In reply to Berkant KocFrom: Berkant Koc <[email protected]> Sent: Tuesday, May 19, 2026 1:09 PM
Quoted 17 lines
hyperv_receive_sub()readsmsg->vid_hdr.typeand dispatches into one of four message-type branches without knowing how many bytes the host wrote intohv->recv_buf. The completion path then runsmemcpy(hv->init_buf, msg, VMBUS_MAX_PACKET_SIZE), so the consumer that wakes onwait_for_completion_timeout()can read up to 16 KiB of residue from a prior message as if it were the response payload.Pass
bytes_recvdintohyperv_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 toSYNTHVID_FEATURE_CHANGEbefore readingis_dirt_needed. The memcpy then usesbytes_recvd, which is bounded byVMBUS_MAX_PACKET_SIZEthrough the call tovmbus_recvpacket().
Quoted 3 lines
Rejected packets are reported via
drm_err_ratelimited()rather than silently dropped, matching the CoCo-hardened pattern inhv_kvp_onchannelcallback().
We discussed several issues with this patch in the feedback from Sashiko. But see one more issue below.
Quoted 41 lines
Fixes 76c56a5affeb ("drm/hyperv: Add DRM driver for hyperv synthetic video device")Cc [email protected] # 5.14+Signed-off-by Berkant Koc <[email protected]>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(-)drivers/gpu/drm/hyperv/hyperv_drm_proto.c +11
-1@@ -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 ||
drivers/gpu/drm/hyperv/hyperv_drm_proto.c static int hyperv_get_supported_resolution(struct hv_device *hdev) · line 454
Quoted 10 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +8
-1msg->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
Quoted 39 lines
drivers/gpu/drm/hyperv/hyperv_drm_proto.c +20
-2+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);@@ -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); }while (bytes_recvd >0 && ret ==0 ); } --2.47.3