Re: [PATCH can-next 11/13] can: gs_usb: implement CAN_CTRLMODE_BERR_REPORTING for devices without native support

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

New issues:
- [High] Out-of-bounds read and use of uninitialized heap memory when processing CAN error frames with missing data payload.
- [High] The CAN error filtering logic silently drops CAN_ERR_RESTARTED frames if they lack a state change flag.
- [Medium] The new CAN error filtering logic drops hardware RX overflow events by bypassing the overflow check.

Pre-existing issues:
- [High] Failing to allocate an SKB drops the USB RX URB permanently, eventually halting reception.
--

--- Patch [11]: [PATCH can-next 11/13] can: gs_usb: implement CAN_CTRLMODE_BERR_REPORTING for devices without native support ---
commit 53e85c22d303a201e9d9f47a5e3240bd4a9af629
Author: Marc Kleine-Budde <[email protected]>

can: gs_usb: implement CAN_CTRLMODE_BERR_REPORTING for devices without native support

This commit implements software filtering for CAN bus errors when hardware
support for disabling bus error reporting is unavailable. It adds a check in
the bulk receive callback to drop bus error frames if reporting is disabled,
while still processing CAN state changes. It also unconditionally advertises
bus error reporting support in ctrlmode_supported.

Link: https://github.com/candle-usb/candleLight_fw/pull/303

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 186abc95c9774..bc69e6b3c512c 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -724,7 +724,8 @@ gs_usb_should_handle_can_error(const struct gs_can *dev, const struct gs_host_fr
>  		return true;
>  
>  	/* handle CAN bus errors */
> -	if (hf->can_id & cpu_to_le32(CAN_ERR_LOSTARB | CAN_ERR_PROT | CAN_ERR_TRX | CAN_ERR_ACK))
> +	if (dev->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING &&
> +	    hf->can_id & cpu_to_le32(CAN_ERR_LOSTARB | CAN_ERR_PROT | CAN_ERR_TRX | CAN_ERR_ACK))
>  		return true;
>  
>  	return false;

[Severity: High]
If a frame only contains CAN_ERR_RESTARTED but no state change flag, this
appears to fall through and return false. Does this silently drop the
automatic bus-off recovery frame, preventing gs_update_state() from handling
it and leaving the interface stuck in the BUS_OFF state?

[Severity: Medium]
When this returns false, gs_usb_receive_bulk_callback() executes a goto to
resubmit_urb. Does this bypass the hardware overflow check
if (hf->flags & GS_CAN_FLAG_OVERFLOW) at the end of the function,
causing the driver to silently drop hardware RX queue overflow notifications?

[Severity: High]
If the USB transfer is shorter than the expected payload (e.g. can_dlc is 0),
minimum_length allows processing just the header. Does accessing
hf->classic_can->data[1] during the state check in gs_usb_state_get_by_hf()
read out-of-bounds or use uninitialized heap memory from the URB buffer since
it bypasses the padding and zeroing that alloc_can_skb() used to provide?

[Severity: High]
This is a pre-existing issue, but returning directly in
gs_usb_receive_bulk_callback() when alloc_can_skb() fails leaks the URB:

gs_usb_receive_bulk_callback()
    skb = alloc_can_skb(netdev, &cf);
    if (!skb)
        return;

Can this lead to permanent loss of RX URBs if memory allocation fails,
eventually halting CAN reception until the interface is reset?

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