Re: [PATCH v5 net-next 8/9] net: dsa: netc: add PTP two-step timestamping support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: dsa: netc: uninitialized tstamp_queue causes NULL deref on port init failure
--

commit 14af7463afd31a995fbafa852be1eeb366978370
Author: Wei Fang <wei.fang@nxp.com>
    
net: dsa: netc: add PTP two-step timestamping support
    
This commit adds two-step TX and RX timestamping support for the NETC switch.
It implements hardware filters to redirect PTP frames to the CPU port and
handles hardware-generated timestamps via the tagger.

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> index 1a7103fe1e343..cb262ae5ba796 100644
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c
[ ... ]
> @@ -277,6 +291,11 @@ static void netc_free_all_ports(struct netc_switch *priv)
>  		if (!np)
>  			continue;
>  
> +		if (np->dp && dsa_port_is_user(np->dp)) {
> +			disable_delayed_work_sync(&np->tstamp_timeout_work);
> +			netc_port_purge_tstamp_queue(np);
> +		}
> +

[Severity: High]
If np->dp is set but netc_port_ptp_init() hasn't run yet, tstamp_timeout_work
and tstamp_queue will be completely zero-initialized since np is allocated
with kzalloc_obj. Is it safe to call disable_delayed_work_sync() and
netc_port_purge_tstamp_queue() on zero-initialized structures?

[ ... ]
> @@ -346,6 +365,15 @@ static int netc_init_all_ports(struct netc_switch *priv)
>  				dev_err(dev, "Failed to create MDIO bus\n");
>  				goto free_all_ports;
>  			}
> +
> +			/* Only the user port needs to support PTP feature, so
> +			 * PTP-related resources, such as tstamp_queue,
> +			 * tstamp_lock, etc., are initialized only for the user
> +			 * port.
> +			 */
> +			err = netc_port_ptp_init(np);
> +			if (err)
> +				return err;
>  		}
>  	}

[Severity: High]
In netc_init_all_ports(), np->dp is assigned early in the loop:

    np->dp = dp;
    ...
    err = netc_port_create_mdio_bus(np, dp->dn);
    if (err) {
        dev_err(dev, "Failed to create MDIO bus\n");
        goto free_all_ports;
    }

If netc_port_create_mdio_bus() or netc_port_get_info_from_dt() fails, it
jumps to free_all_ports before netc_port_ptp_init() is called. Could this
leave the zero-initialized PTP structures exposed to the cleanup path in
netc_free_all_ports() since np->dp was already set?

> diff --git a/drivers/net/dsa/netc/netc_ptp.c b/drivers/net/dsa/netc/netc_ptp.c
> new file mode 100644
> index 0000000000000..756c69a02064e
> --- /dev/null
> +++ b/drivers/net/dsa/netc/netc_ptp.c
[ ... ]
> +void netc_port_purge_tstamp_queue(struct netc_port *np)
> +{
> +	struct sk_buff_head free_list;
> +
> +	__skb_queue_head_init(&free_list);
> +
> +	spin_lock_bh(&np->tstamp_lock);
> +	skb_queue_splice_init(&np->tstamp_queue, &free_list);
> +	spin_unlock_bh(&np->tstamp_lock);
> +
> +	__skb_queue_purge(&free_list);
> +}

[Severity: High]
If np->tstamp_queue is uninitialized (all zeros), skb_queue_splice_init()
will find that list->next is NULL. When it calls __skb_queue_splice(), it
attempts to write to list->next->prev, which would cause a deterministic NULL
pointer dereference and a kernel panic. Additionally, locking the 
uninitialized tstamp_lock would trigger lockdep splats.

Could np->dp be cleared on the error path to avoid this, or should we track
PTP initialization state separately?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012095924.1129312-1-wei.fang@oss.nxp.com?part=8