Re: [PATCH 09/11] Input: ims-pcu - fix DMA mapping violation in line setup

[sashiko-bot@kernel.org]

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

New issues:
- [High] Using GFP_KERNEL in a function called from a USB resume callback risks system deadlocks during suspend/resume.

Pre-existing issues:
- [High] Mismatch between allocation and free sizes for coherent DMA memory.
--

commit ecb1961e66f5ce58ee717e51a89fafeaebd53f86
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: ims-pcu - fix DMA mapping violation in line setup

This commit fixes a DMA mapping alignment violation by replacing the
embedded pcu->cmd_buf with a dynamically allocated buffer for line coding
data.

Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
Cc: stable@vger.kernel.org
Reported-by: Sashiko bot <sashiko-bot@kernel.org>
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index d7a3f4c7214a0..7746a8242f2a0 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -1790,11 +1790,16 @@ static void ims_pcu_stop_io(struct ims_pcu *pcu)
>  static int ims_pcu_line_setup(struct ims_pcu *pcu)
>  {
>  	struct usb_host_interface *interface = pcu->ctrl_intf->cur_altsetting;
> -	struct usb_cdc_line_coding *line = (void *)pcu->cmd_buf;
> +	struct usb_cdc_line_coding *line __free(kfree) =
> +				kmalloc(sizeof(*line), GFP_KERNEL);

[Severity: High]
Does allocating memory with GFP_KERNEL here risk a system deadlock during
suspend/resume?

Since ims_pcu_line_setup() is called by the USB power management callback
ims_pcu_resume():

ims_pcu_resume() {
    if (alt->desc.bInterfaceClass == USB_CLASS_COMM) {
        retval = ims_pcu_start_io(pcu);
        if (retval == 0)
            retval = ims_pcu_line_setup(pcu);
    }
}

During the system resume phase, device locks are held by the PM core. Using
GFP_KERNEL allows direct reclaim, which can initiate filesystem and block I/O.

If these operations attempt to access a suspended device or wait on a shared
lock, it could block indefinitely. Should this use GFP_NOIO instead?

>  	int error;
>  
> -	memset(line, 0, sizeof(*line));
> +	if (!line)
> +		return -ENOMEM;
> +
>  	line->dwDTERate = cpu_to_le32(57600);
> +	line->bCharFormat = USB_CDC_1_STOP_BITS;
> +	line->bParityType = USB_CDC_NO_PARITY;
>  	line->bDataBits = 8;
>  
[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but is there a mismatch between the
allocation and free sizes for the coherent DMA memory?

Looking at ims_pcu_buffers_alloc(), pcu->urb_in_buf is dynamically allocated
with a size of pcu->max_in_size:

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(), the same buffer is freed using
pcu->max_out_size instead:

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

Since max_in_size and max_out_size are initialized independently from their
respective endpoint descriptors, they can differ. Can passing the incorrect
size to usb_free_coherent() result in memory corruption or page leaks?

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