* [PATCH net] net: ravb: Fix possible UAF bug in ravb_remove
@ 2023-03-09 10:02 Zheng Wang
2023-03-09 15:47 ` Sergey Shtylyov
2023-03-10 1:12 ` Yunsheng Lin
0 siblings, 2 replies; 8+ messages in thread
From: Zheng Wang @ 2023-03-09 10:02 UTC (permalink / raw)
To: s.shtylyov
Cc: davem, edumazet, kuba, pabeni, netdev, linux-kernel,
hackerzheng666, 1395428693sheep, alex000young, Zheng Wang
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 <[email protected]>
---
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);
/* Stop PTP Clock driver */
if (info->ccc_gac)
ravb_ptp_stop(ndev);
--
2.25.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net] net: ravb: Fix possible UAF bug in ravb_remove
2023-03-09 10:02 [PATCH net] net: ravb: Fix possible UAF bug in ravb_remove Zheng Wang
@ 2023-03-09 15:47 ` Sergey Shtylyov
2023-03-10 16:33 ` Zheng Hacker
2023-03-10 1:12 ` Yunsheng Lin
1 sibling, 1 reply; 8+ messages in thread
From: Sergey Shtylyov @ 2023-03-09 15:47 UTC (permalink / raw)
To: Zheng Wang
Cc: davem, edumazet, kuba, pabeni, netdev, linux-kernel,
hackerzheng666, 1395428693sheep, alex000young
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 <[email protected]>
Reviewed-by: Sergey Shtylyov <[email protected]>
[...]
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] net: ravb: Fix possible UAF bug in ravb_remove
2023-03-09 10:02 [PATCH net] net: ravb: Fix possible UAF bug in ravb_remove Zheng Wang
2023-03-09 15:47 ` Sergey Shtylyov
@ 2023-03-10 1:12 ` Yunsheng Lin
2023-03-10 16:38 ` Zheng Hacker
1 sibling, 1 reply; 8+ messages in thread
From: Yunsheng Lin @ 2023-03-10 1:12 UTC (permalink / raw)
To: Zheng Wang, s.shtylyov
Cc: davem, edumazet, kuba, pabeni, netdev, linux-kernel,
hackerzheng666, 1395428693sheep, alex000young
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 <[email protected]>
> ---
> 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);
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] net: ravb: Fix possible UAF bug in ravb_remove
2023-03-09 15:47 ` Sergey Shtylyov
@ 2023-03-10 16:33 ` Zheng Hacker
0 siblings, 0 replies; 8+ messages in thread
From: Zheng Hacker @ 2023-03-10 16:33 UTC (permalink / raw)
To: Sergey Shtylyov
Cc: Zheng Wang, davem, edumazet, kuba, pabeni, netdev, linux-kernel,
1395428693sheep, alex000young
Sergey Shtylyov <[email protected]> 于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 <[email protected]>
>
> Reviewed-by: Sergey Shtylyov <[email protected]>
>
> [...]
> > 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);
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] net: ravb: Fix possible UAF bug in ravb_remove
2023-03-10 1:12 ` Yunsheng Lin
@ 2023-03-10 16:38 ` Zheng Hacker
2023-09-02 14:34 ` Salvatore Bonaccorso
0 siblings, 1 reply; 8+ messages in thread
From: Zheng Hacker @ 2023-03-10 16:38 UTC (permalink / raw)
To: Yunsheng Lin
Cc: Zheng Wang, s.shtylyov, davem, edumazet, kuba, pabeni, netdev,
linux-kernel, 1395428693sheep, alex000young
Yunsheng Lin <[email protected]> 于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 <[email protected]>
> > ---
> > 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);
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] net: ravb: Fix possible UAF bug in ravb_remove
2023-03-10 16:38 ` Zheng Hacker
@ 2023-09-02 14:34 ` Salvatore Bonaccorso
2023-09-03 17:11 ` Sergei Shtylyov
0 siblings, 1 reply; 8+ messages in thread
From: Salvatore Bonaccorso @ 2023-09-02 14:34 UTC (permalink / raw)
To: Zheng Hacker
Cc: Yunsheng Lin, Zheng Wang, s.shtylyov, davem, edumazet, kuba,
pabeni, netdev, linux-kernel, 1395428693sheep, alex000young
Hi,
On Sat, Mar 11, 2023 at 12:38:10AM +0800, Zheng Hacker wrote:
> Yunsheng Lin <[email protected]> 于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 <[email protected]>
> > > ---
> > > 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] net: ravb: Fix possible UAF bug in ravb_remove
2023-09-02 14:34 ` Salvatore Bonaccorso
@ 2023-09-03 17:11 ` Sergei Shtylyov
2023-09-03 17:13 ` Sergei Shtylyov
0 siblings, 1 reply; 8+ messages in thread
From: Sergei Shtylyov @ 2023-09-03 17:11 UTC (permalink / raw)
To: Salvatore Bonaccorso, Zheng Hacker
Cc: Yunsheng Lin, Zheng Wang, davem, edumazet, kuba, pabeni, netdev,
linux-kernel, 1395428693sheep, alex000young
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 <[email protected]>
>>>> ---
>>>> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] net: ravb: Fix possible UAF bug in ravb_remove
2023-09-03 17:11 ` Sergei Shtylyov
@ 2023-09-03 17:13 ` Sergei Shtylyov
0 siblings, 0 replies; 8+ messages in thread
From: Sergei Shtylyov @ 2023-09-03 17:13 UTC (permalink / raw)
To: Salvatore Bonaccorso, Zheng Hacker
Cc: Yunsheng Lin, Zheng Wang, davem, edumazet, kuba, pabeni, netdev,
linux-kernel, 1395428693sheep, alex000young
Sorry, had to reply from my Gmail account as the work server rejected
the msg for unknown reason... :-/
MBR, Sergey
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-09-03 17:14 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-09 10:02 [PATCH net] net: ravb: Fix possible UAF bug in ravb_remove Zheng Wang
2023-03-09 15:47 ` Sergey Shtylyov
2023-03-10 16:33 ` Zheng Hacker
2023-03-10 1:12 ` Yunsheng Lin
2023-03-10 16:38 ` Zheng Hacker
2023-09-02 14:34 ` Salvatore Bonaccorso
2023-09-03 17:11 ` Sergei Shtylyov
2023-09-03 17:13 ` Sergei Shtylyov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).
