Thread (6 messages) 6 messages, 3 authors, 10d ago

Re: [PATCH 2/2] HID: hyperv: add KUnit coverage for device info bounds

From: sashiko-bot@kernel.org
Date: 2026-07-10 02:41:28
Also in: linux-hyperv

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

New issues:
- [Low] The embedded KUnit test will be silently disabled and cannot be compiled when both KUnit and the Hyper-V mouse driver are configured as modules.
- [Low] Test-specific mock logic is injected directly into the production VMBus I/O path.

Pre-existing issues:
- [High] The driver and the newly added bounds checking rely on unvalidated inner payload sizes (`pipe_msg->size`, `input_report->header.size`) instead of validating against the actual physical VMBus transport packet size, exposing the guest to OOB reads.
- [High] Missing bounds check before accessing `report_desc[14]` allows out-of-bounds heap memory corruption.
--
--- Patch [2]: [PATCH 2/2] HID: hyperv: add KUnit coverage for device info bounds ---
commit d676a1b2eb911b6f9e0db4e282e431e24c17d123
Author: Michael Bommarito [off-list ref]
Subject: HID: hyperv: add KUnit coverage for device info bounds

This commit adds KUnit tests for the Hyper-V synthetic HID initial
device-info parser. The tests exercise malformed host-provided HID
descriptors using a KUnit-only ACK bypass.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1215,6 +1215,16 @@ config HID_HYPERV_MOUSE
[ ... ]
+config HID_HYPERV_MOUSE_KUNIT_TEST
+	bool "KUnit tests for Hyper-V mouse driver" if !KUNIT_ALL_TESTS
+	depends on KUNIT && HID_HYPERV_MOUSE
[Severity: Low]
When both KUNIT and HID_HYPERV_MOUSE are configured as modules, will this
regression cause the dependency expression to evaluate to "m", forcing the
boolean config to "n" and silently preventing the test from being enabled for
module builds?

The standard kernel idiom for this is "depends on KUNIT=y || KUNIT=HID_HYPERV_MOUSE".
+	default KUNIT_ALL_TESTS
[ ... ]
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -240,13 +243,18 @@ static void mousevsc_on_receive_device_info(struct mousevsc_dev *input_device,
[ ... ]
-	ret = vmbus_sendpacket(input_device->device->channel,
-			&ack,
-			sizeof(struct pipe_prt_msg) +
-			sizeof(struct synthhid_device_info_ack),
-			(unsigned long)&ack,
-			VM_PKT_DATA_INBAND,
-			VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+	if (IS_ENABLED(CONFIG_HID_HYPERV_MOUSE_KUNIT_TEST) &&
+	    !input_device->device) {
[Severity: Low]
Is this regression injecting test-specific mock logic directly into the
production VMBus I/O path?

Could we instead extract the device-info validation into a pure helper
function that the KUnit tests can call directly, avoiding the need to
conditionally bypass network sends in the core logic?
+		ret = 0;
+	} else {
+		ret = vmbus_sendpacket(input_device->device->channel,
[ ... ]
quoted hunk ↗ jump to hunk
@@ -635,5 +643,100 @@ static void __exit mousevsc_exit(void)
[ ... ]
+static void mousevsc_device_info_valid_descriptor(struct kunit *test)
+{
[ ... ]
+	info->hid_descriptor.bLength = sizeof(struct hid_descriptor);
+	info->hid_descriptor.rpt_desc.wDescriptorLength = cpu_to_le16(4);
[Severity: High]
This is a pre-existing issue, but does this test highlight an incomplete
bounds check in the driver?

The test sets the report descriptor length to 4 bytes. However, looking at
mousevsc_probe(), the driver contains a workaround that accesses index 14:

mousevsc_probe() {
    ...
    /* workaround SA-167 */
    if (input_dev->report_desc[14] == 0x25)
        input_dev->report_desc[14] = 0x29;
    ...
}

If a compromised host provides a valid descriptor with a length between 1 and
14, as tested here, could this result in an out-of-bounds heap memory read
and write since the bounds check in mousevsc_on_receive_device_info() only
ensures the size is greater than zero?
+	report = ((u8 *)&info->hid_descriptor) + info->hid_descriptor.bLength;
[ ... ]
+static void mousevsc_device_info_report_desc_oob(struct kunit *test)
+{
[ ... ]
+	info->hid_descriptor.bLength = sizeof(struct hid_descriptor);
+	info->hid_descriptor.rpt_desc.wDescriptorLength = cpu_to_le16(64);
+	report = ((u8 *)&info->hid_descriptor) + info->hid_descriptor.bLength;
+	memset(report, 0x42, 8);
+
+	mousevsc_on_receive_device_info(input_dev, info, sizeof(*info) + 8);
[Severity: High]
This is a pre-existing issue, but does the bounds validation tested here
rely on an unvalidated inner payload size rather than the physical VMBus
transport packet size?

If a compromised or malicious Hyper-V host sends a VMBus packet with a
massive pipe_msg->size but a small physical payload:

mousevsc_on_receive() {
    ...
    mousevsc_on_receive_device_info(input_dev,
                                    (struct synthhid_device_info *)pipe_msg->data,
                                    pipe_msg->size);
    ...
}

Could mousevsc_on_receive() pass this spoofable size to
mousevsc_on_receive_device_info(), leading to out-of-bounds reads during
memcpy() or kmemdup() because the bounds checking trusts this inner size?
+
+	KUNIT_EXPECT_EQ(test, input_dev->dev_info_status, -EINVAL);
+
+	kfree(input_dev->hid_desc);
+}
-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710022854.3739558-1-michael.bommarito@gmail.com?part=2
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help