Re: [PATCH net-next v3 05/10] net: ethernet: ravb: Propagate error from ptp_clock_register()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Calling `ptp_clock_index()` with a NULL pointer causes a deterministic kernel panic when querying ethtool timestamp info.
- [Critical] Failing to clear `priv->ptp.clock` in `ravb_ptp_stop()` leaves a dangling pointer, leading to a Use-After-Free during ethtool queries.
--

commit 8ab377468104948ff315e1ab381238bac1a1783e
Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

net: ethernet: ravb: Propagate error from ptp_clock_register()

If the PTP clock registration failed the error is silently ignored.
Before reworking all callers of ravb_ptp_init() handle and propagate the
error so it can be used.

Link: https://sashiko.dev/#/patchset/20260610102432.3538432-2-niklas.soderlund+renesas@ragnatech.se?part=1

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index e44d065f77cc8..7c7b3ac81e5d6 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1785,7 +1785,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
>  			(1 << HWTSTAMP_FILTER_NONE) |
>  			(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
>  			(1 << HWTSTAMP_FILTER_ALL);
> -		info->phc_index = READ_ONCE(priv->ptp.phc_index);
> +		info->phc_index = ptp_clock_index(priv->ptp.clock);

[Severity: Critical]
Can this code trigger a NULL pointer dereference if priv->ptp.clock is NULL?

If the interface is not brought up yet, or if PTP registration failed during
initialization, priv->ptp.clock will be NULL. When querying ethtool for
timestamping information, ravb_get_ts_info() invokes
ptp_clock_index(priv->ptp.clock).

Since ptp_clock_index() unconditionally dereferences the pointer, it seems
this could result in a crash.

>  	}
>  
>  	return 0;

[ ... ]

> diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
> index f70b616ac847c..19d039b9d24a5 100644
> --- a/drivers/net/ethernet/renesas/ravb_ptp.c
> +++ b/drivers/net/ethernet/renesas/ravb_ptp.c
[ ... ]
> @@ -369,14 +368,10 @@ static void ravb_ptp_sync_irqs(struct net_device *ndev)
>  void ravb_ptp_stop(struct net_device *ndev)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
> -	struct ptp_clock *clock;
> -
> -	WRITE_ONCE(priv->ptp.phc_index, -1);
> -	clock = xchg(&priv->ptp.clock, NULL);
>  
>  	ravb_ptp_disable(ndev);
>  	ravb_ptp_sync_irqs(ndev);
>  
> -	if (clock)
> -		ptp_clock_unregister(clock);
> +	if (priv->ptp.clock)
> +		ptp_clock_unregister(priv->ptp.clock);

[Severity: Critical]
Does this leave priv->ptp.clock as a dangling pointer?

The original code used xchg() to set priv->ptp.clock to NULL before
unregistering it. With this change, when the interface is brought down,
ravb_ptp_stop() unregisters and frees the PTP clock but leaves the pointer
pointing to freed memory.

If an unprivileged user or system daemon queries ethtool while the interface
is down, ravb_get_ts_info() will pass this dangling pointer to
ptp_clock_index(), potentially causing a use-after-free read. Should
priv->ptp.clock be explicitly set to NULL here?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902084703.3228504-1-niklas.soderlund+renesas@ragnatech.se?part=5