Re: [PATCH net 12/20] can: peak_usb: validate uCAN receive record lengths

[sashiko-bot@kernel.org]

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

New issues:
- [High] The new buffer length check prevents graceful loop termination for valid 4-byte padding records, leading to severe log spam.

Pre-existing issues:
- [Critical] Missing NULL check for uninitialized channel devices leads to a NULL pointer dereference.
--

--- Patch [12]: [PATCH net 12/20] can: peak_usb: validate uCAN receive record lengths ---
commit ae45d78fc07951e0485a1ba7921e266e598fe209
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

can: peak_usb: validate uCAN receive record lengths

This patch validates the length of uCAN receive records before copying
their payloads into socket buffers. It requires each record to contain
the fixed header for its type and verifies CAN payload bytes.

Link: https://patch.msgid.link/20260706092836.79754-1-pengpeng@iscas.ac.cn

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> index ef9fd693e9bd3..0d46f4ce5dca7 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> @@ -566,6 +566,13 @@ static int pcan_usb_fd_decode_canmsg(struct pcan_usb_fd_if *usb_if,
>  				     dev->can.ctrlmode);
>  	}
>  
> +	if (!(rx_msg_flags & PUCAN_MSG_RTR) &&
> +	    le16_to_cpu(rx_msg->size) - offsetof(struct pucan_rx_msg, d) <
> +	    cfd->len) {
> +		kfree_skb(skb);
> +		return -EBADMSG;
> +	}
> +

[Severity: Critical]
This is a pre-existing issue, but is there a missing NULL pointer check when
extracting the channel device in this function?

In pcan_usb_fd_decode_canmsg(), the channel index is checked against the
array size:

        if (pucan_msg_get_channel(rm) >= ARRAY_SIZE(usb_if->dev))
                return -ENOMEM;

        dev = usb_if->dev[pucan_msg_get_channel(rm)];
        netdev = dev->netdev;

For single-channel adapters, usb_if->dev[1] is never initialized and remains
NULL. If a malicious or malfunctioning USB device sends a CAN RX message
specifying channel 1, will the driver fetch the NULL pointer and immediately
dereference it when accessing dev->netdev?

This same pattern appears to be present in pcan_usb_fd_decode_status(),
pcan_usb_fd_decode_error(), and pcan_usb_fd_decode_overrun().

[ ... ]

> @@ -728,6 +753,12 @@ static int pcan_usb_fd_decode_buf(struct peak_usb_device *dev, struct urb *urb)
>  	msg_end = urb->transfer_buffer + urb->actual_length;
>  	for (; msg_ptr < msg_end;) {
>  		u16 rx_msg_type, rx_msg_size;
> +		size_t rx_msg_min_size;
> +
> +		if (msg_end - msg_ptr < sizeof(*rx_msg)) {
> +			err = -EBADMSG;
> +			break;
> +		}

[Severity: High]
Will this check fail on valid 4-byte null tags used for padding at the end
of a transfer buffer?

If the hardware or driver uses a 4-byte null tag to mark the end of valid
records (where size = 0), this check for a full 12-byte struct pucan_msg 
will evaluate to true. 

This breaks the loop with err = -EBADMSG, which triggers the fail path and
calls pcan_dump_mem(). Because pcan_dump_mem() uses un-ratelimited logging,
could this result in severe kernel log spam for every receive transfer that
ends in 4-byte padding?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729102802.505168-1-mkl@pengutronix.de?part=12