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
