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

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