[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <872cf8d7-3bd6-b11a-82ac-a9f4c82d0a02@omp.ru>
Date: Tue, 3 Oct 2023 22:39:20 +0300
From: Sergey Shtylyov <s.shtylyov@....ru>
To: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
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!
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...
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.
>> https://elixir.bootlin.com/linux/v6.5-rc3/source
>>
>>>> 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... :-)
> 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. And I suspect you can even test it... :-)
> Best regards,
> Yoshihiro Shimoda
[...]
MBR, Sergey
Powered by blists - more mailing lists