RE: [PATCH v4] net: ravb: Fix possible UAF bug in ravb_remove

[Yoshihiro Shimoda]

Hello Sergey,

> From: Sergey Shtylyov, Sent: Wednesday, October 4, 2023 4:39 AM
> 
> Hello!
> 
>    Concerning the subject: I doubt that UAF acronym is known to
> everybody (e.g. it wasn't known to me), I think we should be able
> to afford spelling out use-after-free there...

I got it. I'll change the subject.

> On 9/20/23 5:37 AM, Yoshihiro Shimoda wrote:
> [...]
> 
> >>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >>>>> index 4d6b3b7d6abb..ce2da5101e51 100644
> >>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >>>>> @@ -2885,6 +2885,9 @@ static int ravb_remove(struct platform_device *pdev)
> >>>>>  	struct ravb_private *priv = netdev_priv(ndev);
> >>>>>  	const struct ravb_hw_info *info = priv->info;
> >>>>>
> >>>>> +	netif_carrier_off(ndev);
> >>>>> +	netif_tx_disable(ndev);
> >>>>> +	cancel_work_sync(&priv->work);
> >>>>
> >>>> Still racy, the carrier can come back up after canceling the work.
> >>>
> >>> I must admit I don't see how/when this driver sets the carrier on ?!?
> >>
> >>    The phylib code does it for this MAC driver, see the call tree of
> >> phy_link_change(), on e.g.
> >>
<snip URL>
> >>
> >>>> But whatever, this is a non-issue in the first place.
> >>>
> >>> Do you mean the UaF can't happen? I think that is real.
> >>
> >>    Looks possible to me, at least now... and anyway, shouldn't we clean up
> >> after ourselves if we call schedule_work()?However my current impression is
> >> that cancel_work_sync() should be called from ravb_close(), after calling
> >> phy_{stop|disconnect}()...
> >
> > I also think so.
> >
> > ravb_remove() calls unregister_netdev().
> >  -> unregister_netdev() calls rtnl_lock() and unregister_netdevice().
> >  --> unregiter_netdevice_queue()
> >  ---> unregiter_netdevice_many()
> >  ----> unregiter_netdevice_many_notify().
> >  -----> dev_close_many()
> >  ------> __dev_close_many()
> >  -------> ops->ndo_stop()
> >
> > ravb_close() calls phy_stop()
> >  -> phy_state_machine() with PHY_HALTED
> >  --> phy_link_down()
> >  ---> phy_link_change()
> >  ----> netif_carrier_off()
> 
>    Thanks for sharing the call chain, I've followed it once again... :-)

Thank you :)

> > The patch will be the following:
> > ---
> >  drivers/net/ethernet/renesas/ravb_main.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index 7df9f9f8e134..e452d90de7c2 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -2167,6 +2167,8 @@ static int ravb_close(struct net_device *ndev)
> >  			of_phy_deregister_fixed_link(np);
> >  	}
> >
> > +	cancel_work_sync(&priv->work);
> > +
> >  	if (info->multi_irqs) {
> >  		free_irq(priv->tx_irqs[RAVB_NC], ndev);
> >  		free_irq(priv->rx_irqs[RAVB_NC], ndev);
> > ---
> >
> > If this patch is acceptable, I'll submit it. But, what do you think?
> 
>    I think it should do the job.

Thank you for your comment! I'll make such a patch.

>  And I suspect you can even test it... :-)

IIUC, causing tx timeout is difficult. But, I guess
we can add a fault injection code somehow.

Best regards,
Yoshihiro Shimoda

> > Best regards,
> > Yoshihiro Shimoda
> 
> [...]
> 
> MBR, Sergey