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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Use-After-Free of `ongoing_tx_skb` due to premature timestamp processing.
- [High] Memory leak of SKBs in `tx_ts_skb_q` during device teardown and lost interrupts.
- [High] Unbounded queueing of TX timestamp SKBs leads to ID wrap-around and corrupt timestamp assignments.
- [High] Invalid pointer dereference of ERR_PTR during PTP clock teardown.
- [Medium] Packet corruption due to early return when disabling hardware timestamps.
- [Medium] Timestamp configuration state corruption on hardware programming failure.

Pre-existing issues:
- [High] A transient SKB allocation failure permanently disables the network interface (DoS).
- [High] Missing bounds check allows malformed MAC-PHY packets to trigger `skb_over_panic`.
--

commit 7f7731256fae07ec53c17aa75b9a25fc0f8b4a24
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>
Subject: net: ethernet: oa_tc6: Support for hardware timestamp

This commit adds hardware timestamping support for the OPEN Alliance
10BASE-T1x MAC-PHY framework. It introduces PTP clock registration,
ioctl handling, and TX/RX packet timestamp extraction functions.

> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6.c b/drivers/net/ethernet/oa_tc6/oa_tc6.c
> index bf96e8d1ccb9a..6cc7c76d1d3c2 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)
> +{
> +	if (!tc6->hw_tstamp_enabled)
> +		return;
> +	if (!skb || (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) == 0)
> +		return;
> +	if (tc6->ts_config.tx_type != HWTSTAMP_TX_ON) {
> +		tc6->tx_hwtstamp_lost++;
> +		return;
> +	}
> +
> +	skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +	u8 ret = tc6->tx_ts_idx++;

[Severity: High]
Does this allow the hardware timestamp ID to wrap around and cause collisions?

The hardware provides only 3 TX timestamp IDs. In oa_tc6_defer_for_hwtstamp,
the code cyclically increments tsc from 1 to 3 and unconditionally adds the
socket buffer to tx_ts_skb_q without checking queue depth. If more than
3 timestamped packets are transmitted before an interrupt fires, the tsc
wraps around. When the interrupt eventually arrives,
oa_tc6_process_deferred_skb will process all buffers matching that tsc,
which could result in the same timestamp being assigned to multiple unrelated
packets.

> +
> +	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);
> +}
> +
> +static int oa_tc6_process_deferred_skb(struct oa_tc6 *tc6, u8 tsc)
> +{
> +	struct skb_shared_hwtstamps tstamp;
> +	struct oa_tc6_ts_info_tx *ski;
> +	struct sk_buff *skb, *tmp;
> +	bool found = false;
> +	int ret = 0;
[ ... ]
> +		if (!ret) {
> +			tstamp.hwtstamp = ktime_set(data[0], data[1]);
> +			skb_tstamp_tx(skb, &tstamp);
> +			tc6->tx_hwtstamp_pkts++;
> +		}
> +
> +		dev_kfree_skb(skb);

[Severity: Critical]
Is there a risk of a use-after-free on ongoing_tx_skb here?

If an skb is assigned to ongoing_tx_skb and placed into tx_ts_skb_q before
it is fully copied to the SPI TX buffer, a stale interrupt for an older
packet sharing the same tsc (due to the ID wrap-around mentioned earlier)
could arrive.

oa_tc6_process_deferred_skb would then unconditionally call dev_kfree_skb on
this buffer. The next iteration of the transmission loop in
oa_tc6_add_tx_skb_to_spi_buf would then dereference the freed ongoing_tx_skb
to continue transmitting its chunks.

> +	}
> +	return ret;
> +}
[ ... ]
> +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;
> +	__be32 ts_val[2];
> +	u32 ts[2];
> +
> +	if (!tc6->hw_tstamp_enabled)
> +		return;

[Severity: Medium]
Could this leave corrupt hardware timestamp tags in the payload?

If the hardware was previously configured to append timestamps and the user
dynamically disables the feature via ioctl, in-flight packets will arrive
with ski->rtsa set. Because of this early return when hw_tstamp_enabled is
false, oa_tc6_update_ts_in_rx_skb skips stripping the 8-byte hardware
timestamp via skb_pull, which destroys the packet framing for the network
stack.

