Re: [PATCH 1/2] HID: hyperv: validate initial device info bounds

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] Incomplete Vulnerability Fix / Out-of-Bounds Read
- [Medium] Missing Completion / Timeout Hang
- [Low] Dead Code / Redundant Check

Pre-existing issues:
- [High] Race Condition / Use-After-Free
- [High] Out-of-Bounds Access (Read/Write) in Probe Workaround
- [High] Out-of-Bounds Read (Information Leak) in Input Report
--

--- Patch [1]: [PATCH 1/2] HID: hyperv: validate initial device info bounds ---
commit a1876077d0b8322cccf9c09f43d240367749b037
Author: Michael Bommarito <michael.bommarito@gmail.com>
Subject: HID: hyperv: validate initial device info bounds

This commit attempts to fix an out-of-bounds read by passing the received
initial-device-info size into the parser and rejecting descriptor lengths
that exceed the packet.

> diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c
> --- a/drivers/hid/hid-hyperv.c
> +++ b/drivers/hid/hid-hyperv.c
> @@ -171,18 +171,32 @@ static void mousevsc_on_receive_device_info(struct mousevsc_dev *input_device,
> -				struct synthhid_device_info *device_info)
> +					    struct synthhid_device_info *device_info,
> +					    u32 device_info_size)
>  {
>  	int ret = 0;
>  	struct hid_descriptor *desc;
>  	struct mousevsc_prt_msg ack;
> +	size_t desc_offset;
> +	size_t desc_size;
>  
>  	input_device->dev_info_status = -ENOMEM;
>  
> +	if (device_info_size < sizeof(*device_info)) {
> +		input_device->dev_info_status = -EINVAL;
> +		goto cleanup;
> +	}

[Severity: Low]
Is this check necessary? In mousevsc_on_receive(), before calling this
function, the code explicitly verifies pipe_msg->size >=
sizeof(struct synthhid_device_info). Since device_info_size is passed
directly from pipe_msg->size, this inner check appears to be unreachable.

> +
>  	input_device->hid_dev_info = device_info->hid_dev_info;
>  	desc = &device_info->hid_descriptor;
> +	desc_offset = offsetof(struct synthhid_device_info, hid_descriptor);
> +	desc_size = device_info_size - desc_offset;
>  	if (desc->bLength == 0)
>  		goto cleanup;
> +	if (desc->bLength < sizeof(*desc) || desc->bLength > desc_size) {
> +		input_device->dev_info_status = -EINVAL;
> +		goto cleanup;
> +	}
>  
>  	/* The pointer is not NULL when we resume from hibernation */
>  	kfree(input_device->hid_desc);
> @@ -197,6 +211,10 @@ static void mousevsc_on_receive_device_info(struct mousevsc_dev *input_device,
>  		input_device->dev_info_status = -EINVAL;
>  		goto cleanup;
>  	}
> +	if (input_device->report_desc_size > desc_size - desc->bLength) {
> +		input_device->dev_info_status = -EINVAL;
> +		goto cleanup;
> +	}
>  
>  	/* The pointer is not NULL when we resume from hibernation */
>  	kfree(input_device->report_desc);

[Severity: High]
This is a pre-existing issue, but can this lead to a use-after-free?

During mousevsc_probe(), the driver waits for the first
SYNTH_HID_INITIAL_DEVICE_INFO and proceeds to hid_add_device(), which parses
input_dev->report_desc.

If the host concurrently sends a second SYNTH_HID_INITIAL_DEVICE_INFO packet,
the channel callback processes it asynchronously and calls
mousevsc_on_receive_device_info(). This executes
kfree(input_device->report_desc) without any locking while the HID core is
actively reading it.

[ ... ]
> @@ -273,14 +291,17 @@ static void mousevsc_on_receive(struct hv_device *device,
>  		break;
>  
>  	case SYNTH_HID_INITIAL_DEVICE_INFO:
> -		WARN_ON(pipe_msg->size < sizeof(struct hv_input_dev_info));
> +		if (WARN_ON_ONCE(pipe_msg->size <
> +				 sizeof(struct synthhid_device_info)))
> +			break;

[Severity: Medium]
If this break is taken, are we skipping the completion event?

By breaking out of the switch statement early, we skip the call to
mousevsc_on_receive_device_info(). As a result, the
complete(&input_device->wait_event) inside its cleanup path is never
executed. This might cause the waiting thread in mousevsc_connect_to_vsp()
to stall until the 5-second timeout expires.

>  
>  		/*
>  		 * Parse out the device info into device attr,
>  		 * hid desc and report desc
>  		 */
>  		mousevsc_on_receive_device_info(input_dev,
> -			(struct synthhid_device_info *)pipe_msg->data);
> +						(struct synthhid_device_info *)pipe_msg->data,
> +						pipe_msg->size);

[Severity: High]
Does this fully prevent out-of-bounds reads if the host sends a malicious
payload?

The pipe_msg->size field comes from the untrusted payload. If a malicious
host sends a short physical VMBus packet but sets pipe_msg->size to an
artificially large value, the bounds checks in
mousevsc_on_receive_device_info() will pass because they are evaluated
against this large device_info_size.

Could we restrict the size to the actual VMBus packet boundary, perhaps using
something like (packet->len8 - packet->offset8) << 3, to ensure we don't
read past the received packet?

>  		break;
>  	case SYNTH_HID_INPUT_REPORT:
>  		input_report =
> 			(struct synthhid_input_report *)pipe_msg->data;
> 		if (!input_dev->init_complete)
> 			break;
> 
> 		len = min(input_report->header.size,
> 			  (u32)sizeof(input_dev->input_buf));
> 		memcpy(input_dev->input_buf, input_report->buffer, len);

[Severity: High]
This is a pre-existing issue, but does this blind trust of
input_report->header.size enable an out-of-bounds read?

When handling SYNTH_HID_INPUT_REPORT, a malicious host could send a
physically short packet but set input_report->header.size to 4096.

The memcpy length is bounded using:

len = min(input_report->header.size, (u32)sizeof(input_dev->input_buf));

However, it isn't validated against the actual VMBus packet envelope. This
could result in copying beyond the packet boundary, potentially causing an
access violation or exposing memory adjacent to the packet.


[Severity: High]
This is a pre-existing issue, but I noticed another potential out-of-bounds
access during the probe workaround in mousevsc_probe():

drivers/hid/hid-hyperv.c:mousevsc_probe() {
    ...
    if (input_dev->report_desc[14] == 0x25)
        input_dev->report_desc[14] = 0x29;
    ...
}

If a malicious host provides a wDescriptorLength of less than 15 bytes
(for instance, 8 bytes), this accesses and potentially writes to memory past
the dynamically allocated report_desc buffer. Should we ensure the buffer is
at least 15 bytes long before accessing index 14?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710022854.3739558-1-michael.bommarito@gmail.com?part=1