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

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 [off-list ref]
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.

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

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

[ ... ]

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

[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