Re: [PATCH 05/11] Input: ims-pcu - fix race condition in reset_device sysfs callback

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect size passed to usb_free_coherent() when freeing the read buffer.
- [High] Sysfs callbacks on `data_intf` race with buffer allocation/freeing, leading to Use-After-Free or NULL pointer dereference.
- [High] Sequence number (ack_id) race condition allows stale delayed responses to complete a new command prematurely.
--

commit 1a84560748d9dd10fa9a7709e4c015dd9ef492e6
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Subject: Input: ims-pcu - fix race condition in reset_device sysfs callback

This commit fixes a race condition in the ims_pcu_reset_device sysfs callback
by acquiring the command mutex before executing commands. This prevents data
races and corruption of the shared command buffer when commands run
concurrently.

> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index e6603aaf7b303..22be49d3b879d 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -1152,6 +1152,8 @@ static ssize_t ims_pcu_reset_device(struct device *dev,
>  
>  	dev_info(pcu->dev, "Attempting to reset device\n");
>  
> +	guard(mutex)(&pcu->cmd_mutex);
> +

[Severity: High]
This is a pre-existing issue, but can this sysfs callback race with buffer
allocation during probe?

During ims_pcu_probe(), usb_driver_claim_interface() binds data_intf to the
driver before ims_pcu_buffers_alloc() is called:

drivers/input/misc/ims-pcu.c:ims_pcu_probe() {
    ...
	error = usb_driver_claim_interface(&ims_pcu_driver,
					   pcu->data_intf, pcu);
    ...
	error = ims_pcu_buffers_alloc(pcu);
    ...
}

This implicitly registers the sysfs attributes before the command buffers are
allocated. Can a concurrent sysfs access like writing to reset_device execute
ims_pcu_execute_command() and write to pcu->urb_out_buf while it is still
null?

Similarly, during ims_pcu_disconnect(), ims_pcu_buffers_free() is called
before usb_driver_release_interface() removes the sysfs attributes. Could a
concurrent sysfs operation access the freed buffers, causing a use-after-free?

>  	error = ims_pcu_execute_command(pcu, PCU_RESET, &reset_byte, 1);
>  	if (error) {
>  		dev_info(pcu->dev,

There are also a couple of other pre-existing issues in this driver:

[Severity: High]
This isn't a bug introduced by this patch, but is the correct size passed to
usb_free_coherent() when freeing the read buffer?

In ims_pcu_buffers_alloc(), pcu->urb_in_buf is allocated using
pcu->max_in_size:

drivers/input/misc/ims-pcu.c:ims_pcu_buffers_alloc() {
    ...
	pcu->urb_in_buf = usb_alloc_coherent(pcu->udev, pcu->max_in_size,
					     GFP_KERNEL, &pcu->read_dma);
    ...
}

However, in ims_pcu_buffers_free(), it is freed using pcu->max_out_size:

drivers/input/misc/ims-pcu.c:ims_pcu_buffers_free() {
    ...
	usb_free_coherent(pcu->udev, pcu->max_out_size,
			  pcu->urb_in_buf, pcu->read_dma);
    ...
}

Is it possible this mismatch could cause memory leaks or crashes on
architectures that rely on the size parameter for unmapping pages?

[Severity: High]
This is a pre-existing issue, but does the sequence number initialization in
__ims_pcu_execute_command() allow stale responses to complete a new command
prematurely?

In __ims_pcu_execute_command(), pcu->expected_response is set and
init_completion() is called before ims_pcu_send_command():

drivers/input/misc/ims-pcu.c:__ims_pcu_execute_command() {
    ...
	pcu->expected_response = expected_response;
	init_completion(&pcu->cmd_done);

	error = ims_pcu_send_command(pcu, command, data, len);
    ...
}

The sequence number pcu->ack_id is only incremented later inside
ims_pcu_send_command().

If a previous command timed out, could a delayed response arrive from the
hardware during this exact window? Its sequence number would match the still
unincremented pcu->ack_id - 1 checked in ims_pcu_handle_response():

drivers/input/misc/ims-pcu.c:ims_pcu_handle_response() {
    ...
		if (pcu->read_buf[0] == pcu->expected_response &&
		    pcu->read_buf[1] == pcu->ack_id - 1) {
			memcpy(pcu->cmd_buf, pcu->read_buf, pcu->read_pos);
			pcu->cmd_buf_len = pcu->read_pos;
			complete(&pcu->cmd_done);
			return;
		}
    ...
}

Would this cause the waiting thread to wake up immediately and process the
stale data instead of the actual response?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260523050634.501509-1-dmitry.torokhov@gmail.com?part=5