Re: [PATCH can-next 10/13] can: gs_usb: gs_update_state(): set CAN_ERR_CNT if device sends CAN error counters

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

New issues:
- [High] The driver unconditionally reads uninitialized memory from the URB transfer buffer when processing malformed CAN error frames with a small DLC.
- [Medium] Valid CAN error flags (like RX/TX overflow) are silently dropped from userspace if the CAN state does not change.
- [Medium] The CAN_ERR_RESTARTED flag is unconditionally cleared from the CAN ID and then immediately checked, causing explicit restart signals from the firmware to be ignored.

Pre-existing issues:
- [High] If `alloc_can_skb()` or `alloc_canfd_skb()` fails, the driver aborts URB processing without resubmitting the URB, potentially permanently starving the RX path.
--

--- Patch [10]: [PATCH can-next 10/13] can: gs_usb: gs_update_state(): set CAN_ERR_CNT if device sends CAN error counters ---
commit f1e0f1d0c2f80b9c65dc60e4897538baed5336ee
Author: Marc Kleine-Budde <[email protected]>

can: gs_usb: gs_update_state(): set CAN_ERR_CNT if device sends CAN error counters

Some gs_usb compatible firmware send the CAN error counters in CAN error
frames, but doesn't set CAN_ERR_CNT in can_id. Fixup the CAN error frame by
setting CAN_ERR_CNT, if RX or TX error counters are greater than 0.

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index ec9a7cbbbc696..186abc95c9774 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c

[ ... ]

> @@ -495,29 +529,35 @@ static void gs_usb_timestamp_stop(struct gs_usb *parent)
> -static void gs_update_state(struct gs_can *dev, struct can_frame *cf)
> +static void gs_update_state(struct gs_can *dev, struct can_frame *cf,
> +			    enum can_state tx_state, enum can_state rx_state)
>  {
>  	struct can_device_stats *can_stats = &dev->can.can_stats;
>  
> -	if (cf->can_id & CAN_ERR_RESTARTED) {
> -		dev->can.state = CAN_STATE_ERROR_ACTIVE;
> +	/* clear CAN state information, will be set consistently by can_change_state() below */
> +	cf->can_id &= ~(CAN_ERR_CRTL | CAN_ERR_BUSOFF | CAN_ERR_RESTARTED);
> +	cf->data[1] &= ~(CAN_ERR_CRTL_RX_WARNING | CAN_ERR_CRTL_TX_WARNING |
> +			 CAN_ERR_CRTL_RX_PASSIVE | CAN_ERR_CRTL_TX_PASSIVE |
> +			 CAN_ERR_CRTL_ACTIVE);
> +
> +	/* some firmware sends CAN error counters but doesn't set CAN_ERR_CNT, fixup */
> +	if (cf->data[6] || cf->data[7])
> +		cf->can_id |= CAN_ERR_CNT;
> +
> +	const enum can_state new_state = max(rx_state, tx_state);
> +
> +	if (new_state == dev->can.state)
> +		return;

[Severity: Medium]
If the CAN state does not change, we return early from gs_update_state()
here, but we've already stripped the CAN_ERR_CRTL flag from cf->can_id.

Does this silently drop valid CAN error flags, such as RX overflow,
preventing them from reaching userspace?

> +
> +	/* some firmware does automatically CAN bus off recovery, account for this */
> +	if (cf->can_id & CAN_ERR_RESTARTED ||

[Severity: Medium]
Since we unconditionally cleared the CAN_ERR_RESTARTED bit from cf->can_id at
the start of gs_update_state(), won't this check always evaluate to false?

Does this cause explicit restart signals sent by the firmware to be ignored?

> +	    (dev->can.state == CAN_STATE_BUS_OFF && new_state < CAN_STATE_BUS_OFF)) {
>  		can_stats->restarts++;
> -	} else if (cf->can_id & CAN_ERR_BUSOFF) {
> -		dev->can.state = CAN_STATE_BUS_OFF;
> -		can_stats->bus_off++;

[ ... ]

> @@ -606,6 +646,90 @@ gs_usb_get_minimum_rx_length(const struct gs_can *dev, const struct gs_host_fram
> +static void
> +gs_usb_state_get_by_hf(const struct gs_can *dev, const struct gs_host_frame *hf,
> +		       enum can_state *tx_state_p, enum can_state *rx_state_p)
> +{
> +	enum can_state tx_state, rx_state;
> +
> +	/* extract CAN state from frame */
> +	if (hf->can_id & cpu_to_le32(CAN_ERR_CRTL | CAN_ERR_BUSOFF)) {
> +		const u8 err_crtl = hf->classic_can->data[1];
> +
> +		if (hf->can_id & cpu_to_le32(CAN_ERR_BUSOFF)) {
> +			tx_state = CAN_STATE_BUS_OFF;
> +			rx_state = CAN_STATE_BUS_OFF;
> +		} else {
> +			if (err_crtl & CAN_ERR_CRTL_RX_PASSIVE)
> +				rx_state = CAN_STATE_ERROR_PASSIVE;

[ ... ]

> +		}
> +
> +		const u8 tx_err = hf->classic_can->data[6];
> +		const u8 rx_err = hf->classic_can->data[7];

[Severity: High]
Does this code in gs_usb_state_get_by_hf() unconditionally read uninitialized
memory from the kmalloc'ed URB transfer buffer if a malformed CAN error frame
with a small DLC is received?

Since the URB length check only ensures the header is present, accessing fixed
offsets like data[6] and data[7] could read past the actual payload, leaking
uninitialized heap memory to the kernel log and causing erratic state
transitions.

> +
> +		netdev_dbg(dev->netdev, "host_frame: TX-state='%s/%u' RX-state='%s/%u' %s%s%s%s%s%s%s",
> +			   can_get_state_str(tx_state), tx_err,
> +			   can_get_state_str(rx_state), rx_err,

[ ... ]

> @@ -671,7 +791,11 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  	if (hf->echo_id == GS_HOST_FRAME_ECHO_ID_RX) { /* normal rx */
> +		struct sk_buff *skb;
> +
>  		if (hf->flags & GS_CAN_FLAG_FD) {
> +			struct canfd_frame *cfd;
> +
>  			skb = alloc_canfd_skb(netdev, &cfd);
>  			if (!skb)
>  				return;

[Severity: High]
This is a pre-existing issue in gs_usb_receive_bulk_callback(), but if this
allocation fails and we return directly, the URB is never resubmitted.

Could this permanently starve the RX path and kill CAN reception under
systemic memory pressure?

> @@ -685,6 +809,13 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  
>  			memcpy(cfd->data, hf->canfd->data, data_length);
>  		} else {
> +			enum can_state tx_state, rx_state;
> +			struct can_frame *cf;
> +
> +			if (hf->can_id & cpu_to_le32(CAN_ERR_FLAG) &&
> +			    !gs_usb_should_handle_can_error(dev, hf, &tx_state, &rx_state))
> +				goto resubmit_urb;
> +
>  			skb = alloc_can_skb(netdev, &cf);
>  			if (!skb)
>  				return;

[Severity: High]
This is also a pre-existing issue in gs_usb_receive_bulk_callback().
Returning directly here without jumping to the resubmit_urb label means the
URB is permanently dropped from the active queue.

Could this lead to the same RX starvation issue mentioned above?

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