Re: [PATCH net-next v3 06/10] net: ethernet: ravb: Replace gPTP flags with callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] ravb_dmac_init leaves the hardware in CCC_OPC_OPERATION mode if dmac_start fails.

Pre-existing issues:
- [High] Calling ethtool -T when the interface is down will trigger a NULL pointer dereference or a Use-After-Free in ravb_get_ts_info().
--

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

net: ethernet: ravb: Replace gPTP flags with callbacks

Prepare for adding Gen4 support which will add a third and new way to
interact with the gPTP clock by replacing the flags for Gen2 behavior
(info->gptp) and Gen3 behavior (info->ccc_gac) with callbacks.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 7c7b3ac81e5d6..5d5b9c0e2f450 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -712,8 +712,8 @@ static int ravb_dmac_init(struct net_device *ndev)
>  		return error;
>  
>  	/* Initialise PTP Clock driver */
> -	if (info->gptp)
> -		ravb_ptp_init(ndev);
> +	if (info->ptp && info->ptp->dmac_start)
> +		return info->ptp->dmac_start(ndev);

[Severity: Medium]
If dmac_start() (e.g. PTP initialization) fails during operations like setting
ring parameters or recovering from a TX timeout, ravb_dmac_init() returns the
error immediately.

Does this leave the hardware in CCC_OPC_OPERATION mode without rolling back
the hardware state to CCC_OPC_CONFIG?

Could this leave the device in an inconsistent state where the DMA is running
but the EMAC is disabled and driver queues are detached, causing a local
DoS until the interface is manually restarted?

>  
>  	return 0;
>  }

[ ... ]

> @@ -1774,7 +1774,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->ptp) {

[Severity: High]
This is a pre-existing issue, but calling ethtool -T when the interface is
down can trigger a NULL pointer dereference or a Use-After-Free here in
ravb_get_ts_info().

If a local unprivileged user queries timestamping info via SIOCETHTOOL while
the interface is administratively down, ravb_get_ts_info() eventually passes
priv->ptp.clock to ptp_clock_index(), which unconditionally dereferences it:

ravb_get_ts_info()
    info->phc_index = ptp_clock_index(priv->ptp.clock);

If the device is probed but not yet opened, priv->ptp.clock is NULL,
resulting in a NULL pointer dereference.

Furthermore, when the device is closed, ravb_ptp_stop() unregisters the
clock but does not clear the pointer:

ravb_ptp_stop()
    if (priv->ptp.clock)
        ptp_clock_unregister(priv->ptp.clock);

Calling ethtool -T in this state triggers a Use-After-Free. Could this
pointer be cleared on stop, and checked before use in ravb_get_ts_info()?

>  		info->so_timestamping =
>  			SOF_TIMESTAMPING_TX_SOFTWARE |
>  			SOF_TIMESTAMPING_TX_HARDWARE |

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