> +	ski = oa_tc6_tsinfo_rx(skb);
> +	if (!ski->rtsa)
> +		return;
> +
> +	memcpy(&ts_val[0], skb->data, 4);
[ ... ]
> -static int oa_tc6_allocate_rx_skb(struct oa_tc6 *tc6)
> +static int oa_tc6_allocate_rx_skb(struct oa_tc6 *tc6, u32 footer)
>  {
> +	struct oa_tc6_ts_info_rx *ski;
> +
>  	tc6->rx_skb = netdev_alloc_skb_ip_align(tc6->netdev, tc6->netdev->mtu +
> -						ETH_HLEN + ETH_FCS_LEN);
> +						ETH_HLEN + ETH_FCS_LEN + OA_TC6_TSTAMP_SZ);
>  	if (!tc6->rx_skb) {
>  		tc6->netdev->stats.rx_dropped++;
>  		return -ENOMEM;

[Severity: High]
This isn't a bug introduced by this patch, but does a transient memory
allocation failure permanently disable the interface?

If netdev_alloc_skb_ip_align fails under memory pressure, it returns -ENOMEM.
This error propagates up to the threaded IRQ handler
oa_tc6_macphy_threaded_irq, which explicitly checks for non-zero returns from
the rx path and calls disable_irq_nosync and oa_tc6_disable_traffic. This
permanently halts the interface instead of safely dropping the frame and
recovering.

>  	}
>  
> +	ski = oa_tc6_tsinfo_rx(tc6->rx_skb);
> +	ski->rtsa = FIELD_GET(OA_TC6_DATA_FOOTER_RTSA_VALID, footer);
> +	ski->rtsp = FIELD_GET(OA_TC6_DATA_FOOTER_RTSP_VALID, footer);
>  	return 0;
>  }
[ ... ]
> @@ -831,13 +999,15 @@ static int oa_tc6_process_spi_data_rx_buf(struct oa_tc6 *tc6, u16 length)
>  	/* Process the chunk with ongoing rx frame data */
> -	oa_tc6_prcs_ongoing_rx_frame(tc6, data);
> +	oa_tc6_prcs_ongoing_rx_frame(tc6, data, footer);

[Severity: High]
This is a pre-existing issue, but is there a missing bounds check here that
could cause a panic?

If the hardware sends a continuous stream of data chunks without ever setting
the end_valid flag, the receive loop continuously appends 64 bytes using
skb_put in oa_tc6_prcs_ongoing_rx_frame. Without validating against the
available socket buffer tailroom, a broken or malicious MAC-PHY could exhaust
the buffer and trigger an skb_over_panic.

>  
>  	return 0;
>  }
[ ... ]
> @@ -1151,6 +1332,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: High]
Are the socket buffers in tx_ts_skb_q properly freed during device teardown?

When traffic is disabled or the device is torn down, it appears tx_ts_skb_q
is ignored by oa_tc6_free_pending_skbs. If an interrupt is lost, the buffers
remain in the queue forever, which could lead to a permanent memory leak of
the socket buffers and their associated references.

>  
>  	/* 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]
Can this cause an invalid pointer dereference on cleanup?

If ptp_clock_register fails, it returns an ERR_PTR which is stored directly
in tc6->ptp_clock. The function returns an error without resetting the
pointer to NULL. Later, oa_tc6_ptp_unregister checks if tc6->ptp_clock is
true, which it is for an ERR_PTR, and attempts to unregister it, leading to a
panic.

> +	dev_info(&tc6->spi->dev, "%s registered. index %d", info->name,
> +		 ptp_clock_index(tc6->ptp_clock));
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(oa_tc6_ptp_register);
[ ... ]
> 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_hwtstamp_set(struct oa_tc6 *tc6,
> +			struct kernel_hwtstamp_config *cfg)
> +{
[ ... ]
> +	hwtstamp_config_from_kernel(&tc6->ts_config, cfg);
> +
> +	/* Supports timestamping all traffic */
> +	if (cfg->rx_filter != HWTSTAMP_FILTER_NONE)
> +		tc6->ts_config.rx_filter = HWTSTAMP_FILTER_ALL;
> +	return oa_tc6_set_hwtstamp_settings(tc6);

[Severity: Medium]
Does this leave the timestamp configuration in an inconsistent state if
hardware programming fails?

The driver copies the new configuration into tc6->ts_config before calling
oa_tc6_set_hwtstamp_settings to program the hardware via SPI. If the SPI
transfer fails, the function returns an error, but tc6->ts_config remains
overwritten with the new values. Future queries to the timestamp
configuration will falsely report the failed configuration.

> +}
> +EXPORT_SYMBOL_GPL(oa_tc6_hwtstamp_set);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-s2500-mac-phy-support-v6-0-18ce79500371@onsemi.com?part=6