Re: [PATCH v4 1/1] mfd: Add Host Interface (HIF) support for Nuvoton NCT6694

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] Calling devm_request_threaded_irq() with a NULL primary handler and IRQF_SHARED unconditionally fails.
- [High] The Linux virtual IRQ number is incorrectly written to the hardware Super-I/O IRQ routing register.
- [High] USB transfer buffers are allocated with devm_kzalloc(), violating DMA cacheline alignment requirements.
- [High] Hardware interrupts are enabled before the IRQ handler is registered, and disabled after the handler is freed, creating a window for unhandled interrupt storms.
- [High] Missing bounds check on interface->endpoint array before accessing the first endpoint in the USB probe function.
- [High] The USB interrupt callback ignores urb->actual_length, risking reading uninitialized or stale memory.
- [High] Returning IRQ_NONE after successfully acknowledging the device's interrupt in the HIF IRQ handler.
- [High] Software-only IRQ masking causes an interrupt storm if a child device's IRQ is disabled by the OS.
--

commit 719984a8b6993b3c3d29315e4713194e4c4a7af4
Author: Ming Yu <a0282524688@gmail.com>

mfd: Add Host Interface (HIF) support for Nuvoton NCT6694

The Nuvoton NCT6694 provides a Host Interface (HIF) via eSPI. This commit
updates the driver configuration to allow selecting between the USB and HIF
interfaces, sharing common functions and sub-devices.

> diff --git a/drivers/mfd/nct6694-hif.c b/drivers/mfd/nct6694-hif.c
> new file mode 100644
> index 0000000000000..ca8c08b0d2e34
> --- /dev/null
> +++ b/drivers/mfd/nct6694-hif.c
[ ... ]
> +static irqreturn_t nct6694_irq_handler(int irq, void *data)
> +{
> +	struct nct6694 *nct6694 = data;
> +	struct nct6694_hif_data *hdata = nct6694->priv;
> +	u8 reg_data[4];
> +	u32 intr_status;
> +	int ret;
> +
> +	/* Check interrupt status is set */
> +	if (!(inb(hdata->shm_base + NCT6694_SHM_COFS_STS) & NCT6694_SHM_COFS_STS_COFS4W))
> +		return IRQ_NONE;
> +
> +	/* Clear interrupt status */
> +	outb(NCT6694_SHM_COFS_STS_COFS4W, hdata->shm_base + NCT6694_SHM_COFS_STS);
> +
> +	ret = regmap_bulk_read(hdata->regmap, NCT6694_SHM_INTR_STATUS,
> +			       reg_data, ARRAY_SIZE(reg_data));
> +	if (ret)
> +		return IRQ_NONE;

Is it correct to return IRQ_NONE here?

The driver has already verified the interrupt belongs to the device and
cleared it at the hardware level.

Should this return IRQ_HANDLED instead to prevent misleading the kernel's
spurious interrupt detector?

[ ... ]
> +static int nct6694_irq_init(struct nct6694 *nct6694, int irq)
> +{
> +	struct nct6694_hif_data *hdata = nct6694->priv;
> +	struct nct6694_sio_data *sio_data = hdata->sio_data;
> +	unsigned char cofs_ctl2;
> +	int ret;
> +
> +	/* Set SIRQ number */
> +	ret = superio_enter(sio_data);
> +	if (ret)
> +		return ret;
> +
> +	superio_select(sio_data, SIO_REG_LD_SHM);
> +
> +	if (!superio_inb(sio_data, SIO_REG_SHM_ENABLE)) {
> +		superio_exit(sio_data);
> +		return -EIO;
> +	}
> +
> +	hdata->shm_base = superio_inw(sio_data, SIO_REG_SHM_BASE_ADDR);
> +
> +	superio_outb(sio_data, SIO_REG_SHM_IRQ_NR, irq);

Does irq hold a Linux virtual IRQ number here?

platform_get_irq() returns a Linux-internal virtual IRQ number (virq), while
the Super-I/O hardware configuration register typically expects a physical ISA
IRQ routing number (e.g. 0-15).

Could writing a virtual IRQ number misconfigure the hardware?

[ ... ]
> +static void nct6694_irq_disable(struct irq_data *data)
> +{
> +	struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data);
> +	irq_hw_number_t hwirq = irqd_to_hwirq(data);
> +
> +	guard(spinlock_irqsave)(&nct6694->irq_lock);
> +
> +	nct6694->irq_enable &= ~BIT(hwirq);
> +}

Could this software-only IRQ masking cause an interrupt storm?

If a child device's IRQ is disabled by the OS, nct6694->irq_enable is updated
but the interrupt is not masked at the hardware level.

Since the top-level handler nct6694_irq_handler() dispatches interrupts via
generic_handle_irq_safe(), which ignores irq_enable, could the device
immediately reassert the interrupt if the child handler never runs to clear
the hardware condition?

[ ... ]
> +static int nct6694_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = nct6694_irq_init(data, irq);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, nct6694_irq_release, data);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_request_threaded_irq(dev, irq, NULL, nct6694_irq_handler,
> +					IRQF_ONESHOT | IRQF_SHARED,
> +					dev_name(dev), data);

