Re: [PATCH v7 02/10] mfd: nct6694: Refactor USB-specific data into nct6694_usb_data

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

Pre-existing issues:
- [High] GPIO chip is registered with `can_sleep = false` despite its callbacks performing sleeping operations.
- [High] The USB interrupt callback does not check `urb->actual_length`, leading to stale data reads and interrupt storms.
- [High] The USB interrupt callback fails to validate the return value of `irq_find_mapping()`, allowing arbitrary interrupt injection.
--

--- Patch [2]: [PATCH v7 02/10] mfd: nct6694: Refactor USB-specific data into nct6694_usb_data ---
commit e3cdeafa7e419309614b6129c5ba029024c522a3
Author: Ming Yu <[email protected]>

mfd: nct6694: Refactor USB-specific data into nct6694_usb_data

This commit separates USB transport-specific fields from the core nct6694
structure into a new nct6694_usb_data structure. It also adds a void *priv
pointer to the core structure to decouple the shared MFD core from the USB
transport layer.

> diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c
> index 308b2fda3055c..58c1cbcbe3f22 100644
> --- a/drivers/mfd/nct6694.c
> +++ b/drivers/mfd/nct6694.c
[ ... ]
> @@ -326,11 +357,11 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  		goto err_ida;
>  	}
>  
> -	usb_fill_int_urb(nct6694->int_in_urb, udev, usb_rcvintpipe(udev, NCT6694_INT_IN_EP),
> -			 nct6694->int_buffer, sizeof(*nct6694->int_buffer), usb_int_callback,
> +	usb_fill_int_urb(udata->int_in_urb, udev, usb_rcvintpipe(udev, NCT6694_INT_IN_EP),
> +			 udata->int_buffer, sizeof(*udata->int_buffer), usb_int_callback,
>  			 nct6694, int_endpoint->bInterval);

[Severity: High]
This isn't a bug introduced by this patch, but I have a question about the
usb_int_callback() function referenced here.

drivers/mfd/nct6694.c:usb_int_callback() {
    ...
    int_status = le32_to_cpu(*status_le);
    ...
}

The driver parses the URB payload as interrupt status without verifying
urb->actual_length. If a malfunctioning USB device sends an interrupt IN
packet smaller than 4 bytes, the buffer will retain its previous payload
from the USB core.

Could this cause the driver to process the same interrupts continuously and
create an interrupt storm?

[Severity: High]
This is another pre-existing issue, but I wanted to ask about the IRQ
mapping in the same callback.

drivers/mfd/nct6694.c:usb_int_callback() {
    ...
    generic_handle_irq_safe(irq_find_mapping(nct6694->domain, irq));
    ...
}

The return value of irq_find_mapping() is passed directly to the generic
IRQ handler without validation. If the USB device sends a payload with bits
set for unmapped or out-of-bounds IRQs, irq_find_mapping() could return 0.

Could passing a 0 virq to generic_handle_irq_safe() lead to arbitrary
system interrupt injection, such as invoking important system timers on
some architectures?

>  
> -	ret = usb_submit_urb(nct6694->int_in_urb, GFP_KERNEL);
> +	ret = usb_submit_urb(udata->int_in_urb, GFP_KERNEL);
>  	if (ret)
>  		goto err_ida;
[ ... ]

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
there appears to be an issue in the related GPIO driver registration.

drivers/gpio/gpio-nct6694.c:nct6694_gpio_probe() {
    ...
    data->gpio.can_sleep = false;
    ...
}

The driver registers the GPIO chip with can_sleep set to false. However,
the accessors like nct6694_get_value and nct6694_set_value acquire a mutex
and perform USB bulk transfers that sleep.

Could this lead to a scheduling while atomic bug if the GPIO subsystem or
another caller attempts to toggle the GPIOs from an atomic context?

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