Re: [PATCH net-next v9 05/11] net: ethernet: oa_tc6: Support for hardware timestamp

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] net/oa_tc6: Register address byte offsets incorrectly used as array indices
- [High] net/oa_tc6: Uninitialized stack memory leaked to userspace on PTP unregister
- [High] net/oa_tc6: TOCTOU race in HWTSTAMP settings unmasks interrupts
- [Medium] net/oa_tc6: Torn reads on 64-bit network statistics on 32-bit systems
- [Medium] net/oa_tc6: Software timestamping capabilities missing from ethtool_ts_info
--

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

net: ethernet: oa_tc6: Support for hardware timestamp

This commit adds hardware timestamping capabilities to the OA TC6 framework.
It introduces PTP registration, ethtool callbacks, and handles timestamp
capture registers within the data path.

> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6.c b/drivers/net/ethernet/oa_tc6/oa_tc6.c
> index f8e9309a72f11..0b6e216de8a1b 100644
> --- a/drivers/net/ethernet/oa_tc6/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6.c
[ ... ]
> @@ -50,6 +61,193 @@ static int oa_tc6_get_parity(u32 p)
[ ... ]
> +static void oa_tc6_events_handle(struct oa_tc6 *tc6, u32 val, const u32 *ts)
> +{
[ ... ]
> +		tc6->ts_ttsc_pending &= ~BIT(bit_num);
> +
> +		ski->hwtstamp = ktime_set(ts[2 * bit_num],
> +					  ts[2 * bit_num + 1]);
> +		tc6->tx_hwtstamp_pkts++;
[Severity: Medium]
Is it possible to experience torn reads on this 64-bit statistic on 32-bit
systems? 

Since this is incremented in a threaded IRQ and read locklessly in 
oa_tc6_get_ts_stats(), it appears this counter and the related err/lost
counters might require the u64_stats_sync infrastructure to prevent returning
corrupted values to userspace on 32-bit architectures.

> +		list_move_tail(&skb->list, &deliver);
> +	}
> +	spin_unlock_bh(&tc6->tx_skb_lock);
[ ... ]
> @@ -686,15 +897,28 @@ static void oa_tc6_disable_traffic(struct oa_tc6 *tc6)
[ ... ]
>  static int oa_tc6_process_extended_status(struct oa_tc6 *tc6)
>  {
> +	u32 regs[OA_TC6_STATUS0_TTSC_REG_COUNT];
> +	bool ts_valid = !!tc6->ptp_clock;
>  	u32 value;
>  	int ret;
>  
> -	ret = oa_tc6_read_register(tc6, OA_TC6_REG_STATUS0, &value);
> +	/* When timestamp is disabled, there is no behavior change
> +	 * as it reads only STATUS0 register. When enabled,
> +	 * TTSCA_HIGH..TTSCC_LOW are fetched together with STATUS0
> +	 * to avoid having to make second SPI transaction. Reading few
> +	 * extra registers, even it may not be needed every time this
> +	 * function is called, it is more efficient than making second
> +	 * SPI transaction, when needed.
> +	 */
> +	ret = oa_tc6_read_registers(tc6, OA_TC6_REG_STATUS0, regs,
> +				    ts_valid ?
> +				    OA_TC6_STATUS0_TTSC_REG_COUNT : 1);
>  	if (ret) {
>  		netdev_err(tc6->netdev, "STATUS0 register read failed: %d\n",
>  			   ret);
>  		return ret;
>  	}
> +	value = regs[0];
>  
>  	/* This function is called for each chunk received in a given SPI
>  	 * transaction. In case, extended status bit is set in more than
> @@ -703,6 +927,11 @@ static int oa_tc6_process_extended_status(struct oa_tc6 *tc6)
>  	if (!value)
>  		return 0;
>  
> +	if ((value & OA_TC6_STATUS0_TTSCA_MASK) != 0)
> +		oa_tc6_events_handle(tc6, value &
> +				     OA_TC6_STATUS0_TTSCA_MASK,
> +				     &regs[OA_TC6_TTSC_REG_OFFSET]);

[Severity: High]
Can this sequence leak uninitialized stack memory to userspace?

If oa_tc6_ptp_unregister() is called, tc6->ptp_clock is set to NULL, which
makes ts_valid evaluate to false. This means only 1 register is read into
the regs array. 

However, if a pending hardware timestamp event fires shortly after, the
event bit in OA_TC6_STATUS0_TTSCA_MASK might still be set. This would result
in oa_tc6_events_handle() receiving a pointer to the uninitialized portion of
the regs stack array (&regs[OA_TC6_TTSC_REG_OFFSET]), which is then
interpreted as a ktime_t and passed up to userspace via skb_tstamp_tx().

> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6_std_def.h b/drivers/net/ethernet/oa_tc6/oa_tc6_std_def.h
> index 403a9c22b5f14..92e936a18490b 100644
> --- a/drivers/net/ethernet/oa_tc6/oa_tc6_std_def.h
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6_std_def.h
[ ... ]
> @@ -70,6 +76,33 @@
>  
>  #define OA_TC6_REG_MMS_MASK		GENMASK(19, 16)
>  
> +#define OA_TC6_TSTAMP_SZ			8
> +
> +#define OA_TC6_TTSCA_REG_ID			1
> +#define OA_TC6_TTSCB_REG_ID			2
> +#define OA_TC6_TTSCC_REG_ID			3
> +
> +/* STATUS0 and the TTSCA/B/C capture registers are contiguous in MMS 0, so
> + * they are fetched with a single control transaction while timestamping is
> + * available.
> + */
> +#define OA_TC6_TTSC_REG_OFFSET		(OA_TC6_REG_TTSCA_HIGH - \
> +					 OA_TC6_REG_STATUS0)
> +#define OA_TC6_TTSC_REG_COUNT		(2 * OA_TC6_TTSCC_REG_ID)
> +#define OA_TC6_STATUS0_TTSC_REG_COUNT	(OA_TC6_TTSC_REG_OFFSET + \
> +					 OA_TC6_TTSC_REG_COUNT)
> +
> +/* Useful for reading all the registers between CONFIG0 and INT_MASK0
> + * in a single SPI transaction
> + */
> +#define CFG0_TO_IMASK0_REG_COUNT	(OA_TC6_REG_INT_MASK0 - \
> +					 OA_TC6_REG_CONFIG0 + 1)
> +#define CFG0_VAL_OFFSET			(0)
> +#define STATUS0_VAL_OFFSET		(OA_TC6_REG_STATUS0 - \
> +					 OA_TC6_REG_CONFIG0)
> +#define IRQM_VAL_OFFSET			(OA_TC6_REG_INT_MASK0 - \
> +					 OA_TC6_REG_CONFIG0)

