Re: [PATCH net v2] net: ravb: fix use-after-free in ravb_get_ts_info

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

Hello,

Some of the issues pointed out in this review are addressed in the RAVB 
PTP rework series [1]. I will respin it as soon as the PTP clock for 
R-Car Gen4 is merged [2].

1. https://lore.kernel.org/all/20260610102432.3538432-1-niklas.soderlund+renesas@ragnatech.se/
2. https://lore.kernel.org/all/20260803095232.3527111-1-niklas.soderlund+renesas@ragnatech.se/

On 2026-08-04 18:06:20 -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: ravb: fix use-after-free in ravb_get_ts_info
> 
> This clears priv->ptp.clock in ravb_ptp_stop() after unregistering the PHC,
> so that ravb_get_ts_info() no longer hands a dangling pointer to
> ptp_clock_index().  It also gates the whole hardware timestamping capability
> block in ravb_get_ts_info() on priv->ptp.clock being set.
> 
> A few questions on the changelog and on the new guards.
> 
> > The PHC is registered by ravb_open() and unregistered by ravb_close().
> > However, ravb_ptp_stop() leaves priv->ptp.clock pointing at the freed
> > clock. Since the netdev remains registered after ndo_stop, get_ts_info
> > can still pass the dangling pointer to ptp_clock_index(), resulting in a
> > use-after-free.
> 
> [Severity: Low]
> Should the changelog also mention the plain NULL dereference case?
> ravb_ptp_init() is only reached from ravb_open(), ravb_wol_restore() /
> ravb_resume(), ravb_set_ringparam() and ravb_tx_timeout_work(), never from
> ravb_probe().
> 
> So between register_netdev() and the first open, priv->ptp.clock is NULL,
> and pre-patch ravb_get_ts_info() passed it directly to ptp_clock_index(),
> which is just:
> 
> drivers/ptp/ptp_clock.c:ptp_clock_index() {
> 	return ptp->index;
> }
> 
> That means ethtool -T on a gptp/ccc_gac ravb interface that has never been
> brought up faulted too.  Since this carries Cc: stable, would it help
> backport triage to state that the most easily reachable manifestation is a
> NULL dereference in the never-opened state, not only a dangling pointer read
> after close?
> 
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index 5f88733094d0f..f22cff05ba35d 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -1768,7 +1768,7 @@ static int ravb_get_ts_info(struct net_device *ndev,
> >  	struct ravb_private *priv = netdev_priv(ndev);
> >  	const struct ravb_hw_info *hw_info = priv->info;
> >  
> > -	if (hw_info->gptp || hw_info->ccc_gac) {
> > +	if ((hw_info->gptp || hw_info->ccc_gac) && priv->ptp.clock) {
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does a NULL-only test cover
> the failure mode that actually puts an invalid pointer in priv->ptp.clock?
> ravb_ptp_init() stores the return value unchecked:
> 
> drivers/net/ethernet/renesas/ravb_ptp.c:ravb_ptp_init() {
> 	...
> 	priv->ptp.clock = ptp_clock_register(&priv->ptp.info, &pdev->dev);
> }
> 
> With CONFIG_PTP_1588_CLOCK=y, ptp_clock_register() returns ERR_PTR(-EINVAL),
> ERR_PTR(-ENOMEM) or ERR_PTR(err) on failure, so priv->ptp.clock can be a
> non-NULL error pointer.  An error pointer passes the new test here and then
> reaches ptp_clock_index(), which is a bare return ptp->index, and it also
> passes the new test in ravb_ptp_stop() and reaches ptp_clock_unregister(),
> which dereferences it right away via ptp_vclock_in_use(ptp) and
> get_device(&ptp->dev).
> 
> Would !IS_ERR_OR_NULL(priv->ptp.clock) be the right guard, plus having
> ravb_ptp_init() normalize a failed registration to NULL?  The sibling driver
> does check: rcar_gen4_ptp_register() tests IS_ERR() and propagates
> PTR_ERR().
> 
> [Severity: Medium]
> Is suppressing the entire block, rather than just phc_index, the intended
> behaviour change?  With no PHC registered, ethtool -T now reports
> tx_types = 0, rx_filters = 0 and loses SOF_TIMESTAMPING_TX_SOFTWARE, since
> __ethtool_get_ts_info() only ORs back:
> 
> net/ethtool/common.c:__ethtool_get_ts_info() {
> 	...
> 	info->so_timestamping |= SOF_TIMESTAMPING_RX_SOFTWARE |
> 				 SOF_TIMESTAMPING_SOFTWARE;
> }
> 
> Meanwhile the peer paths in the same driver still implement all of it:
> ravb_hwtstamp_set() still accepts HWTSTAMP_TX_ON and
> HWTSTAMP_FILTER_PTP_V2_L2_EVENT (and silently promotes unknown filters to
> HWTSTAMP_FILTER_ALL), ravb_start_xmit() still calls skb_tx_timestamp(), and
> the receive path still fills skb_hwtstamps().
> 
> There is a second effect on CONFIG_PTP_1588_CLOCK=n builds.  CONFIG_RAVB
> depends on PTP_1588_CLOCK_OPTIONAL and ravb_ptp.o is linked
> unconditionally, and the stub is:
> 
> include/linux/ptp_clock_kernel.h {
> 	static inline struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
> 							   struct device *parent)
> 	{ return NULL; }
> }
> 
> so priv->ptp.clock is permanently NULL there and this block becomes dead
> code, even though the timestamping registers are still programmed and the
> hwtstamp ioctl still works.  Before the patch those builds reported the
> hardware filters with phc_index == -1.
> 
> Also, ethtool -T is generally read as static device capability, but it now
> changes with interface up/down because the PHC lifetime follows
> ravb_open()/ravb_close().  Would keeping the capability bits and only
> computing info->phc_index when a PHC exists be a narrower fix?
> 
> >  		info->so_timestamping =
> >  			SOF_TIMESTAMPING_TX_SOFTWARE |
> >  			SOF_TIMESTAMPING_TX_HARDWARE |
> 
> [ ... ]
> 
> > diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
> > index 226c6c0ab945b..fc8c601ed3a5e 100644
> > --- a/drivers/net/ethernet/renesas/ravb_ptp.c
> > +++ b/drivers/net/ethernet/renesas/ravb_ptp.c
> > @@ -337,5 +337,8 @@ void ravb_ptp_stop(struct net_device *ndev)
> >  	ravb_write(ndev, 0, GIC);
> >  	ravb_write(ndev, 0, GIS);
> >  
> > -	ptp_clock_unregister(priv->ptp.clock);
> > +	if (priv->ptp.clock) {
> > +		ptp_clock_unregister(priv->ptp.clock);
> > +		priv->ptp.clock = NULL;
> > +	}
> >  }
> 
> [Severity: Medium]
> Can this new NULL store race the interrupt handler?  ravb_ptp_interrupt()
> reads the same field in hardirq context and passes it on without a NULL
> check:
> 
> drivers/net/ethernet/renesas/ravb_ptp.c:ravb_ptp_interrupt() {
> 	u32 gis = ravb_read(ndev, GIS);
> 
> 	gis &= ravb_read(ndev, GIC);
> 	if (gis & GIS_PTCF) {
> 		...
> 		event.timestamp = ravb_read(ndev, GCPT);
> 		ptp_clock_event(priv->ptp.clock, &event);
> 	}
> 	...
> }
> 
> The locksets do not intersect: ravb_ptp_interrupt() runs from
> ravb_interrupt()/ravb_multi_interrupt() under spin_lock(&priv->lock), while
> ravb_ptp_stop() runs in process context and takes no lock.  The irqs are
> installed with devm_request_irq() at probe and stay installed while the
> interface is down, and ravb_close() calls ravb_ptp_stop() before
> ravb_stop_dma() and before napi_disable().
> 
> Writing GIC = 0 first does not appear to close the window, because the
> handler's guard gis &= ravb_read(ndev, GIC) may already have been sampled:
> 
> CPU0 (hardirq)
>     reads GIS (GIS_PTCF set), reads GIC (GIC_PTCE set)
>     ravb_read(ndev, GCPT)          <-- slow MMIO read
> 
> CPU1 (ravb_close)
>     ravb_write(ndev, 0, GIC);
>     ptp_clock_unregister(priv->ptp.clock);
>     priv->ptp.clock = NULL;
> 
> CPU0 (hardirq, continues)
>     ptp_clock_event(NULL, &event); <-- spin_lock_irqsave(&ptp->tsevqs_lock)
> 
> Would masking GIC, then serializing with the handler (clearing the pointer
> under spin_lock_irqsave(&priv->lock), or synchronize_irq() on the
> DMAC/gPTP irq), and only then calling the sleeping ptp_clock_unregister()
> on a local copy be safer?  Should ravb_ptp_interrupt() also tolerate a NULL
> clock now that NULL is a reachable state while the irq is live?
> 
> [Severity: Medium]
> Does clearing the pointer only after ptp_clock_unregister() returns leave
> part of the use-after-free in place?  ravb_get_ts_info() does two separate
> loads of priv->ptp.clock, one for the new test and one for
> ptp_clock_index(), and there is no lock shared with this writer.
> 
> The assumption that RTNL serializes the two sides does not seem to hold.
> __ethtool_get_ts_info() calls ops->get_ts_info() under nothing stronger
> than rcu_read_lock(), and it is reachable from setsockopt(SO_TIMESTAMPING,
> SOF_TIMESTAMPING_BIND_PHC):
> 
> net/core/sock.c:sock_timestamping_bind_phc() {
> 	...
> 	num = ethtool_get_phc_vclocks(dev, &vclock_index);
> 	dev_put(dev);
> 	...
> }
> 
> which holds only a dev_get_by_index() reference, no rtnl_lock().  So:
> 
> CPU0 (setsockopt)                    CPU1 (ip link set down)
> ravb_get_ts_info()
>   priv->ptp.clock != NULL            ravb_ptp_stop()
>                                        ptp_clock_unregister(...)
>                                        priv->ptp.clock = NULL;
>   ptp_clock_index(priv->ptp.clock)
>     return ptp->index;   <-- NULL
> 
> And during the window where the pointer is still non-NULL but
> ptp_clock_unregister() is sleeping in the device_del()/put_device() path, a
> reader can still use a clock that is being torn down; ptp_clock_release()
> ends in a plain kfree(ptp) with no RCU deferral and the reader holds no
> kref.
> 
> Would loading and clearing the pointer once under a lock shared with the
> reader (for example priv->lock), and clearing it before the unregister,
> close both of these?
> -- 
> pw-bot: cr

-- 
Kind Regards,
Niklas Söderlund