Re: [PATCH] HID: universal-pidff: stop the device when force-feedback init fails

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

Pre-existing issues:
- [Critical] Missing validation of input reports in `universal_pidff_probe` causes an out-of-bounds heap write.
--

--- Patch [1]: [PATCH] HID: universal-pidff: stop the device when force-feedback init fails ---
commit 551db56065200f5b5fc8ed4d3eebe2e09f3793b4
Author: Baul Lee <[email protected]>

HID: universal-pidff: stop the device when force-feedback init fails

This commit fixes a use-after-free issue when force-feedback initialization
fails. It ensures the hardware is properly stopped if initialization fails,
preventing the hidraw character device from outliving the device and writing
into freed memory on subsequent opens.

> diff --git a/drivers/hid/hid-universal-pidff.c b/drivers/hid/hid-universal-pidff.c
> --- a/drivers/hid/hid-universal-pidff.c
> +++ b/drivers/hid/hid-universal-pidff.c
> @@ -104,12 +104,14 @@ static int universal_pidff_probe(struct hid_device *hdev,
>  	error = init_function(hdev, id->driver_data);

[Severity: Critical]
This isn't a bug introduced by this patch, but does calling init_function()
here without verifying if the inputs list is empty risk an out-of-bounds
write?

If a crafted USB device exposes a PID usage page and output reports but no
input reports, hdev->inputs remains empty. When hid_pidff_init_with_quirks()
is called without inputs:

drivers/hid/usbhid/hid-pidff.c:hid_pidff_init_with_quirks() {
    struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
    struct input_dev *dev = hidinput->input;
    ...
}

Because the inputs list is empty, list_entry() points to the list head
itself. Due to the struct layouts of hid_device and hid_input, fetching
hidinput->input aliases directly to hid->hidraw.

This pointer to a much smaller struct hidraw object is then passed as a
struct input_dev pointer to pidff_init_fields().

If the descriptor contains the required output reports to pass
pidff_reports_ok(), can operations like this write out of bounds?

drivers/hid/usbhid/hid-pidff.c:pidff_init_fields() {
    if (pidff->type_id[PID_CONSTANT])
        set_bit(FF_CONSTANT, dev->ffbit);
    ...
}

>  	if (error) {
>  		hid_warn(hdev, "Error initialising force feedback\n");
> -		goto err;
> +		goto err_stop;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1