From: Berkant Koc <[email protected]> Sent: Tuesday, May 19, 2026 1:09 PM
> 
> hyperv_receive_sub() reads msg->vid_hdr.type and dispatches into one
> of four message-type branches without knowing how many bytes the host
> wrote into hv->recv_buf. The completion path then runs
> memcpy(hv->init_buf, msg, VMBUS_MAX_PACKET_SIZE), so the consumer
> that wakes on wait_for_completion_timeout() can read up to 16 KiB of
> residue from a prior message as if it were the response payload.
> 
> Pass bytes_recvd into hyperv_receive_sub() and reject any packet that
> does not cover the pipe + synthvid header. For each of the three
> completion-driving types (SYNTHVID_VERSION_RESPONSE,
> SYNTHVID_RESOLUTION_RESPONSE, SYNTHVID_VRAM_LOCATION_ACK) also
> require the type-specific payload before memcpy/complete, and apply
> the same rule to SYNTHVID_FEATURE_CHANGE before reading is_dirt_needed.
> The memcpy then uses bytes_recvd, which is bounded by
> VMBUS_MAX_PACKET_SIZE through the call to vmbus_recvpacket().
> 
> Rejected packets are reported via drm_err_ratelimited() rather than
> silently dropped, matching the CoCo-hardened pattern in
> hv_kvp_onchannelcallback().

We discussed several issues with this patch in the feedback
from Sashiko. But see one more issue below.

> 
> Fixes: 76c56a5affeb ("drm/hyperv: Add DRM driver for hyperv synthetic video 
> device")
> Cc: [email protected] # 5.14+
> Signed-off-by: Berkant Koc <[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(-)
> 
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c 
> b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> index c3d0ff229..12d3feb4f 100644
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> @@ -420,26 +420,62 @@ static int hyperv_get_supported_resolution(struct 
> hv_device *hdev)
>       return 0;
>  }
> 
> -static void hyperv_receive_sub(struct hv_device *hdev)
> +static void hyperv_receive_sub(struct hv_device *hdev, u32 bytes_recvd)
>  {
>       struct hyperv_drm_device *hv = hv_get_drvdata(hdev);
>       struct synthvid_msg *msg;
> +     size_t hdr_size;
> 
>       if (!hv)
>               return;
> 
> +     hdr_size = sizeof(struct pipe_msg_hdr) +
> +                sizeof(struct synthvid_msg_hdr);
> +     if (bytes_recvd < hdr_size) {
> +             drm_err_ratelimited(&hv->dev,
> +                                 "synthvid packet too small for header: 
> %u\n",
> +                                 bytes_recvd);
> +             return;
> +     }
> +
>       msg = (struct synthvid_msg *)hv->recv_buf;
> 
>       /* Complete the wait event */
>       if (msg->vid_hdr.type == SYNTHVID_VERSION_RESPONSE ||
>           msg->vid_hdr.type == SYNTHVID_RESOLUTION_RESPONSE ||
>           msg->vid_hdr.type == SYNTHVID_VRAM_LOCATION_ACK) {
> -             memcpy(hv->init_buf, msg, VMBUS_MAX_PACKET_SIZE);
> +             size_t need = hdr_size;
> +
> +             switch (msg->vid_hdr.type) {
> +             case SYNTHVID_VERSION_RESPONSE:
> +                     need += sizeof(struct synthvid_version_resp);
> +                     break;
> +             case SYNTHVID_RESOLUTION_RESPONSE:
> +                     need += sizeof(struct 
> synthvid_supported_resolution_resp);

I'm concerned that this might be too aggressive.  The last element
of struct synthvid_supported_resolution_resp is an array, and there's
a count in the message describing how many elements of the array
are populated. But Hyper-V may not (and probably doesn't) include
unpopulated elements in the response message.  So "need" is likely
calculated as too large. Are you able to test this in a Hyper-V VM to
confirm?

I think you'll find it necessary to first check that enough bytes
have arrived to read the "resolution_count" field, and then use
that value to calculate "need".  There are several other places
in hardened VMBus drivers that use that same two-level
technique. It's a pain, but there's not really any alternative.

Michael

> +                     break;
> +             case SYNTHVID_VRAM_LOCATION_ACK:
> +                     need += sizeof(struct synthvid_vram_location_ack);
> +                     break;
> +             }
> +             if (bytes_recvd < need) {
> +                     drm_err_ratelimited(&hv->dev,
> +                                         "synthvid packet too small for type 
> %u: %u < %zu\n",
> +                                         msg->vid_hdr.type, bytes_recvd, 
> need);
> +                     return;
> +             }
> +             memcpy(hv->init_buf, msg, bytes_recvd);
>               complete(&hv->wait);
>               return;
>       }
> 
>       if (msg->vid_hdr.type == SYNTHVID_FEATURE_CHANGE) {
> +             if (bytes_recvd < hdr_size +
> +                 sizeof(struct synthvid_feature_change)) {
> +                     drm_err_ratelimited(&hv->dev,
> +                                         "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
> 


Reply via email to