Does this sequence create a window for unhandled interrupt storms?

nct6694_irq_init() enables interrupts on the device hardware, and the OS
handler is requested afterwards. Similarly, because devres unwinds in reverse
order, the OS handler will be freed before the hardware interrupt is disabled
during removal.

Also, can devm_request_threaded_irq() succeed here?

The Linux interrupt core explicitly rejects requests for shared interrupts
(IRQF_SHARED) with a NULL primary hardirq handler, returning -EINVAL.

> diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c
> index 308b2fda3055c..cf3457c2b3ed2 100644
> --- a/drivers/mfd/nct6694.c
> +++ b/drivers/mfd/nct6694.c
[ ... ]
> @@ -210,10 +236,12 @@ static void usb_int_callback(struct urb *urb)
>  	int_status = le32_to_cpu(*status_le);
>  
>  	while (int_status) {
> -		int irq = __ffs(int_status);
> +		int hwirq = __ffs(int_status);
> +		unsigned int virq = irq_find_mapping(nct6694->domain, hwirq);

Is it safe to read *status_le without checking urb->actual_length?

If a buggy or malicious USB device sends a short packet or a Zero-Length
Packet (ZLP), could the driver read uninitialized or stale data from the
transfer buffer?

[ ... ]
> @@ -284,18 +313,28 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  	if (!nct6694)
>  		return -ENOMEM;
>  
> -	nct6694->usb_msg = devm_kzalloc(dev, sizeof(union nct6694_usb_msg), GFP_KERNEL);
> -	if (!nct6694->usb_msg)
> +	udata = devm_kzalloc(dev, sizeof(*udata), GFP_KERNEL);
> +	if (!udata)
>  		return -ENOMEM;
>  
> -	nct6694->int_buffer = devm_kzalloc(dev, sizeof(*nct6694->int_buffer), GFP_KERNEL);
> -	if (!nct6694->int_buffer)
> +	udata->usb_msg = devm_kzalloc(dev, sizeof(*udata->usb_msg), GFP_KERNEL);
> +	if (!udata->usb_msg)
>  		return -ENOMEM;
>  
> -	nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
> -	if (!nct6694->int_in_urb)
> +	udata->int_buffer = devm_kzalloc(dev, sizeof(*udata->int_buffer), GFP_KERNEL);
> +	if (!udata->int_buffer)
>  		return -ENOMEM;

Is it safe to allocate USB DMA transfer buffers using devm_kzalloc()?

Memory allocated via devm_kzalloc() is prefixed with a devres header, causing
the actual payload to be unaligned to ARCH_DMA_MINALIGN.

Could this lead to cache invalidation wiping out concurrent CPU updates to the
adjacent devres header on architectures with non-coherent DMA? Should standard
kmalloc() be used instead?

[ ... ]
> @@ -326,11 +364,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);

Does the code verify that the interface has any endpoints configured before
accessing &interface->endpoint[0].desc to set int_endpoint earlier in the
probe function?

If a malformed USB device exposes an interface with zero endpoints, could
accessing endpoint[0] result in an out-of-bounds read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515085746.114361-2-a0282524688@gmail.com?part=1