Re: [PATCH v7 08/10] mfd: nct6694: Submit the interrupt URB after the core is registered

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

Pre-existing issues:
- [High] Missing validation of irq_find_mapping() return value leads to spurious system interrupts (e.g., IRQ 0).
- [High] Missing urb->actual_length validation leads to processing stale memory and spurious interrupt storms.
--

--- Patch [8]: [PATCH v7 08/10] mfd: nct6694: Submit the interrupt URB after the core is registered ---
commit 8c344a9772877b334fe47602ee4be5e3e806ecab
Author: Ming Yu <[email protected]>

mfd: nct6694: Submit the interrupt URB after the core is registered

The interrupt URB is submitted before nct6694_core_probe() creates the
IRQ domain. A device completing the URB early makes the callback pass a
NULL domain to irq_find_mapping(), which then falls back to the default
domain and dispatches interrupts the device does not own.

Submit the URB once the core is registered, and kill it before the core
is removed so the callback never runs without a domain.

> diff --git a/drivers/mfd/nct6694-usb.c b/drivers/mfd/nct6694-usb.c
> index c8667984df5eb..cc3e7b7b3a0dc 100644
> --- a/drivers/mfd/nct6694-usb.c
> +++ b/drivers/mfd/nct6694-usb.c
> @@ -312,20 +312,20 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  			 udata->int_buffer, sizeof(*udata->int_buffer), nct6694_usb_int_callback,

[Severity: High]
This is a pre-existing issue, but does this callback risk dispatching
interrupts the device does not own if an unmapped IRQ is received?

Looking at the loop inside nct6694_usb_int_callback():

	while (int_status) {
		int irq = __ffs(int_status);

		generic_handle_irq_safe(irq_find_mapping(nct6694->domain, irq));
		int_status &= ~BIT(irq);
	}

If a malfunctioning or malicious USB device sends an interrupt status for an
IRQ bit that has not been mapped, irq_find_mapping() will return 0.

By passing 0 directly to generic_handle_irq_safe(), wouldn't this bypass
validation and incorrectly invoke the system's handler for IRQ 0 (typically
the x86 PIT)?

Would using generic_handle_domain_irq_safe() be a safer approach?

[Severity: High]
This isn't a bug introduced by this patch, but could this callback trigger
an infinite loop of spurious interrupts if the USB device returns a short
packet?

In nct6694_usb_int_callback(), we dereference the transfer buffer:

	int_status = le32_to_cpu(*status_le);

If the USB device sends a 0-byte packet with a successful status,
urb->actual_length would be 0, and the buffer retains its previous state.

Since the bits are only cleared in the local int_status variable and
the URB is resubmitted, wouldn't the exact same interrupts be triggered
again?

Should we verify urb->actual_length is at least 4 before processing?

>  			 nct6694, int_endpoint->bInterval);
>  
> -	ret = usb_submit_urb(udata->int_in_urb, GFP_KERNEL);
> -	if (ret)
> -		goto err_urb;
> -
>  	usb_set_intfdata(iface, nct6694);
>  
>  	ret = nct6694_core_probe(dev, nct6694, nct6694_usb_devs, ARRAY_SIZE(nct6694_usb_devs));
>  	if (ret)
> -		goto err_mfd;
> +		goto err_urb;
> +
> +	ret = usb_submit_urb(udata->int_in_urb, GFP_KERNEL);
> +	if (ret)
> +		goto err_core;
>  
>  	return 0;

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