Re: [PATCH net-next v4 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:
- [Critical] The PTP clock driver is leaked on the ravb_open() error path for Gen3 hardware, leading to a resource leak and Use-After-Free on unbind.

Pre-existing issues:
- [High] DMA descriptor rings and RX buffers are leaked if ravb_dmac_init() or ravb_open() fails.
--

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

net: ethernet: ravb: Replace gPTP flags with callbacks

This commit prepares for Gen4 support by replacing the Gen2 (info->gptp)
and Gen3 (info->ccc_gac) flags with callbacks in a new ravb_gptp_info
struct, simplifying hardware-specific checks.

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> --- 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: High]
This is a pre-existing issue, but does this error return leak the previously 
allocated DMA descriptor rings and RX buffers?

If ravb_dmac_init() fails here (or at ravb_set_opmode earlier), the caller 
ravb_open() branches to out_set_reset, which skips calling ravb_ring_free(). 
Because ravb_open() is reachable via a userspace command like ip link set up, 
could this be triggered repeatedly to exhaust DMA coherent memory?

[ ... ]

> @@ -1124,8 +1124,8 @@ static int ravb_stop_dma(struct net_device *ndev)
>  	}
>  
>  	/* Stop PTP Clock driver */
> -	if (info->gptp)
> -		ravb_ptp_stop(ndev);
> +	if (info->ptp && info->ptp->dmac_stop)
> +		info->ptp->dmac_stop(ndev);

[Severity: Critical]
For Gen3 devices, dmac_stop is NULL since they use ndev_close. If ravb_open()
fails during PHY initialization after the PTP clock has been started, 
ravb_open() jumps to out_ptp_stop and calls ravb_stop_dma(). 

Since dmac_stop is NULL, does the PTP clock remain fully registered? 

If the interface is left down and the module is subsequently removed, 
unregister_netdev() will skip ravb_close(), leaving the PTP clock device 
active. Could userspace then access the leaked /dev/ptpX device and trigger a 
use-after-free on the freed priv pointer?

[ ... ]

> @@ -1967,8 +1957,11 @@ static int ravb_open(struct net_device *ndev)
>  	ravb_emac_init(ndev);
>  
>  	/* Initialise PTP Clock driver */
> -	if (info->ccc_gac)
> -		ravb_ptp_init(ndev);
> +	if (info->ptp && info->ptp->ndev_open) {
> +		error = info->ptp->ndev_open(ndev);
> +		if (error)
> +			goto out_ptp_stop;
> +	}
>  
>  	/* PHY control start */
>  	error = ravb_phy_start(ndev);

[Severity: Critical]
If ravb_phy_start() fails, the code branches to out_ptp_stop, which calls
ravb_stop_dma(ndev). As noted above, ravb_stop_dma() will not clean up the
Gen3 PTP clock because dmac_stop is NULL. 

Should the out_ptp_stop block in ravb_open() explicitly call 
info->ptp->ndev_close(ndev) to ensure the clock is properly unregistered on 
this error path?

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