Re: [PATCH v7 07/10] mfd: nct6694: Validate the interrupt IN endpoint

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

Pre-existing issues:
- [High] Unchecked transfer lengths lead to stale data reads.
- [Medium] Mismatched endpoint address used for interrupt pipe creation.
- [Medium] Missing endpoint existence and type validation for required bulk endpoints.
--

--- Patch [7]: [PATCH v7 07/10] mfd: nct6694: Validate the interrupt IN endpoint ---
commit 403c0781b839695d774ed394e3d21842e56bd309
Author: Ming Yu <[email protected]>

mfd: nct6694: Validate the interrupt IN endpoint

The probe reads endpoint[0] of the current altsetting without checking
that the interface actually describes any endpoint. A device reporting
zero endpoints makes the driver read past the endpoint array.

> diff --git a/drivers/mfd/nct6694-usb.c b/drivers/mfd/nct6694-usb.c
> index 793ce54c02aad..c8667984df5eb 100644
> --- a/drivers/mfd/nct6694-usb.c
> +++ b/drivers/mfd/nct6694-usb.c
> @@ -305,13 +304,9 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  	if (ret)
>  		goto err_urb;
>  
> -	interface = iface->cur_altsetting;
> -
> -	int_endpoint = &interface->endpoint[0].desc;
> -	if (!usb_endpoint_is_int_in(int_endpoint)) {
> -		ret = -ENODEV;
> +	ret = usb_find_int_in_endpoint(iface->cur_altsetting, &int_endpoint);
> +	if (ret)
>  		goto err_urb;

[Severity: Medium]
This is a pre-existing issue, but does this probe function also need to
validate the bulk endpoints?

It appears that nct6694_usb_read_msg() uses usb_bulk_msg() with a hardcoded
NCT6694_BULK_IN_EP. If a malformed device lacks these bulk endpoints or
configures them with the wrong type, could it trigger a 'BOGUS urb xfer'
warning in the USB core?

> -	}
>  
>  	usb_fill_int_urb(udata->int_in_urb, udev, usb_rcvintpipe(udev, NCT6694_INT_IN_EP),
>  			 udata->int_buffer, sizeof(*udata->int_buffer), nct6694_usb_int_callback,
>  			 nct6694, int_endpoint->bInterval);

[Severity: Medium]
This is a pre-existing issue, but does this mismatch the found endpoint
address?

We dynamically find the interrupt endpoint above and use its bInterval here,
but we still hardcode NCT6694_INT_IN_EP for the pipe creation. If a malformed
device has its first interrupt IN endpoint at a different address, and 0x81 is
used for a different type, could this result in a type mismatch and a kernel
warning?

[Severity: High]
This isn't a bug introduced by this patch, but does the driver handle short or
zero-length packets correctly?

Looking at nct6694_usb_int_callback(), it seems to read the transfer buffer
without checking urb->actual_length:

drivers/mfd/nct6694-usb.c:nct6694_usb_int_callback() {
    ...
	int_status = le32_to_cpu(*status_le);

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

Similarly, in nct6694_usb_read_msg(), it appears rx_len is not checked before
evaluating msg->response_header.sts. If a device returns short or zero-length
packets, could this cause the driver to read uninitialized memory and
potentially trigger spurious IRQs?

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