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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The in-flight one-step Sync SKB outlives the DSA port structures on driver unbind, causing a Use-After-Free in the SKB destructor.
- [High] The unbounded skb_onestep_queue can cause an Out-Of-Memory (OOM) crash if an attacker floods the port with forwarded PTP Sync packets.
- [High] Calling ptp_parse_header() without ensuring the packet headers are in the linear data area causes an out-of-bounds read on fragmented packets.
- [Medium] Modifying the PTP originTimestamp in software without updating the UDP checksum breaks the checksum for non-offloaded packets.
--

--- Patch [7]: [PATCH net-next 7/7] net: dsa: netc: add PTP one-step timestamping support ---
commit df83cf11f57c79cb36a98872a09e3e01dbf7936a
Author: Wei Fang <[email protected]>

net: dsa: netc: add PTP one-step timestamping support

This commit adds one-step TX timestamping support for PTP Sync frames on the
NETC switch. It implements serialization of one-step Sync transmission per
port and intercepts packets to program the PTP originTimestamp before handing
the frame to the hardware.

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c
[ ... ]
> @@ -1011,6 +1014,8 @@ static void netc_free_ports_resources(struct netc_switch *priv)
>  		 * (sock_efree) while holding a spinlock.
>  		 */
>  		__skb_queue_purge(&np->skb_txtstamp_queue);
> +		cancel_work_sync(&np->onestep_work);
> +		__skb_queue_purge(&np->skb_onestep_queue);

[Severity: High]
Does this teardown path need to wait for or cancel in-flight packets that
were already handed off to the conduit device? Local queues are purged, but
an SKB currently in-flight might retain a pointer to the port structure in
its destructor.

>  	}
>  }
> diff --git a/drivers/net/dsa/netc/netc_ptp.c b/drivers/net/dsa/netc/netc_ptp.c
> --- a/drivers/net/dsa/netc/netc_ptp.c
> +++ b/drivers/net/dsa/netc/netc_ptp.c
[ ... ]
> +static void netc_port_prepare_onestep_sync(struct netc_port *np,
> +					   struct sk_buff *skb,
> +					   u32 ptp_class, bool *twostep)
> +{
> +	struct netc_switch *priv = np->switch_priv;
> +	u16 correction_offset, timestamp_offset;
> +	struct ptp_header *ptp_hdr;
> +	u8 msg_type, twostep_flag;
> +	bool is_udp = false;
> +	u32 pkt_type;
> +	u8 *pkt_hdr;
> +
> +	ptp_hdr = ptp_parse_header(skb, ptp_class);
> +	if (!ptp_hdr) {

[Severity: High]
Does calling ptp_parse_header() here risk an out-of-bounds read if the
packet is fragmented? The function directly accesses the IP header, but the
code hasn't verified that the headers are in the linear data area using
pskb_may_pull() yet. The bounds check later in this function happens after
the read has already occurred.

> +		dev_dbg_ratelimited(priv->dev,
> +				    "Port %d failed to parse Sync header\n",
> +				    np->dp->index);
> +		return;
> +	}
[ ... ]
> +static void netc_port_program_onestep(struct netc_port *np,
> +				      struct sk_buff *skb,
> +				      u64 tstamp)
> +{
> +	u16 correction_offset = NETC_SKB_CB(skb)->correction_offset;
> +	u16 timestamp_offset = NETC_SKB_CB(skb)->timestamp_offset;
> +	bool is_udp = NETC_SKB_CB(skb)->is_udp;
> +	u8 *pkt_hdr = skb_mac_header(skb);
> +	u64 sec;
> +	u32 ns;
> +
> +	NETC_SKB_CB(skb)->tstamp = tstamp;
> +	NETC_SKB_CB(skb)->ptp_flag = NETC_PTP_FLAG_ONESTEP;
> +
> +	/* Update originTimestamp field of Sync packet
> +	 * - 48 bits seconds field
> +	 * - 32 bits nanoseconds field
> +	 */
> +	sec = div_u64_rem(tstamp, NSEC_PER_SEC, &ns);
> +	put_unaligned_be16((sec >> 32) & 0xffff, pkt_hdr + timestamp_offset);
> +	put_unaligned_be32(sec & 0xffffffff, pkt_hdr + timestamp_offset + 2);
> +	put_unaligned_be32(ns, pkt_hdr + timestamp_offset + 6);

[Severity: Medium]
Does modifying the originTimestamp payload here break the UDP checksum for
non-offloaded packets? The hardware PM_SINGLE_STEP_CH feature only updates
the checksum for the residence time delta it adds to the correction field.
Do we need to incrementally update the software checksum for this payload
modification?

> +
> +	netc_port_set_onestep_control(np, is_udp, correction_offset);
> +
> +	/* Orphan the skb to release the socket send buffer quota immediately.
> +	 * This is safe because sock_wfree() only updates sk_wmem_alloc and
> +	 * does not touch skb->data. After skb_orphan(), we install our own
> +	 * destructor so that when the conduit driver frees the skb after TX
> +	 * completion, we get notified to send the next queued Sync packet.
> +	 */
> +	skb_orphan(skb);
> +	skb_shinfo(skb)->destructor_arg = np;
> +	skb->destructor = netc_onestep_skb_destructor;

[Severity: High]
Can this in-flight SKB outlive the netc_port structure during driver unbind?
If the driver unbinds while this packet is still queued in the conduit
device, the devres-managed netc_port will be freed. When the conduit device
later transmits and frees the SKB, will netc_onestep_skb_destructor()
dereference a freed pointer?

> +}
[ ... ]
> +struct sk_buff *netc_onestep_sync_handler(struct dsa_switch *ds, int port,
> +					  struct sk_buff *skb)
> +{
> +	struct netc_port *np = NETC_PORT(ds, port);
> +	struct netc_switch *priv = ds->priv;
> +	u64 tstamp;
> +
> +	/* Serialize one-step Sync packets: only one can be in-flight at a
> +	 * time because the SINGLE_STEP register is shared and must match the
> +	 * packet currently being transmitted. Claim the in-flight slot under
> +	 * ptp_lock. If another one-step Sync is already in-flight, queue this
> +	 * skb and return NULL; ownership is transferred to the queue, so no
> +	 * extra reference is needed and netc_xmit() stops processing it.
> +	 */
> +	spin_lock_bh(&np->ptp_lock);
> +	if (test_bit(NETC_FLAG_ONESTEP_IN_PROGRESS, &np->flags)) {
> +		__skb_queue_tail(&np->skb_onestep_queue, skb);
> +		spin_unlock_bh(&np->ptp_lock);
> +
> +		return NULL;

[Severity: High]
Is there a risk of memory exhaustion if an external source floods the port
with forwarded PTP Sync packets? Since this queue has no length limit and
returning NULL consumes the SKB, dsa_user_xmit() will return NETDEV_TX_OK,
bypassing qdisc backpressure mechanisms.

> +	}
> +
> +	tstamp = netc_timer_get_current_time(priv->tmr_dev);

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