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 <[email protected]> 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. > 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 [ ... ] > 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 [ ... ] > @@ -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, [ ... ] > @@ -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/[email protected]?part=2
