Re: [PATCH can-next 10/10] can: gs_usb: gs_usb_get_minimum_rx_length(): enforce data_length of 8 bytes for CAN error frames

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] can: gs_usb: valid CAN error frames dropped due to inflated minimum_length
- [Medium] can: gs_usb: missing cf->len update truncates CAN error frames
--

commit ee50d7ce750c8b15bfde5ea17af1803de6e6715b
Author: Marc Kleine-Budde <mkl@pengutronix.de>

can: gs_usb: gs_usb_get_minimum_rx_length(): enforce data_length of 8 bytes for CAN error frames

This commit updates the minimum RX length calculation for CAN error frames to
strictly expect 8 bytes of data. By definition, CAN error frames have a data
length of 8 bytes, so this aligns the data length calculation for error
frames with that specification.

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 3c0464edb5d39..9ec6fed45b814 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -576,8 +576,12 @@ gs_usb_get_minimum_rx_length(const struct gs_can *dev, const struct gs_host_fram
>  			minimum_length = sizeof(hf->header) + data_length;
>  	} else {
>  		if (hf->echo_id == GS_HOST_FRAME_ECHO_ID_RX &&
> -		    !(hf->can_id & cpu_to_le32(CAN_RTR_FLAG)))
> -			data_length = can_cc_dlc2len(hf->can_dlc);
> +		    !(hf->can_id & cpu_to_le32(CAN_RTR_FLAG))) {
> +			if (hf->can_id & cpu_to_le32(CAN_ERR_FLAG))
> +				data_length = CAN_ERR_DLC;
> +			else
> +				data_length = can_cc_dlc2len(hf->can_dlc);
> +		}

[Severity: High]
Does inflating data_length here cause valid, unpadded CAN error frames to
be discarded?

By forcing data_length to 8, the returned minimum_length will increase.
If a USB device sends a short, unpadded error frame where hf->can_dlc is
less than 8, the frame might be shorter than the newly inflated
minimum_length. The check in gs_usb_receive_bulk_callback() would then fail
and drop the frame:

gs_usb_receive_bulk_callback() {
    ...
    minimum_length = gs_usb_get_minimum_rx_length(dev, hf, &data_length);
    if (urb->actual_length < minimum_length) {
        stats->rx_errors++;
        stats->rx_length_errors++;
    ...
}

[Severity: Medium]
Could forcing data_length to 8 here also create a mismatch with cf->len
when the frame is constructed?

In gs_usb_receive_bulk_callback(), the frame length is initialized using the
unmodified hf->can_dlc, but the payload copying relies on data_length:

gs_usb_receive_bulk_callback() {
    ...
    can_frame_set_cc_len(cf, hf->can_dlc, dev->can.ctrlmode);
    ...
    memcpy(cf->data, hf->classic_can->data, data_length);
    ...
}

If the device sends an error frame with hf->can_dlc less than 8, data_length
is now forced to 8, meaning 8 bytes of payload are copied. However, cf->len
will be initialized to a value less than 8. This length mismatch would
truncate the error frame details exposed to the network stack and userspace
tools.

>  
>  		if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
>  			/* timestamp follows data field of max size */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261004-gs_usb-cleanups-and-fixes-v1-0-7b4b06244a77@pengutronix.de?part=10