Re: [PATCH] net: gianfar: fix use-after-free in gfar_enet_open on startup_gfar failure

[Simon Horman <horms@kernel.org>]

On Sat, Jul 04, 2026 at 04:47:34PM -0700, Rosen Penev wrote:
> If startup_gfar() fails in gfar_enet_open(), the PHY remains connected
> and IRQs remain requested. Since ndo_open returned an error, IFF_UP is
> not set and ndo_stop (gfar_close) will not be called on unregister,
> leaving the IRQ handlers registered against freed net_device memory
> when the driver is removed.
> 
> Add proper error unwinding: phy_disconnect() and gfar_free_irq() before
> returning the error.
> 
> Fixes: 80ec396cb6b5 ("gianfar: Don't free/request irqs on device reset")
> 

nit: no blank line here

> Assisted-by: Opencode:Big-Pickle
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/net/ethernet/freescale/gianfar.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index 89215e1ddc2d..4b3a5eaadfb5 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -2878,8 +2878,11 @@ static int gfar_enet_open(struct net_device *dev)
>  		return err;
>  
>  	err = startup_gfar(dev);
> -	if (err)
> +	if (err) {
> +		phy_disconnect(dev->phydev);
> +		gfar_free_irq(priv);
>  		return err;
> +	}
>  
>  	return err;
>  }

The AI-generated review on https://netdev-ai.bots.linux.dev/sashiko/ has
the following to say. I'd suggest addressing this using a more
comprehensive patch that makes use of an idiomatic goto ladder to unwind on
error.

  Should the gfar_request_irq() failure path also disconnect the PHY?
  Looking at gfar_enet_open() after this patch:
      err = init_phy(dev);
      if (err)
          return err;
      err = gfar_request_irq(priv);
      if (err)
          return err;
      err = startup_gfar(dev);
      if (err) {
          phy_disconnect(dev->phydev);
          gfar_free_irq(priv);
          return err;
      }
  init_phy() calls of_phy_connect(), which installs the driver's
  adjust_link callback (holding a net_device pointer) into the phy_device.
  If gfar_request_irq() then fails, the function returns without calling
  phy_disconnect().
  Since ndo_open returned an error, IFF_UP is not set and gfar_close()
  will not be called by unregister_netdev(). When the driver is later
  removed (gfar_remove -> unregister_netdev -> free_gfar_dev), the
  net_device is freed while the phy_device still holds a reference to
  it via adjust_link, which is the same use-after-free scenario the
  commit message describes.
  This pre-existed the patch, but since the changelog says it fixes the
  failure of gfar_enet_open() leaving stale references, would it make
  sense to also add phy_disconnect(dev->phydev) to the gfar_request_irq()
  error path, e.g.:
      err = gfar_request_irq(priv);
      if (err) {
          phy_disconnect(dev->phydev);
          return err;
      }