Re: [PATCH] Input: ims-pcu - only expose sysfs attributes on control interface

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The `ims_pcu_ofn_is_attr_visible()` function introduced in the patch returns `0` when `intf != pcu->ctrl_intf` for the `ofn` attribute group. Because `ofn` is a named attribute group, returning `0` suppresses the individual files but still creates an empty 'ofn' directory on the data interface.

Pre-existing issues:
- [High] The `buttons->input` pointer is assigned after `input_register_device()`, and there is no memory barrier before setting `pcu->setup_complete = true`. On weakly ordered architectures, the CPU can reorder these stores, leading to a NULL pointer dereference in the interrupt handler (`ims_pcu_handle_response`) if an event arrives and observes `setup_complete == true` before `buttons->input` is initialized.
- [High] The driver claims the secondary data interface (`pcu->data_intf`) in `ims_pcu_probe` but fails to release it in `ims_pcu_disconnect` when unbinding the control interface. The data interface remains bound with its `intfdata` pointing to the freed `pcu` structure. Subsequent sysfs accesses on the data interface will dereference this freed pointer, resulting in a use-after-free (UAF).
- [High] A malicious USB device can provide an incorrect master interface number in the CDC Union descriptor. `ims_pcu_parse_cdc_data()` uses this to set `pcu->ctrl_intf` and assigns `intfdata` to it. If this differs from the interface currently being probed, the probed interface will have `NULL` driver data. When the sysfs core subsequently calls `ims_pcu_is_attr_visible()`, `usb_get_intfdata()` returns `NULL`, causing a NULL pointer dereference when accessing `pcu->ctrl_intf`.
- [High] The sysfs attribute store function `ims_pcu_reset_device` calls `ims_pcu_execute_command()` without acquiring `pcu->cmd_mutex`. This allows concurrent sysfs operations (e.g., setting backlight brightness or updating firmware) to interleave their command preparations, causing data corruption in the shared `pcu->urb_out_buf` and race conditions on the `pcu->cmd_done` completion structure.
- [High] In `ims_pcu_buffers_free()`, the input URB DMA buffer (`pcu->urb_in_buf`) is incorrectly freed using `pcu->max_out_size` instead of the `pcu->max_in_size` used during allocation.
--

commit e7e143a0135bed02c30cf00e17a9a25adf1ab6c4
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Subject: Input: ims-pcu - only expose sysfs attributes on control interface

When the driver was converted to use the driver core to instantiate device
attributes, they started appearing on all interfaces bound to the driver.
This patch updates the is_visible callbacks to verify that the interface
being accessed is indeed the control interface.

> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 4c022a36dbe84..7c76bc82ab703 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -1249,6 +1249,9 @@ static umode_t ims_pcu_is_attr_visible(struct kobject *kobj,
>  	struct ims_pcu *pcu = usb_get_intfdata(intf);
>  	umode_t mode = attr->mode;
>  
> +	if (intf != pcu->ctrl_intf)
> +		return 0;

[Severity: High]
This isn't a bug introduced by this patch, but could pcu be NULL here?

If a malicious USB device provides an incorrect master interface number in the
CDC Union descriptor, ims_pcu_parse_cdc_data() might set pcu->ctrl_intf to
a different interface than the one being probed. When the sysfs core
subsequently calls ims_pcu_is_attr_visible(), usb_get_intfdata() would
return NULL, leading to a NULL pointer dereference when accessing
pcu->ctrl_intf.

[ ... ]

> @@ -1488,6 +1491,9 @@ static umode_t ims_pcu_ofn_is_attr_visible(struct kobject *kobj,
>  	struct ims_pcu *pcu = usb_get_intfdata(intf);
>  	umode_t mode = attr->mode;
>  
> +	if (intf != pcu->ctrl_intf)
> +		return 0;

[Severity: Low]
Since ofn is a named attribute group, does returning 0 here suppress the
individual files but still leave an empty ofn directory on the data interface?

According to sysfs guidelines, should this return SYSFS_GROUP_INVISIBLE
instead to fully suppress the group directory?


I also noticed a few pre-existing issues in the surrounding code:

drivers/input/misc/ims-pcu.c:ims_pcu_init_application_mode() {
        ...
        if (info->has_gamepad) {
                error = ims_pcu_setup_gamepad(pcu);
                if (error)
                        goto err_destroy_buttons;
        }

        pcu->setup_complete = true;

        return 0;
}

[Severity: High]
This is a pre-existing issue, but should there be a memory barrier before
setting pcu->setup_complete = true?

On weakly ordered architectures, could the CPU reorder these stores? If an
event arrives and ims_pcu_handle_response() observes setup_complete == true
before buttons->input is fully initialized, it might lead to a NULL pointer
dereference in the interrupt handler.

drivers/input/misc/ims-pcu.c:ims_pcu_disconnect() {
        ...
        /*
         * See if we are dealing with control or data interface. The cleanup
         * happens when we unbind primary (control) interface.
         */
        if (alt->desc.bInterfaceClass != USB_CLASS_COMM)
                return;

        ims_pcu_stop_io(pcu);
        ...
        ims_pcu_buffers_free(pcu);
        kfree(pcu);
}

[Severity: High]
This isn't a bug introduced by this patch, but does this fail to release the
claimed data interface?

If the driver claims the secondary data interface (pcu->data_intf) in
ims_pcu_probe(), failing to release it via usb_driver_release_interface()
when unbinding the control interface results in an asymmetric teardown.
The data interface remains bound with its intfdata pointing to the freed
pcu structure, and reading its sysfs attributes could trigger a use-after-free.

drivers/input/misc/ims-pcu.c:ims_pcu_reset_device() {
        ...
        dev_info(pcu->dev, "Attempting to reset device\n");

        error = ims_pcu_execute_command(pcu, PCU_RESET, &reset_byte, 1);
        if (error) {
        ...
}

[Severity: High]
This is a pre-existing issue, but shouldn't ims_pcu_reset_device() acquire
pcu->cmd_mutex before calling ims_pcu_execute_command()?

Without acquiring the lock, concurrent sysfs operations (e.g., setting
backlight brightness or updating firmware) could interleave their command
preparations. This could cause data corruption in the shared
pcu->urb_out_buf and race conditions on the pcu->cmd_done completion
structure.

drivers/input/misc/ims-pcu.c:ims_pcu_buffers_free() {
        ...
        usb_kill_urb(pcu->urb_in);
        usb_free_urb(pcu->urb_in);

        usb_free_coherent(pcu->udev, pcu->max_out_size,
                          pcu->urb_in_buf, pcu->read_dma);
        ...
}

[Severity: High]
This isn't a bug introduced by this patch, but is pcu->urb_in_buf being freed
with the wrong size?

It appears the input URB DMA buffer is incorrectly freed using
pcu->max_out_size instead of the pcu->max_in_size that was used during
allocation. Depending on the DMA allocator, this could lead to memory
corruption or leaks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/ahp23lj4_vXIeUBl@google.com?part=1