linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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 <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);
 	/* 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 <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

^ 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 <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);
> 

^ 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 <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);
>

^ 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 <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);
> >

^ 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 <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

^ 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 <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

^ 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).