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

[sashiko-bot@kernel.org]

Sashiko AI review found 8 potential issue(s):
- [High] devm_request_threaded_irq() with IRQF_SHARED and a NULL primary handler unconditionally fails.
- [High] Writing a Linux virtual IRQ number to the SIO hardware register corrupts interrupt routing.
- [High] Use-After-Free during USB disconnect due to incorrect teardown ordering of the IRQ domain.
- [High] Direct DMA to stack variables causes kernel panics.
- [High] Missing endpoint count check allows out-of-bounds array access.
- [Medium] Unbalanced release_region() and unsynchronized hardware access due to ignored superio_enter() return value.
- [Medium] Devres teardown ordering creates a window for an unhandled interrupt storm.
- [Medium] Unmapped hardware interrupts invoke the handler for Linux IRQ 0, causing system instability.
--

commit 60375817e31ea4cd313c9492ef71c36123f2e1d1
Author: Ming Yu <a0282524688@gmail.com>

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

This commit adds Host Interface (HIF) via eSPI for the Nuvoton NCT6694,
introducing a new transport driver while reorganizing the core and USB
driver. It provides common read/write functions and shared memory
interrupt handling.

> diff --git a/drivers/mfd/nct6694-hif.c b/drivers/mfd/nct6694-hif.c
> new file mode 100644
> index 0000000000000..7560754e74810
> --- /dev/null
> +++ b/drivers/mfd/nct6694-hif.c
[ ... ]
> +static irqreturn_t nct6694_irq_handler(int irq, void *data)
> +{
[ ... ]
> +	while (intr_status) {
> +		int irq = __ffs(intr_status);
> +
> +		generic_handle_irq_safe(irq_find_mapping(nct6694->domain, irq));
> +		intr_status &= ~BIT(irq);
> +	}
> +
> +	return IRQ_HANDLED;
> +}

If the hardware sets an unexpected interrupt bit that hasn't been mapped,
irq_find_mapping() returns 0. Does passing 0 directly into
generic_handle_irq_safe() inadvertently invoke the handler for Linux IRQ 0,
causing system instability? This applies to both this function and the USB
interrupt handler.

[ ... ]
> +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;
> +
> +	/* Set SIRQ number */
> +	superio_enter(sio_data);

What happens if the request_muxed_region() inside superio_enter() fails and
returns -EBUSY? If the return value is ignored, the driver might proceed
with unsynchronized hardware access and later call superio_exit(),
releasing an I/O region it never successfully acquired.

> +	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);

Could writing the Linux virtual IRQ number here misconfigure the hardware?
The irq parameter comes from platform_get_irq(), which returns a
dynamically allocated virtual IRQ number rather than the physical ISA or
eSPI IRQ line expected by the Super I/O chip.

[ ... ]
> +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);

The IRQ subsystem explicitly rejects shared interrupts without a primary
handler, as the default primary handler unconditionally masks the shared
line and starves other devices. Will this devm_request_threaded_irq() call
unconditionally return -EINVAL and fail to initialize?

Also, devres actions execute in reverse order during removal. Since the
hardware interrupt is enabled via nct6694_irq_init() before the IRQ handler
is requested, the handler will be freed before the hardware interrupt is
disabled during teardown. Could this create a window for an unhandled
interrupt storm?

> diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c
> index 8ce2c4985aaba..903a0a7f0694c 100644
> --- a/drivers/mfd/nct6694.c
> +++ b/drivers/mfd/nct6694.c
[ ... ]
> +static int nct6694_usb_read_msg(struct nct6694 *nct6694,
> +				const struct nct6694_cmd_header *cmd_hd,
> +				void *buf)
> +{
[ ... ]
> +	/* Receive data packet from USB device */
> +	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), buf,
> +			   le16_to_cpu(cmd_hd->len), &rx_len, NCT6694_URB_TIMEOUT);

Child drivers often pass pointers to local stack variables as the buf
argument. When passed directly to usb_bulk_msg(), does this cause a kernel
panic if CONFIG_VMAP_STACK is enabled, due to performing DMA on a stack
variable? Would it be safer to allocate a bounce buffer for the payload?
This applies to nct6694_usb_write_msg() as well.

[ ... ]
>  static int nct6694_usb_probe(struct usb_interface *iface,
>  			     const struct usb_device_id *id)
>  {
[ ... ]
> +	interface = iface->cur_altsetting;
> +
> +	int_endpoint = &interface->endpoint[0].desc;
> +	if (!usb_endpoint_is_int_in(int_endpoint)) {
> +		ret = -ENODEV;
> +		goto err_irq_domain;
> +	}

If a malformed or malicious USB device presents an interface with zero
endpoints, will this direct access to interface->endpoint[0] result in an
out-of-bounds read?

[ ... ]
>  static void nct6694_usb_disconnect(struct usb_interface *iface)
>  {
>  	struct nct6694 *nct6694 = usb_get_intfdata(iface);
> +	struct nct6694_usb_data *udata = nct6694->priv;
>  
> -	usb_kill_urb(nct6694->int_in_urb);
> +	usb_kill_urb(udata->int_in_urb);
>  	irq_domain_remove(nct6694->domain);
> -	usb_free_urb(nct6694->int_in_urb);
> +	usb_free_urb(udata->int_in_urb);
>  }

The child MFD devices are registered using devm_mfd_add_devices(), which
means they will be unbound by devres after this disconnect function returns.
If irq_domain_remove() is called here, will the child drivers attempt to
access the already-freed IRQ domain during their devres cleanup routines,
leading to a use-after-free?

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