Re: [PATCH can] can: gs_usb: fix hardware timestamp state for mixed channels

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Potential deadlock due to using `spin_lock_bh` for `tc_lock` while it is acquired in an URB completion handler.
- [High] A malicious or buggy USB device can cause a kernel panic by sending an RX URB for a channel that hasn't been fully initialized.
- [High] A malicious USB device can cause a kernel panic by sending an RX URB for a timestamp-capable channel before the timecounter is initialized.
--

--- Patch [1]: [PATCH can] can: gs_usb: fix hardware timestamp state for mixed channels ---
commit d94a7b828312e3ba70b37ab83b64617437b2e0cb
Author: Shuangpeng Bai <shuangpeng.kernel@gmail.com>

can: gs_usb: fix hardware timestamp state for mixed channels

The hardware timestamp state is shared by struct gs_usb, but gs_can_open()
and gs_can_close() tie its initialization and teardown to active_channels
and to the feature bits of the channel being opened or closed.

This is wrong for mixed-channel devices in both directions. If a
non-timestamp channel opens first, a later timestamp-capable channel does
not initialize the shared cyclecounter/timecounter because active_channels
is already non-zero. Timestamp RX then calls timecounter_cyc2time() with
parent->tc.cc unset.

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index ec9a7cbbbc696..bf956fef0d196 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
[ ... ]
> @@ -480,19 +481,23 @@ static void gs_usb_timestamp_init(struct gs_usb *parent)
>  	cc->shift = 32 - bits_per(NSEC_PER_SEC / GS_USB_TIMESTAMP_TIMER_HZ);
>  	cc->mult = clocksource_hz2mult(GS_USB_TIMESTAMP_TIMER_HZ, cc->shift);
>  
> -	spin_lock_init(&parent->tc_lock);
>  	spin_lock_bh(&parent->tc_lock);

[Severity: High]
This is a pre-existing issue, but is it possible for a deadlock to occur here?

The lock tc_lock is acquired using spin_lock_bh() in process context 
(gs_usb_timestamp_work). If a USB RX URB completion 
(gs_usb_receive_bulk_callback) fires in hardirq context, it interrupts 
the process context. 

The URB callback then calls gs_usb_skb_set_timestamp(), which also tries 
to acquire tc_lock using spin_lock_bh():

    gs_usb_skb_set_timestamp() {
        ...
        spin_lock_bh(&parent->tc_lock);
        ...
    }

Would this spin forever since the lock is already held by the interrupted 
thread?

>  	timecounter_init(&parent->tc, &parent->cc, ktime_get_real_ns());
>  	spin_unlock_bh(&parent->tc_lock);
>  
[ ... ]
> @@ -980,10 +986,13 @@ static int gs_can_open(struct net_device *netdev)
>  
>  	can_rx_offload_enable(&dev->offload);
>  
> -	if (!parent->active_channels) {
> -		if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
> -			gs_usb_timestamp_init(parent);
> +	if ((dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP) &&
> +	    !parent->timestamp_started) {
> +		gs_usb_timestamp_init(parent);
> +		timestamp_started = true;
> +	}

[Severity: High]
This is a pre-existing issue, but could a malicious USB device cause a 
kernel panic by sending an RX URB before the timecounter is initialized?

When opening a timestamp-capable channel while a non-timestamp channel is 
already open and submitting RX URBs, the network stack sets 
__LINK_STATE_START on the new channel before calling gs_can_open(). This 
makes netif_running() return true immediately.

During the race window before gs_can_open() initializes the timestamp state 
here, an RX frame for the new channel could be processed:

    gs_usb_skb_set_timestamp() {
        ...
        ns = timecounter_cyc2time(&parent->tc, timestamp);
        ...
    }

Since parent->tc.cc is still NULL, would this dereference a NULL pointer?

[ ... ]
> @@ -1577,6 +1585,8 @@ static int gs_usb_probe(struct usb_interface *intf,
>  	parent->channel_cnt = icount;

[Severity: High]
This is a pre-existing issue, but could a malicious USB device trigger a NULL 
pointer dereference by sending an RX URB for a channel that hasn't been 
fully initialized?

The count parent->channel_cnt is set to its maximum here, before the 
underlying objects are fully allocated in the loop that follows.

If userspace brings up the first channel while the loop is still 
initializing the remaining channels, RX URBs are submitted. A device 
could immediately send an RX URB with a channel index that hasn't been 
allocated yet.

The callback gs_usb_receive_bulk_callback() checks the channel:

    if (hf->channel >= parent->channel_cnt)
        ...

This check would pass, but the uninitialized array element is accessed:

    dev = parent->canch[hf->channel];
    netdev = dev->netdev;

Would this cause a panic when dereferencing dev?

>  
>  	init_usb_anchor(&parent->rx_submitted);
> +	spin_lock_init(&parent->tc_lock);
> +	INIT_DELAYED_WORK(&parent->timestamp, gs_usb_timestamp_work);
>  
>  	usb_set_intfdata(intf, parent);
>  	parent->udev = udev;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718052815.2507496-1-shuangpeng.kernel@gmail.com?part=1