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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 4 Oct 2023 05:05:16 +0000
From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
To: Sergey Shtylyov <s.shtylyov@....ru>, Paolo Abeni <pabeni@...hat.com>,
	Jakub Kicinski <kuba@...nel.org>, Zheng Wang <zyytlz.wz@....com>
CC: "lee@...nel.org" <lee@...nel.org>, "linyunsheng@...wei.com"
	<linyunsheng@...wei.com>, "davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "richardcochran@...il.com"
	<richardcochran@...il.com>, "p.zabel@...gutronix.de"
	<p.zabel@...gutronix.de>, "geert+renesas@...der.be"
	<geert+renesas@...der.be>, "magnus.damm@...il.com" <magnus.damm@...il.com>,
	Biju Das <biju.das.jz@...renesas.com>, "wsa+renesas@...g-engineering.com"
	<wsa+renesas@...g-engineering.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "linux-renesas-soc@...r.kernel.org"
	<linux-renesas-soc@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "hackerzheng666@...il.com"
	<hackerzheng666@...il.com>, "1395428693sheep@...il.com"
	<1395428693sheep@...il.com>, "alex000young@...il.com"
	<alex000young@...il.com>
Subject: RE: [PATCH v4] net: ravb: Fix possible UAF bug in ravb_remove

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