Re: [PATCH net-next 6/7] net: dsa: netc: add PTP two-step timestamping support

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Lockless dereference of a concurrently queued SKB clone in the TX fast path leads to Use-After-Free.
- [High] Lockless purge of `skb_txtstamp_queue` during teardown races with the active RX handler.
- [High] Insufficient length verification in `netc_rcv()` leads to an out-of-bounds pull and kernel panic.
- [Medium] Hardware timestamping state is partially applied if RX filter configuration fails.
--

--- Patch [6]: [PATCH net-next 6/7] net: dsa: netc: add PTP two-step timestamping support ---
commit 13ed29f4ee1bf2dc117e6beb81ab58582da3681f
Author: Wei Fang <[email protected]>
Subject: net: dsa: netc: add PTP two-step timestamping support

Add two-step TX timestamping and RX timestamping for the NETC switch.
For RX, install ingress port filter table (IPFT) rules to redirect PTP
frames. For two-step TX, clone the skb, allocate a timestamp ID, and manage
the asynchronous response via switch tags.

[ ... ]
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c
[ ... ]
> +static void netc_free_ports_resources(struct netc_switch *priv)
> +{
> +	struct dsa_port *dp;
> +
> +	dsa_switch_for_each_available_port(dp, priv->ds) {
> +		struct netc_port *np = priv->ports[dp->index];
> +
> +		if (!dsa_port_is_user(dp))
> +			continue;
> +
> +		/* No new SKBs can be enqueued during teardown. Purge without
> +		 * the spinlock to avoid calling kfree_skb() with a destructor
> +		 * (sock_efree) while holding a spinlock.
> +		 */
> +		__skb_queue_purge(&np->skb_txtstamp_queue);

[Severity: High]
During switch teardown (e.g., driver unbind), the DSA teardown sequence
destroys the switch before the master/conduit interface's RX handler is
unregistered. Is it possible for late timestamp responses to still arrive?

If they do arrive, netc_twostep_tstamp_handler() will iterate over
skb_txtstamp_queue under ptp_lock.

Does calling __skb_queue_purge() here without the lock risk corrupting the
list pointers while the RX softirq is concurrently traversing them?

> +	}
> +}
[ ... ]
> --- a/drivers/net/dsa/netc/netc_ptp.c
> +++ b/drivers/net/dsa/netc/netc_ptp.c
[ ... ]
> +int netc_port_hwtstamp_set(struct dsa_switch *ds, int port,
> +			   struct kernel_hwtstamp_config *config,
> +			   struct netlink_ext_ack *extack)
> +{
> +	struct netc_port *np = NETC_PORT(ds, port);
> +	int rx_filter, err;
> +
> +	switch (config->tx_type) {
> +	case HWTSTAMP_TX_ON:
> +	case HWTSTAMP_TX_OFF:
> +		np->ptp_tx_type = config->tx_type;
> +		break;
[ ... ]
> +	err = netc_port_set_ptp_filter(np, rx_filter);
> +	if (err) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to set PTP filter");
> +		return err;
> +	}

[Severity: Medium]
If netc_port_set_ptp_filter() fails and returns an error here, np->ptp_tx_type
remains updated from the earlier switch statement.

Does this leave the software state for hardware timestamping partially applied?
Should np->ptp_tx_type be restored to its previous value on failure?

> +
> +	config->rx_filter = rx_filter;
> +
> +	return 0;
> +}
[ ... ]
> +void netc_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
> +{
[ ... ]
> +	if (np->ptp_tx_type == HWTSTAMP_TX_ON) {
> +		struct sk_buff *clone = skb_clone_sk(skb);
> +
> +		if (unlikely(!clone))
> +			return;
> +
> +		if (netc_port_txtstamp_twostep(np, clone)) {
> +			kfree_skb(clone);
> +			return;
> +		}
> +
> +		NETC_SKB_CB(skb)->clone = clone;
> +		NETC_SKB_CB(skb)->ptp_flag = NETC_PTP_FLAG_TWOSTEP;
> +	}
> +}

[Severity: High]
When PTP two-step TX timestamping is used, netc_port_txtstamp() enqueues a
clone of the SKB via netc_port_txtstamp_twostep() before the original SKB
reaches netc_xmit().

If the hardware delivers a delayed timestamp response for a reused ts_req_id
before netc_xmit() executes, could the RX handler match this newly enqueued
clone, dequeue it, and free it via skb_complete_tx_timestamp()?

If so, when the tagger netc_xmit() eventually runs and calls
netc_fill_tp_tag_subtype2():

net/dsa/tag_netc.c:netc_fill_tp_tag_subtype2() {
    struct sk_buff *clone = NETC_SKB_CB(skb)->clone;
    u8 ts_req_id = NETC_SKB_CB(clone)->ts_req_id;
    ...
}

Could it dereference a clone that has already been freed?

[ ... ]
> --- a/net/dsa/tag_netc.c
> +++ b/net/dsa/tag_netc.c
[ ... ]
> @@ -126,14 +190,17 @@ static int netc_get_rx_tag_len(int type, int subtype)
>  static struct sk_buff *netc_rcv(struct sk_buff *skb,
>  				struct net_device *ndev)
>  {
> +	struct dsa_port *dp = ndev->dsa_ptr;
>  	struct netc_tag_cmn *tag_cmn;
>  	int tag_len, sw_id, port;
>  	int type, subtype;
> +	void *tag;
>  
> -	if (unlikely(!pskb_may_pull(skb, NETC_TAG_MAX_LEN)))
> +	if (unlikely(!pskb_may_pull(skb, NETC_TAG_MAX_LEN - 2)))
>  		goto err_free_skb;

[Severity: High]
This length check was weakened to 12 bytes (NETC_TAG_MAX_LEN - 2) to admit
26-byte two-step response frames.

However, if a malformed 12-byte or 13-byte frame with NETC_TAG_TH_SUBTYPE1 is
received, it passes this 12-byte check. Later in the function, it calls:

tag_len = netc_get_rx_tag_len(type, subtype);
skb_pull_rcsum(skb, tag_len);

Since netc_get_rx_tag_len() returns 14 for NETC_TAG_TH_SUBTYPE1, will this
unconditionally pull 14 bytes?

Can this cause skb->len to underflow and result in an out-of-bounds memory
access during header stripping?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6