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

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

[Shuangpeng Bai]

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.

Conversely, if a timestamp-capable channel opens first and starts the
shared delayed work, then closes while a non-timestamp channel remains
active, disconnect may close the non-timestamp channel last. The old
teardown check skips gs_usb_timestamp_stop() in that case and frees
struct gs_usb while the delayed work timer is still queued.

Track whether the shared timestamp work has been started in struct
gs_usb. Start it whenever opening a timestamp-capable channel and the
shared state is not started yet, and stop it when the last channel closes
regardless of that channel's feature bits. Initialize the delayed work and
lock once during probe.

Fixes: 45dfa45f52e6 ("can: gs_usb: add RX and TX hardware timestamp support")
Cc: stable@vger.kernel.org
Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
---
 drivers/net/can/usb/gs_usb.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index ec9a7cbbbc69..bf956fef0d19 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -337,6 +337,7 @@ struct gs_usb {
 
 	unsigned int hf_size_rx;
 	u8 active_channels;
+	bool timestamp_started;
 	u8 channel_cnt;
 
 	unsigned int pipe_in;
@@ -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);
 	timecounter_init(&parent->tc, &parent->cc, ktime_get_real_ns());
 	spin_unlock_bh(&parent->tc_lock);
 
-	INIT_DELAYED_WORK(&parent->timestamp, gs_usb_timestamp_work);
 	schedule_delayed_work(&parent->timestamp,
 			      GS_USB_TIMESTAMP_WORK_DELAY_SEC * HZ);
+
+	parent->timestamp_started = true;
 }
 
 static void gs_usb_timestamp_stop(struct gs_usb *parent)
 {
+	if (!parent->timestamp_started)
+		return;
+
 	cancel_delayed_work_sync(&parent->timestamp);
+	parent->timestamp_started = false;
 }
 
 static void gs_update_state(struct gs_can *dev, struct can_frame *cf)
@@ -959,6 +964,7 @@ static int gs_can_open(struct net_device *netdev)
 	struct urb *urb = NULL;
 	u32 ctrlmode;
 	u32 flags = 0;
+	bool timestamp_started = false;
 	int rc, i;
 
 	rc = open_candev(netdev);
@@ -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;
+	}
 
+	if (!parent->active_channels) {
 		for (i = 0; i < GS_MAX_RX_URBS; i++) {
 			u8 *buf;
 
@@ -1094,11 +1103,11 @@ static int gs_can_open(struct net_device *netdev)
 out_usb_free_urb:
 	usb_free_urb(urb);
 out_usb_kill_anchored_urbs:
+	if (timestamp_started)
+		gs_usb_timestamp_stop(parent);
+
 	if (!parent->active_channels) {
 		usb_kill_anchored_urbs(&parent->rx_submitted);
-
-		if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
-			gs_usb_timestamp_stop(parent);
 	}
 
 	can_rx_offload_disable(&dev->offload);
@@ -1155,8 +1164,7 @@ static int gs_can_close(struct net_device *netdev)
 	if (!parent->active_channels) {
 		usb_kill_anchored_urbs(&parent->rx_submitted);
 
-		if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
-			gs_usb_timestamp_stop(parent);
+		gs_usb_timestamp_stop(parent);
 	}
 
 	/* Stop sending URBs */
@@ -1577,6 +1585,8 @@ static int gs_usb_probe(struct usb_interface *intf,
 	parent->channel_cnt = icount;
 
 	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;
-- 
2.43.0

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

[sashiko-bot]

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