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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 2 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch fixes the error-frame check for one flag combination
  but misses another.
- [Low] For classic error frames, gs_usb_get_minimum_rx_length() now
  returns data_length = CAN_ERR_DLC (8), and…
- [Low] The commit fixes a regression but has no Fixes: tag.

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch.

--

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

This patch changes gs_usb_get_minimum_rx_length() so that classic CAN
error frames received from the device use a data_length of CAN_ERR_DLC (8)
instead of the length derived from the DLC. With that change, both the
minimum URB length check and the copy into cf->data cover the error frame
payload that gs_update_state() reads.

> therefore, for error frames increase the value of data_length in
> gs_usb_get_minimum_rx_length() to CAN_ERR_DLC.
>
> Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>

[Severity: Low]
Should this have a Fixes: tag?

Commit 395d988f9386 ("can: gs_usb: gs_usb_receive_bulk_callback(): check
actual_length before accessing data") changed the classic copy from this:

    memcpy(cf->data, hf->classic_can->data, 8);

to a copy based on data_length. After that change, gs_update_state() reads
a zeroed cf->data[1] for error frames with can_dlc < 2.

395d988f9386 has its own Fixes: tag and is a stable candidate. Would adding
the following help stable trees pick up this correction too?

Fixes: 395d988f9386 ("can: gs_usb: gs_usb_receive_bulk_callback(): check actual_length before accessing data")

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 3d49a73446927..dd01fd22510e2 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))

[Severity: High]
Does a frame with both CAN_ERR_FLAG and CAN_RTR_FLAG set still end up with
data_length == 0 here?

The new CAN_ERR_FLAG test sits inside the !CAN_RTR_FLAG condition. A
classic RX frame from the device with both flags set in can_id therefore
gets data_length = 0. Without GS_CAN_FEATURE_HW_TIMESTAMP, minimum_length
is then just sizeof(hf->header).

gs_usb_receive_bulk_callback() then copies nothing. It still calls
gs_update_state(), because that call only tests CAN_ERR_FLAG:

    memcpy(cf->data, hf->classic_can->data, data_length);

    /* ERROR frames tell us information about the controller */
    if (le32_to_cpu(hf->can_id) & CAN_ERR_FLAG)
        gs_update_state(dev, cf);

If CAN_ERR_CRTL is also set, gs_update_state() reads cf->data[1]. That
byte is the zero fill from alloc_can_skb(), not data from the device, so
the code takes this branch:

    } else {
        dev->can.state = CAN_STATE_ERROR_ACTIVE;
    }

The driver then reports error active instead of the warning or passive
state the device sent. It also skips the error_warning and error_passive
counters.

With GS_CAN_FEATURE_HW_TIMESTAMP enabled, the URB must already contain the
full data field, but those bytes are still not copied.

Before 395d988f9386 the classic path always copied 8 bytes, so this flag
combination is still a regression after this patch. Could CAN_ERR_FLAG be
tested before the RTR exclusion, or could ERR|RTR frames be rejected?

> +				data_length = CAN_ERR_DLC;

[Severity: Low]
For classic error frames the copy length is now CAN_ERR_DLC. However,
gs_usb_receive_bulk_callback() still sets cf->len from the device's
hf->can_dlc:

    can_frame_set_cc_len(cf, hf->can_dlc, dev->can.ctrlmode);

    memcpy(cf->data, hf->classic_can->data, data_length);

can_frame_set_cc_len() only clamps the value to CAN_MAX_DLEN. Suppose a
device sends an error frame with can_dlc < 8. Would userspace then get a
frame with cf->len < CAN_ERR_DLC, while the driver has copied and acted on
bytes past cf->len? Since 395d988f9386 the copy length and cf->len always
matched.

The commit message says "By definition, CAN error frames have a length of
CAN_ERR_DLC (= 8) bytes". Should cf->len also be set to CAN_ERR_DLC for
error frames?

> +			else
> +				data_length = can_cc_dlc2len(hf->can_dlc);
> +		}

[ ... ]

[Severity: Low]
This is a pre-existing issue, not one introduced by this patch.
gs_usb_receive_bulk_callback() passes the raw hf->can_dlc from the device
to can_frame_set_cc_len() without checking its range:

    can_frame_set_cc_len(cf, hf->can_dlc, dev->can.ctrlmode);

The helper in include/linux/can/length.h expects the caller to have
checked the value already:

    /* the caller already ensured that dlc is a value from 0 .. 15 */
    if (ctrlmode & CAN_CTRLMODE_CC_LEN8_DLC && dlc > CAN_MAX_DLEN)
        cf->len8_dlc = dlc;

gs_make_candev() advertises CAN_CTRLMODE_CC_LEN8_DLC. With cc-len8-dlc
enabled, can a device that sends can_dlc > 15 pass a len8_dlc value of up
to 255 to raw socket readers?

In-kernel users read the value through can_get_cc_dlc(), which rejects
values above CAN_MAX_RAW_DLC.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004-gs_usb-cleanups-and-fixes-v1-0-7b4b06244a77%40pengutronix.de