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

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

On 2026-09-03 08:47:54 +0000, sashiko-bot@kernel.org wrote:
> 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.

Please check patch 9 in this series. The change is an effect of the LLM 
driven fix merged a head of this series with tried to correct this issue 
in a way to complicated way by using the READ_ONCE and caching the 
value...

> 
> >  	}
> >  
> >  	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

-- 
Kind Regards,
Niklas Söderlund