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 <[email protected]> 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/[email protected]?part=1
