Re: [PATCH v7 06/10] mfd: nct6694: Transfer command payloads via a dedicated DMA buffer

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

Pre-existing issues:
- [High] The GPIO controller driver incorrectly sets `data->gpio.can_sleep = false`, falsely advertising to the GPIO core that its callbacks can be invoked from atomic contexts.
- [High] Missing check for unmapped IRQs allows a USB device to arbitrarily trigger IRQ 0 handlers.
--

--- Patch [6]: [PATCH v7 06/10] mfd: nct6694: Transfer command payloads via a dedicated DMA buffer ---
commit 20bf57a851a06631ddae8833be2bd95d21bbd709
Author: Ming Yu <[email protected]>

mfd: nct6694: Transfer command payloads via a dedicated DMA buffer

This commit fixes a DMA safety issue on non-coherent architectures by
transferring command payloads through a dedicated buffer owned by the
transport. It also ensures commands exceeding the maximum firmware packet
size are rejected.

> diff --git a/drivers/mfd/nct6694-usb.c b/drivers/mfd/nct6694-usb.c
> index 2289ebfde7fab..793ce54c02aad 100644
> --- a/drivers/mfd/nct6694-usb.c
> +++ b/drivers/mfd/nct6694-usb.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but does this code leave us vulnerable to
spurious IRQ 0 triggers?

Looking at nct6694_usb_int_callback() in drivers/mfd/nct6694-usb.c, the
return value is passed directly into the handler:

    generic_handle_irq_safe(irq_find_mapping(nct6694->domain, irq));

If a buggy or malicious USB device sends an interrupt URB with bits set
for unmapped IRQs, irq_find_mapping() returns 0. Since 0 is often a valid
system interrupt on some architectures, could this crash the system by
unconditionally passing 0 to generic_handle_irq_safe()?

[Severity: High]
This isn't a bug introduced by this patch, but does the GPIO controller
driver falsely advertise that its callbacks can run in atomic context?

In drivers/gpio/gpio-nct6694.c during nct6694_gpio_probe(), the driver sets:

    data->gpio.can_sleep = false;

However, the GPIO accessors like nct6694_get_value() take a mutex and
perform blocking USB I/O:

    guard(mutex)(&data->lock);
    ret = nct6694_read_msg(data->nct6694, &cmd_hd, &data->reg_val);

Will this cause a kernel panic if a consumer of this GPIO chip calls a
gpiod_* function from within a spinlock or hardirq context, given that the
GPIO core permits atomic usage based on this flag?

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