| Message ID | 20231004091253.4194205-3-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [net,1/2] ravb: Fix dma_free_coherent() of desc_bat timing in ravb_remove() | expand |
Hello! Hm, concerning the subject: don't we actually have use-after-free in ravb_tx_timeout() only? Also, you place () after the function names in patch #1 but not in this patch, why? On 10/4/23 12:12 PM, Yoshihiro Shimoda wrote: > The ravb_stop() should call cancel_work_sync(). Otherwise, > ravb_tx_timeout_work() is possible to use the freed priv after > ravb_remove() was called like below: > > CPU0 CPU1 > ravb_tx_timeout() > ravb_remove() > unregister_netdev() > free_netdev(ndev) > // free priv > ravb_tx_timeout_work() > // use priv > > unregister_netdev() will call .ndo_stop() so that ravb_stop() is > called. And, after phy_stop() was called, netif_carrier_off() s/was/is/? > is also called. So that .ndo_tx_timeout() will be not called Will not be... > after phy_stop(). > > Link: https://lore.kernel.org/netdev/872cf8d7-3bd6-b11a-82ac-a9f4c82d0a02@omp.ru/ > Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") > Reported-by: Zheng Wang <zyytlz.wz@163.com> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Otherwise: Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> [...] MBR, Sergey
Hello Sergey, > From: Sergey Shtylyov, Sent: Thursday, October 5, 2023 3:30 AM > > Hello! > > Hm, concerning the subject: don't we actually have use-after-free in ravb_tx_timeout() > only? IIUC, the issue causes ravb_remove(), and is in ravb_tx_timeout_work(). > Also, you place () after the function names in patch #1 but not in this patch, why? I thought that the subject was long so that remove the ()... So, I'll fix the subject as the follow: ravb: Fix use-after-free issue in ravb_tx_timeout_work() > On 10/4/23 12:12 PM, Yoshihiro Shimoda wrote: > > > The ravb_stop() should call cancel_work_sync(). Otherwise, > > ravb_tx_timeout_work() is possible to use the freed priv after > > ravb_remove() was called like below: > > > > CPU0 CPU1 > > ravb_tx_timeout() > > ravb_remove() > > unregister_netdev() > > free_netdev(ndev) > > // free priv > > ravb_tx_timeout_work() > > // use priv > > > > unregister_netdev() will call .ndo_stop() so that ravb_stop() is > > called. And, after phy_stop() was called, netif_carrier_off() > > s/was/is/? I'll fix it. > > is also called. So that .ndo_tx_timeout() will be not called > > Will not be... Oops. I'll fix it. > > after phy_stop(). > > > > Link: <snip URL> > > Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") > > Reported-by: Zheng Wang <zyytlz.wz@163.com> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Otherwise: > > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> Thank you for your review! Best regards, Yoshihiro Shimoda > [...] > > MBR, Sergey
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 9e2e801049cc..0ef0b88b7145 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);
The ravb_stop() should call cancel_work_sync(). Otherwise, ravb_tx_timeout_work() is possible to use the freed priv after ravb_remove() was called like below: CPU0 CPU1 ravb_tx_timeout() ravb_remove() unregister_netdev() free_netdev(ndev) // free priv ravb_tx_timeout_work() // use priv unregister_netdev() will call .ndo_stop() so that ravb_stop() is called. And, after phy_stop() was called, netif_carrier_off() is also called. So that .ndo_tx_timeout() will be not called after phy_stop(). Link: https://lore.kernel.org/netdev/872cf8d7-3bd6-b11a-82ac-a9f4c82d0a02@omp.ru/ Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") Reported-by: Zheng Wang <zyytlz.wz@163.com> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/net/ethernet/renesas/ravb_main.c | 2 ++ 1 file changed, 2 insertions(+)