[Severity: Critical]
Are these macros computing byte offsets instead of array word indices?

Since the registers are 32 bits, subtracting the register addresses
(e.g., OA_TC6_REG_INT_MASK0 which is 0x000C minus OA_TC6_REG_CONFIG0
which is 0x0004) yields a byte difference of 8.

When these offsets are used as indices for a u32 array (like regs[8]), they
access wildly out-of-bounds memory instead of the intended sequential index.
Should these offsets be divided by 4 (the register size)?

> 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..539cbd36a1d04
> --- /dev/null
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6_tstamp.c
> @@ -0,0 +1,224 @@
[ ... ]
> +static int oa_tc6_set_hwtstamp_settings(struct oa_tc6 *tc6,
> +					const struct hwtstamp_config *ts_cfg)
> +{
> +	u32 regs[CFG0_TO_IMASK0_REG_COUNT];
> +	u32 cfg0, irqm, status0;
> +	bool disabled;
> +	int ret;
> +
> +	if (tc6->ts_config.flags == ts_cfg->flags &&
> +	    tc6->ts_config.tx_type == ts_cfg->tx_type &&
> +	    tc6->ts_config.rx_filter == ts_cfg->rx_filter)
> +		return 0;
> +
> +	spin_lock_bh(&tc6->tx_skb_lock);
> +	disabled = tc6->disable_traffic;
> +	spin_unlock_bh(&tc6->tx_skb_lock);
> +	if (disabled)
> +		return -EIO;
> +
> +	/* CONFIG0, STATUS0 and INT_MASK0 span one register block, so a
> +	 * single control transaction covers all three.
> +	 */
> +	ret = oa_tc6_read_registers(tc6, OA_TC6_REG_CONFIG0, regs,
> +				    ARRAY_SIZE(regs));
> +	if (ret) {
> +		dev_err(&tc6->spi->dev, "Failed to read CFG0 block\n");
> +		goto out;
> +	}

[Severity: High]
Does this unlocked read-modify-write sequence introduce a TOCTOU race
that could unmask hardware interrupts during an error condition?

The lock is dropped just before oa_tc6_read_registers(), allowing concurrent
execution of oa_tc6_disable_traffic() from the SPI threaded IRQ if a hardware
error occurs. If oa_tc6_disable_traffic() runs here, it sets disable_traffic
and writes ALL_INTERRUPTS to mask interrupts and prevent an interrupt storm.

However, this function will continue, read its locally-modified irqm, and
write it back below, silently undoing the emergency masking.

> +
> +	cfg0 = regs[CFG0_VAL_OFFSET];
> +	status0 = regs[STATUS0_VAL_OFFSET];
> +	irqm = regs[IRQM_VAL_OFFSET];

[Severity: Critical]
Does this code write out-of-bounds stack memory back to the device?

As mentioned in the previous comment regarding the macro definitions,
STATUS0_VAL_OFFSET evaluates to 4 and IRQM_VAL_OFFSET evaluates to 8. 
The regs array is only 9 elements large (0x0C - 0x04 + 1 = 9). Because
these are not proper array indices, this reads from incorrect offsets
and subsequently writes that corrupted state back to the device's
configuration registers.

[ ... ]
> +	ret = oa_tc6_write_register(tc6, OA_TC6_REG_INT_MASK0, irqm);
[ ... ]
> +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: Medium]
Does this assignment inadvertently clear the software timestamping
flags for this interface?

By directly assigning these hardware flags to info->so_timestamping, the
capabilities for software fallback (like SOF_TIMESTAMPING_TX_SOFTWARE and
SOF_TIMESTAMPING_SOFTWARE) are not included. This might cause ethtool and
userspace applications like ptp4l to assume software timestamping is completely
unsupported when the hardware clock is active.

Should it OR the hardware flags with the default software capabilities, or 
perhaps call ethtool_op_get_ts_info() first to prefill them?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-s2500-mac-phy-support-v9-0-dcefe1d0bf0d@onsemi.com?part=5