Re: [PATCH net-next v4 06/16] net: ethernet: oa_tc6: Support for hardware timestamp

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Data race and Use-After-Free accessing SKB control block after dropping lock.
- [High] Early return in RX timestamp handler fails to pull the hardware timestamp, corrupting the packet.
- [High] Unaligned memory access when reading the hardware timestamp triggers traps/panics on strict architectures.
- [High] Premature queueing of `ongoing_tx_skb` causes Use-After-Free during SPI transmission.
- [High] Error recovery path frees `ongoing_tx_skb` without removing it from `tx_ts_skb_q`, causing list corruption.
- [High] Missing clearance of `ERR_PTR` in `oa_tc6_ptp_register` causes kernel panics upon deregistration.
- [Medium] Un-timestamped SKBs are permanently leaked upon driver module exit.
- [Medium] Logic bug when handling wrapped TX timestamp IDs applies the timestamp to the wrong SKB.
- [Medium] Missing bounds check before reading the timestamp from RX SKB.
- [Low] `oa_tc6_get_ts_info` overwrites and breaks software timestamping capabilities.
--

commit a7d440ad6e924ff31545feb9b5387e71f09e4798
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

net: ethernet: oa_tc6: Support for hardware timestamp

PTP register/unregister calls are implemented in oa_tc6_ptp.c.
The APIs that work with the hardware for timestamp is provided
by vendor code as it may be vendor dependent.

Interface for ndo_hwtstamp_set/get, ioctl, control and status
callback for ethtool are provided to support hardware timestamp
feature.

> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6.c b/drivers/net/ethernet/oa_tc6/oa_tc6.c
> index c7d70d37ba538..9410cecfdc2aa 100644
> --- a/drivers/net/ethernet/oa_tc6/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6.c
[ ... ]
> +static void oa_tc6_defer_for_hwtstamp(struct oa_tc6 *tc6,
> +				      struct sk_buff *skb)
> +{
[ ... ]
> +	skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +	u8 ret = tc6->tx_ts_idx++;
> +
> +	if (ret == OA_TC6_TTSCC_REG_ID)
> +		tc6->tx_ts_idx = OA_TC6_TTSCA_REG_ID;
> +	oa_tc6_tsinfo_tx(skb)->tsc = ret;
> +
> +	list_add_tail(&skb->list, &tc6->tx_ts_skb_q);

[Severity: High]
If the SPI transfer fails later, the error recovery path in
oa_tc6_cleanup_ongoing_tx_skb() frees ongoing_tx_skb via kfree_skb().
Does it leave a dangling pointer in tx_ts_skb_q since it isn't unlinked
here?

A subsequent hardware timestamp event would then iterate over
tx_ts_skb_q in oa_tc6_process_deferred_skb() and dereference the freed
skb, causing linked list corruption and a use-after-free.

> +}
> +
> +static int oa_tc6_process_deferred_skb(struct oa_tc6 *tc6, u8 tsc)
> +{
[ ... ]
> +		if (found) {
> +			dev_warn_ratelimited(&tc6->spi->dev,
> +					     "Multiple skbs. tsc = %d\n",
> +					     tsc);
> +			tc6->tx_hwtstamp_err++;
> +		}
> +		found = true;
> +		list_del(&skb->list);
> +
> +		/* Retrieve the timestamping info */
> +		ret = oa_tc6_read_registers(tc6,
> +					    OA_TC6_REG_TTSCA_HIGH +
> +					    2 * (tsc - 1), &data[0], 2);

[Severity: Medium]
If multiple skbs share the same ID due to missed interrupts wrapping
the counter, does this logic apply the timestamp to the wrong skb?

The list traversal processes the oldest skb first, reading the hardware
register and applying the new timestamp to the stale skb. It then
processes the newer skb, triggers a warning, and reads the hardware
register again. Reading the register twice might clear it or return
zero, leaving the actual newly transmitted skb with an invalid or zero
timestamp.

> +
> +		if (!ret) {
> +			tstamp.hwtstamp = ktime_set(data[0], data[1]);
> +			skb_tstamp_tx(skb, &tstamp);
> +			tc6->tx_hwtstamp_pkts++;
> +		}
[ ... ]
> +static void oa_tc6_update_ts_in_rx_skb(struct oa_tc6 *tc6)
> +{
> +	struct sk_buff *skb = tc6->rx_skb;
> +	struct oa_tc6_ts_info_rx *ski;
> +	u32 ts[2];
> +
> +	if (!tc6->hw_tstamp_enabled)
> +		return;
> +	ski = oa_tc6_tsinfo_rx(skb);
> +	if (!ski->rtsa)
> +		return;

[Severity: High]
If the timestamp valid flag is false (!ski->rtsa), this early return
skips the skb_pull() call. Does this abandon the 8-byte hardware header
at the start of the skb data buffer, causing the network stack to parse
these dummy bytes as the Ethernet header and corrupting the packet?

> +
> +	ts[0] = be32_to_cpu(*((u32 *)(skb->data)));
> +	ts[1] = be32_to_cpu(*((u32 *)(skb->data) + 1));

[Severity: High]
Can this cast trigger an unaligned memory access panic on architectures
with strict alignment requirements?

The receive skb is allocated using netdev_alloc_skb_ip_align(), which
aligns skb->data to a 2-byte offset. Casting skb->data to a u32 pointer
and dereferencing it performs a 32-bit read from a 2-byte aligned
address. Should get_unaligned_be32() be used here instead?

[Severity: Medium]
Is there a risk of an out-of-bounds memory read here if the skb payload
is smaller than 8 bytes?

If a runt packet arrives with less than 8 bytes of data, casting and
reading from skb->data will read past the buffer, and the subsequent
skb_pull() might fail silently without properly decrementing skb->len.

> +
> +	/* Check parity */
> +	if ((oa_tc6_get_parity(ts[0]) ^ oa_tc6_get_parity(ts[1])) ==
[ ... ]
> @@ -866,6 +1041,8 @@ static u16 oa_tc6_prepare_spi_tx_buf_for_tx_skbs(struct oa_tc6 *tc6)
>  			tc6->ongoing_tx_skb = tc6->waiting_tx_skb;
>  			tc6->waiting_tx_skb = NULL;
>  			spin_unlock_bh(&tc6->tx_skb_lock);
> +			oa_tc6_defer_for_hwtstamp(tc6,
> +						  tc6->ongoing_tx_skb);

[Severity: High]
Does queueing the ongoing_tx_skb for timestamping here create a
use-after-free risk during the SPI transmission?

The skb is added to tx_ts_skb_q before its SPI transmission completes.
If the MAC-PHY asserts a TX timestamp interrupt for a previously
transmitted frame with a matching ID, oa_tc6_process_deferred_skb()
might find this currently-transmitting skb in the queue and free it.
The transmission loop in oa_tc6_add_tx_skb_to_spi_buf() would then
dereference the freed skb's length.

>  		}
>  		if (!tc6->ongoing_tx_skb)
>  			break;
[ ... ]
> @@ -1073,6 +1250,7 @@ netdev_tx_t oa_tc6_start_xmit(struct oa_tc6 *tc6, struct sk_buff *skb)
>  	spin_lock_bh(&tc6->tx_skb_lock);
>  	tc6->waiting_tx_skb = skb;
>  	spin_unlock_bh(&tc6->tx_skb_lock);
> +	oa_tc6_tsinfo_tx(skb)->tsc = 0;

[Severity: High]
Does accessing the skb control block here introduce a data race or a
use-after-free?

Once the tx_skb_lock is released, the SPI kthread can immediately dequeue
the skb, assign it a valid hardware timestamp ID, transmit it, and even
free it. Overwriting the control block with tsc = 0 here modifies a
potentially freed skb, or overwrites a valid ID assigned by the SPI
thread, which breaks the TX timestamp correlation.

>  
>  	/* Wake spi kthread to perform spi transfer */
>  	wake_up_interruptible(&tc6->spi_wq);
[ ... ]
> @@ -1103,6 +1281,8 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)
>  	SET_NETDEV_DEV(netdev, &spi->dev);
>  	mutex_init(&tc6->spi_ctrl_lock);
>  	spin_lock_init(&tc6->tx_skb_lock);
> +	tc6->tx_ts_idx = OA_TC6_TTSCA_REG_ID;
> +	INIT_LIST_HEAD(&tc6->tx_ts_skb_q);

[Severity: Medium]
When the driver is unbound or the module is exited, are the
un-timestamped skbs left in tx_ts_skb_q leaked?

If a timestamp interrupt is lost, the skbs remain in this list
indefinitely. If the teardown path does not traverse and free the skbs
in tx_ts_skb_q, this could guarantee a memory leak of all
un-timestamped skbs.

>  
>  	/* Set the SPI controller to pump at realtime priority */
>  	tc6->spi->rt = true;
> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6_ptp.c b/drivers/net/ethernet/oa_tc6/oa_tc6_ptp.c
> new file mode 100644
> index 0000000000000..921191ec6829f
> --- /dev/null
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6_ptp.c
[ ... ]
> +int oa_tc6_ptp_register(struct oa_tc6 *tc6, struct ptp_clock_info *info)
> +{
> +	/* Not supporting hardware timestamp isn't an error */
> +	if (!tc6->hw_tstamp_supported)
> +		return 0;
> +
> +	snprintf(info->name, sizeof(info->name), "%s",
> +		 "OA TC6 PTP clock");
> +	tc6->ptp_clock = ptp_clock_register(info, &tc6->spi->dev);
> +	if (IS_ERR(tc6->ptp_clock)) {
> +		dev_err(&tc6->spi->dev, "Registration of %s failed",
> +			info->name);
> +		return -EFAULT;
> +	}

[Severity: High]
If ptp_clock_register() fails, does this leave an error pointer stored
in tc6->ptp_clock?

If the function returns an error without setting tc6->ptp_clock to
NULL, later teardown routines like oa_tc6_ptp_unregister() will check
if (tc6->ptp_clock) which evaluates to true. Passing the error pointer
to ptp_clock_unregister() would then dereference it and trigger a
kernel panic.

> +	dev_info(&tc6->spi->dev, "%s registered. index %d", info->name,
> +		 ptp_clock_index(tc6->ptp_clock));
> +	return 0;
> +}
[ ... ]
> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6_tstamp.c b/drivers/net/ethernet/oa_tc6/oa_tc6_tstamp.c
> new file mode 100644
> index 0000000000000..272701a4081dc
> --- /dev/null
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6_tstamp.c
[ ... ]
> +int oa_tc6_get_ts_info(struct oa_tc6 *tc6,
> +		       struct kernel_ethtool_ts_info *info)
> +{
> +	if (!tc6->ptp_clock)
> +		return ethtool_op_get_ts_info(tc6->netdev, info);
> +
> +	info->so_timestamping = SOF_TIMESTAMPING_RAW_HARDWARE |
> +				SOF_TIMESTAMPING_TX_HARDWARE |
> +				SOF_TIMESTAMPING_RX_HARDWARE;

[Severity: Low]
Does this assignment completely overwrite and break the software
timestamping capabilities?

By replacing the flags rather than bitwise ORing them with the
software timestamping flags normally provided by
ethtool_op_get_ts_info(), user-space tools querying ethtool might not
see software timestamping capabilities and could fail to fall back to
them.

> +	info->phc_index = ptp_clock_index(tc6->ptp_clock);
> +	info->tx_types = BIT(HWTSTAMP_TX_ON);
> +	info->rx_filters = BIT(HWTSTAMP_FILTER_ALL);
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-s2500-mac-phy-support-v4-0-de0fbc13c6d8@onsemi.com?part=6