| Message ID | 20230309100248.3831498-1-zyytlz.wz@163.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: ravb: Fix possible UAF bug in ravb_remove | expand |
Hello! On 3/9/23 1:02 PM, Zheng Wang wrote: > In ravb_probe, priv->work was bound with ravb_tx_timeout_work. > If timeout occurs, it will start the work. And if we call > ravb_remove without finishing the work, ther may be a use > after free bug on ndev. Have you actually encountered it? > Fix it by finishing the job before cleanup in ravb_remove. > > Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") Hm... sorry about that. :-) > Signed-off-by: Zheng Wang <zyytlz.wz@163.com> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> [...] > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 0f54849a3823..07a08e72f440 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -2892,6 +2892,7 @@ static int ravb_remove(struct platform_device *pdev) > struct ravb_private *priv = netdev_priv(ndev); > const struct ravb_hw_info *info = priv->info; > > + cancel_work_sync(&priv->work); I think we need an empty line here... > /* Stop PTP Clock driver */ > if (info->ccc_gac) > ravb_ptp_stop(ndev); MBR, Sergey
On 2023/3/9 18:02, Zheng Wang wrote: > In ravb_probe, priv->work was bound with ravb_tx_timeout_work. > If timeout occurs, it will start the work. And if we call > ravb_remove without finishing the work, ther may be a use ther -> there > after free bug on ndev. > > Fix it by finishing the job before cleanup in ravb_remove. > > Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") > Signed-off-by: Zheng Wang <zyytlz.wz@163.com> > --- > drivers/net/ethernet/renesas/ravb_main.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 0f54849a3823..07a08e72f440 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -2892,6 +2892,7 @@ static int ravb_remove(struct platform_device *pdev) > struct ravb_private *priv = netdev_priv(ndev); > const struct ravb_hw_info *info = priv->info; > > + cancel_work_sync(&priv->work); As your previous patch, I still do not see anything stopping dev_watchdog() from calling dev->netdev_ops->ndo_tx_timeout after cancel_work_sync(), maybe I missed something obvious here? > /* Stop PTP Clock driver */ > if (info->ccc_gac) > ravb_ptp_stop(ndev); >
Sergey Shtylyov <s.shtylyov@omp.ru> 于2023年3月9日周四 23:47写道： > > Hello! > > On 3/9/23 1:02 PM, Zheng Wang wrote: > > > In ravb_probe, priv->work was bound with ravb_tx_timeout_work. > > If timeout occurs, it will start the work. And if we call > > ravb_remove without finishing the work, ther may be a use > > after free bug on ndev. > > Have you actually encountered it? Sorry, I haven't encountered it. All of the analysis is based on static analysis. > > > Fix it by finishing the job before cleanup in ravb_remove. > > > > Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") > > Hm... sorry about that. :-) > > > Signed-off-by: Zheng Wang <zyytlz.wz@163.com> > > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > [...] > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > > index 0f54849a3823..07a08e72f440 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -2892,6 +2892,7 @@ static int ravb_remove(struct platform_device *pdev) > > struct ravb_private *priv = netdev_priv(ndev); > > const struct ravb_hw_info *info = priv->info; > > > > + cancel_work_sync(&priv->work); > > I think we need an empty line here... Yes, will add it in the next version of patch. Best regards, Zheng > > > /* Stop PTP Clock driver */ > > if (info->ccc_gac) > > ravb_ptp_stop(ndev); >
Yunsheng Lin <linyunsheng@huawei.com> 于2023年3月10日周五 09:12写道： > > On 2023/3/9 18:02, Zheng Wang wrote: > > In ravb_probe, priv->work was bound with ravb_tx_timeout_work. > > If timeout occurs, it will start the work. And if we call > > ravb_remove without finishing the work, ther may be a use > > ther -> there > Sorry about the typo, will correct it in the next version. > > after free bug on ndev. > > > > Fix it by finishing the job before cleanup in ravb_remove. > > > > Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") > > Signed-off-by: Zheng Wang <zyytlz.wz@163.com> > > --- > > drivers/net/ethernet/renesas/ravb_main.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > > index 0f54849a3823..07a08e72f440 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -2892,6 +2892,7 @@ static int ravb_remove(struct platform_device *pdev) > > struct ravb_private *priv = netdev_priv(ndev); > > const struct ravb_hw_info *info = priv->info; > > > > + cancel_work_sync(&priv->work); > > As your previous patch, I still do not see anything stopping > dev_watchdog() from calling dev->netdev_ops->ndo_tx_timeout > after cancel_work_sync(), maybe I missed something obvious > here? > Yes, that's a keyed suggestion. I was hurry to report the issue today so wrote with many mistakes. Thanks agagin for the advice. I will review other patch carefully. Best regards, Zheng > > > /* Stop PTP Clock driver */ > > if (info->ccc_gac) > > ravb_ptp_stop(ndev); > >
Hi, On Sat, Mar 11, 2023 at 12:38:10AM +0800, Zheng Hacker wrote: > Yunsheng Lin <linyunsheng@huawei.com> 于2023年3月10日周五 09:12写道： > > > > On 2023/3/9 18:02, Zheng Wang wrote: > > > In ravb_probe, priv->work was bound with ravb_tx_timeout_work. > > > If timeout occurs, it will start the work. And if we call > > > ravb_remove without finishing the work, ther may be a use > > > > ther -> there > > > > Sorry about the typo, will correct it in the next version. > > > > after free bug on ndev. > > > > > > Fix it by finishing the job before cleanup in ravb_remove. > > > > > > Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") > > > Signed-off-by: Zheng Wang <zyytlz.wz@163.com> > > > --- > > > drivers/net/ethernet/renesas/ravb_main.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > > > index 0f54849a3823..07a08e72f440 100644 > > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > > @@ -2892,6 +2892,7 @@ static int ravb_remove(struct platform_device *pdev) > > > struct ravb_private *priv = netdev_priv(ndev); > > > const struct ravb_hw_info *info = priv->info; > > > > > > + cancel_work_sync(&priv->work); > > > > As your previous patch, I still do not see anything stopping > > dev_watchdog() from calling dev->netdev_ops->ndo_tx_timeout > > after cancel_work_sync(), maybe I missed something obvious > > here? > > > Yes, that's a keyed suggestion. I was hurry to report the issue today > so wrote with many mistakes. > Thanks agagin for the advice. I will review other patch carefully. > > Best regards, > Zheng Looking through some older reports and proposed patches, has this even been accepted later? Or did it felt trough the cracks? Regards, Salvatore
Hello! On 9/2/23 5:34 PM, Salvatore Bonaccorso wrote: [...] >>>> In ravb_probe, priv->work was bound with ravb_tx_timeout_work. >>>> If timeout occurs, it will start the work. And if we call >>>> ravb_remove without finishing the work, ther may be a use >>> >>> ther -> there >>> >> >> Sorry about the typo, will correct it in the next version. >> >>>> after free bug on ndev. BTW, is UAF a common abbreviation? I for one didn't know it... >>>> >>>> Fix it by finishing the job before cleanup in ravb_remove. >>>> >>>> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") >>>> Signed-off-by: Zheng Wang <zyytlz.wz@163.com> >>>> --- >>>> drivers/net/ethernet/renesas/ravb_main.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >>>> index 0f54849a3823..07a08e72f440 100644 >>>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c >>>> @@ -2892,6 +2892,7 @@ static int ravb_remove(struct platform_device *pdev) >>>> struct ravb_private *priv = netdev_priv(ndev); >>>> const struct ravb_hw_info *info = priv->info; >>>> >>>> + cancel_work_sync(&priv->work); >>> >>> As your previous patch, I still do not see anything stopping >>> dev_watchdog() from calling dev->netdev_ops->ndo_tx_timeout >>> after cancel_work_sync(), maybe I missed something obvious >>> here? >>> >> Yes, that's a keyed suggestion. I was hurry to report the issue today >> so wrote with many mistakes. >> Thanks agagin for the advice. I will review other patch carefully. >> >> Best regards, >> Zheng > > Looking through some older reports and proposed patches, has this even > been accepted later? No, the latest patch was v4 and it still didn't seem acceptable; I for one don't agree that Zheng does his things in ravb_remove(), not ravb_close(). > Or did it felt trough the cracks? No, there are just too long delays between versions... and the patch still doesn't seem correct enough... :-/ > Regards, > Salvatore MBR, Sergey
Sorry, had to reply from my Gmail account as the work server rejected the msg for unknown reason... :-/ MBR, Sergey
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 0f54849a3823..07a08e72f440 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -2892,6 +2892,7 @@ static int ravb_remove(struct platform_device *pdev) struct ravb_private *priv = netdev_priv(ndev); const struct ravb_hw_info *info = priv->info; + cancel_work_sync(&priv->work); /* Stop PTP Clock driver */ if (info->ccc_gac) ravb_ptp_stop(ndev);
In ravb_probe, priv->work was bound with ravb_tx_timeout_work. If timeout occurs, it will start the work. And if we call ravb_remove without finishing the work, ther may be a use after free bug on ndev. Fix it by finishing the job before cleanup in ravb_remove. Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") Signed-off-by: Zheng Wang <zyytlz.wz@163.com> --- drivers/net/ethernet/renesas/ravb_main.c | 1 + 1 file changed, 1 insertion(+)