Re: [PATCH can-next v5 6/6] can: gs_usb: fix hardware timestamp state for mixed channels

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] can: gs_usb: uninitialized timecounter race in
  gs_usb_timestamp_start()
- [Medium] can: gs_usb: uninitialized variable ts_supported in
  gs_usb_probe()
--

commit 7c42eeba5c6b2155ed555de5122751f1b8542d0f
Author: Shuangpeng Bai <shuangpeng.kernel@gmail.com>

can: gs_usb: fix hardware timestamp state for mixed channels

This commit initializes the hardware timestamping infrastructure in
gs_usb_probe() when at least one channel supports hardware timestamps,
and tracks active timestamp-capable channels in gs_usb_timestamp_start()
and gs_usb_timestamp_stop().

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 6327069f63c95..0a1cd14c940cc 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c

[ ... ]

> @@ -481,18 +482,35 @@ static void gs_usb_timestamp_init(struct gs_usb *parent)
>  	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);
> +	disable_delayed_work(&parent->timestamp);
> +}
> +
> +static void gs_usb_timestamp_start(struct gs_usb *parent)
> +{
> +	scoped_guard(spinlock_bh, &parent->tc_lock) {
> +		parent->active_timestamp_channels++;
> +		if (parent->active_timestamp_channels > 1)
> +			return;
> +
> +		timecounter_init(&parent->tc, &parent->cc, ktime_get_real_ns());
> +	}
> +
> +	enable_delayed_work(&parent->timestamp);
>  	schedule_delayed_work(&parent->timestamp,
>  			      GS_USB_TIMESTAMP_WORK_DELAY_SEC * HZ);
>  }

[Severity: High]
Can moving timecounter_init() into gs_usb_timestamp_start() race with
incoming RX URBs and cause a NULL pointer dereference or read an
incompletely initialized timecounter?

On a mixed-channel device where channel 0 does not support hardware
timestamps and channel 1 does, opening channel 0 submits the shared bulk
IN URBs while parent->tc.cc remains NULL.  When channel 1 is opened
later, __dev_open() sets __LINK_STATE_START (making netif_running() true)
before calling gs_can_open(), and timecounter_init() ->
gs_usb_timestamp_read() also drops parent->tc_lock for a synchronous USB
control transfer before finishing parent->tc initialization:

CPU0                                  CPU1 (<softirq>)
__dev_open(ch1)
  set_bit(__LINK_STATE_START, ...)
  gs_can_open(ch1)
                                      gs_usb_receive_bulk_callback()
                                        netif_running(ch1) == true
                                        gs_usb_rx_offload()
                                          gs_usb_skb_set_timestamp()
                                            timecounter_cyc2time(&parent->tc)
                                              tc->cc->mask  <-- NULL deref
    gs_usb_timestamp_start()
      spin_lock_bh(&parent->tc_lock)
      timecounter_init(&parent->tc, ...)
        tc->cc = cc
        gs_usb_timestamp_read()
          spin_unlock_bh(&parent->tc_lock)
          gs_usb_get_timestamp()
                                      gs_usb_receive_bulk_callback()
                                        ...
                                          timecounter_cyc2time(&parent->tc)
                                            <-- uses uninitialized tc->mask

If an RX URB for channel 1 (such as a frame queued in firmware as
described in commit 5886e4d5ecec ("can: gs_usb: fix time stamp counter
initialization")) completes before timecounter_init() sets tc->cc,
timecounter_cyc2time() dereferences a NULL tc->cc; if it completes while
gs_usb_timestamp_read() has dropped parent->tc_lock, it reads an
incompletely initialized struct timecounter.

[ ... ]

> @@ -1526,6 +1541,7 @@ static int gs_usb_probe(struct usb_interface *intf,
>  	};
>  	struct gs_device_config dconf;
>  	unsigned int icount, i;
> +	bool ts_supported;
>  	int rc;
>  
>  	rc = usb_find_common_endpoints(intf->cur_altsetting,
> @@ -1599,6 +1615,9 @@ static int gs_usb_probe(struct usb_interface *intf,
>  		parent->canch[i] = dev;
>  		parent->canch[i]->parent = parent;
>  
> +		if (parent->canch[i]->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
> +			ts_supported = true;
> +
>  		/* set RX packet size based on FD and if hardware
>  		 * timestamps are supported.
>  		 */
> @@ -1616,6 +1635,9 @@ static int gs_usb_probe(struct usb_interface *intf,
>  		parent->hf_size_rx = max(parent->hf_size_rx, hf_size_rx);
>  	}
>  
> +	if (ts_supported)
> +		gs_usb_timestamp_init(parent);

[Severity: Medium]
Is ts_supported used uninitialized when probing a device where no
channel has GS_CAN_FEATURE_HW_TIMESTAMP set?  ts_supported is declared
without an initializer and is only assigned when the feature bit is
present.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-gs_usb-fixes-v5-0-d52dd4940d46@pengutronix.de?part=6