[PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[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, there 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>
Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
v3:
- fix typo in commit message
v2:
- stop dev_watchdog so that handle no more timeout work suggested by Yunsheng Lin,
add an empty line to make code clear suggested by Sergey Shtylyov
---
 drivers/net/ethernet/renesas/ravb_main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 0f54849a3823..eb63ea788e19 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2892,6 +2892,10 @@ 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);
+	
 	/* Stop PTP Clock driver */
 	if (info->ccc_gac)
 		ravb_ptp_stop(ndev);
-- 
2.25.1

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Sergey Shtylyov]

On 3/11/23 9:06 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, there 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>
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

   Well, I haven't reviewed v3 yet...

> ---
> v3:
> - fix typo in commit message
> v2:
> - stop dev_watchdog so that handle no more timeout work suggested by Yunsheng Lin,
> add an empty line to make code clear suggested by Sergey Shtylyov
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 0f54849a3823..eb63ea788e19 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2892,6 +2892,10 @@ 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);
> +	

   Thinking about it again (and looking on some drivers): can ravb_remove() be
called without ravb_close() having been called on the bound devices?
   So I suspect this code should be added to ravb_close()...

[...]

MBR, Sergey

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Yunsheng Lin]

On 2023/3/12 2:06, 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, there 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>
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> v3:
> - fix typo in commit message
> v2:
> - stop dev_watchdog so that handle no more timeout work suggested by Yunsheng Lin,
> add an empty line to make code clear suggested by Sergey Shtylyov
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 0f54849a3823..eb63ea788e19 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2892,6 +2892,10 @@ 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);

LGTM.
Reviewed-by: Yunsheng Lin <linyunsheng@huawei.com>

As noted by Sergey, ravb_remove() and ravb_close() may
share the same handling, but may require some refactoring
in order to do that. So for a fix, it seems the easiest
way to just add the handling here.

> +	
>  	/* Stop PTP Clock driver */
>  	if (info->ccc_gac)
>  		ravb_ptp_stop(ndev);
> 

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Zheng Hacker]

Sergey Shtylyov <s.shtylyov@omp.ru> 于2023年3月13日周一 04:26写道：
>
> On 3/11/23 9:06 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, there 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>
> > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>
>    Well, I haven't reviewed v3 yet...

Please forgive my rudeness, I forgot that..

> > ---
> > v3:
> > - fix typo in commit message
> > v2:
> > - stop dev_watchdog so that handle no more timeout work suggested by Yunsheng Lin,
> > add an empty line to make code clear suggested by Sergey Shtylyov
> > ---
> >  drivers/net/ethernet/renesas/ravb_main.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index 0f54849a3823..eb63ea788e19 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -2892,6 +2892,10 @@ 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);
> > +
>
>    Thinking about it again (and looking on some drivers): can ravb_remove() be
> called without ravb_close() having been called on the bound devices?
>    So I suspect this code should be added to ravb_close()...
>

Yes, as this bug is found by static analysis, I've also seen a lot of
other drivers, many of them put the cancel-work-related code into
*_close as we must close all open file handle before remove a driver.
So I think you'are right. I'll try to add the code to ravb_close.

Best regards,
Zheng

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Zheng Hacker]

Yunsheng Lin <linyunsheng@huawei.com> 于2023年3月13日周一 09:15写道：
>
> On 2023/3/12 2:06, 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, there 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>
> > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> > ---
> > v3:
> > - fix typo in commit message
> > v2:
> > - stop dev_watchdog so that handle no more timeout work suggested by Yunsheng Lin,
> > add an empty line to make code clear suggested by Sergey Shtylyov
> > ---
> >  drivers/net/ethernet/renesas/ravb_main.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index 0f54849a3823..eb63ea788e19 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -2892,6 +2892,10 @@ 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);
>
> LGTM.
> Reviewed-by: Yunsheng Lin <linyunsheng@huawei.com>
>
> As noted by Sergey, ravb_remove() and ravb_close() may
> share the same handling, but may require some refactoring
> in order to do that. So for a fix, it seems the easiest
> way to just add the handling here.

Dear Yunsheng,

I think Sergey is right for I've seen other drivers' same handling
logic. Do you think we should try to move the cancel-work-related code
from ravb_remove to ravb_close funtion?
Appreciate for your precise advice.

Best regards,
Zheng

>
> > +
> >       /* Stop PTP Clock driver */
> >       if (info->ccc_gac)
> >               ravb_ptp_stop(ndev);
> >

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Yunsheng Lin]

On 2023/3/13 11:02, Zheng Hacker wrote:
> Yunsheng Lin <linyunsheng@huawei.com> 于2023年3月13日周一 09:15写道：
>>
>> On 2023/3/12 2:06, 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, there 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>
>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>> ---
>>> v3:
>>> - fix typo in commit message
>>> v2:
>>> - stop dev_watchdog so that handle no more timeout work suggested by Yunsheng Lin,
>>> add an empty line to make code clear suggested by Sergey Shtylyov
>>> ---
>>>  drivers/net/ethernet/renesas/ravb_main.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 0f54849a3823..eb63ea788e19 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -2892,6 +2892,10 @@ 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);
>>
>> LGTM.
>> Reviewed-by: Yunsheng Lin <linyunsheng@huawei.com>
>>
>> As noted by Sergey, ravb_remove() and ravb_close() may
>> share the same handling, but may require some refactoring
>> in order to do that. So for a fix, it seems the easiest
>> way to just add the handling here.
> 
> Dear Yunsheng,
> 
> I think Sergey is right for I've seen other drivers' same handling
> logic. Do you think we should try to move the cancel-work-related code
> from ravb_remove to ravb_close funtion?
> Appreciate for your precise advice.

As Sergey question "can ravb_remove() be called without ravb_close()
having been called on the bound devices?"
If I understand it correctly, I think ravb_remove() can be called
without ravb_close() having been called on the bound devices. I am
happy to be corrected if I am wrong.

Yes, you can call *_close() directly in *_remove(), but that may
require some refactoring and a lot of testing.

Also, if you found the bug through some static analysis, it may
be better to make it clear in the commit log and share some info
about the static analysis, which I suppose it is a tool?

> 
> Best regards,
> Zheng
> 
>>
>>> +
>>>       /* Stop PTP Clock driver */
>>>       if (info->ccc_gac)
>>>               ravb_ptp_stop(ndev);
>>>
> .
> 

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Zheng Hacker]

Yunsheng Lin <linyunsheng@huawei.com> 于2023年3月13日周一 11:32写道：
>
> On 2023/3/13 11:02, Zheng Hacker wrote:
> > Yunsheng Lin <linyunsheng@huawei.com> 于2023年3月13日周一 09:15写道：
> >>
> >> On 2023/3/12 2:06, 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, there 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>
> >>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> >>> ---
> >>> v3:
> >>> - fix typo in commit message
> >>> v2:
> >>> - stop dev_watchdog so that handle no more timeout work suggested by Yunsheng Lin,
> >>> add an empty line to make code clear suggested by Sergey Shtylyov
> >>> ---
> >>>  drivers/net/ethernet/renesas/ravb_main.c | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >>> index 0f54849a3823..eb63ea788e19 100644
> >>> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >>> @@ -2892,6 +2892,10 @@ 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);
> >>
> >> LGTM.
> >> Reviewed-by: Yunsheng Lin <linyunsheng@huawei.com>
> >>
> >> As noted by Sergey, ravb_remove() and ravb_close() may
> >> share the same handling, but may require some refactoring
> >> in order to do that. So for a fix, it seems the easiest
> >> way to just add the handling here.
> >
> > Dear Yunsheng,
> >
> > I think Sergey is right for I've seen other drivers' same handling
> > logic. Do you think we should try to move the cancel-work-related code
> > from ravb_remove to ravb_close funtion?
> > Appreciate for your precise advice.
>
> As Sergey question "can ravb_remove() be called without ravb_close()
> having been called on the bound devices?"
> If I understand it correctly, I think ravb_remove() can be called
> without ravb_close() having been called on the bound devices. I am
> happy to be corrected if I am wrong.
>

Hi Yunsheng,

I'm still not sure. I'll look at code more carefully and see if there
is more proof about it.
And as I'm not familiar with the related code, let's see how Sergey thnks.

> Yes, you can call *_close() directly in *_remove(), but that may
> require some refactoring and a lot of testing.

>
> Also, if you found the bug through some static analysis, it may
> be better to make it clear in the commit log and share some info
> about the static analysis, which I suppose it is a tool?

Yes, I'll append this msg to commit msg later.

> >
> >>
> >>> +
> >>>       /* Stop PTP Clock driver */
> >>>       if (info->ccc_gac)
> >>>               ravb_ptp_stop(ndev);
> >>>
> > .
> >

Best regards,
Zheng

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Jakub Kicinski]

On Sun, 12 Mar 2023 02:06:30 +0800 Zheng Wang wrote:
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")

You must CC all people involved in a commit if you put it as Fixes.
Are you using the get_maintainer.pl script?
How do you call in exactly?

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Zheng Hacker]

Jakub Kicinski <kuba@kernel.org> 于2023年3月14日周二 06:39写道：
>
> On Sun, 12 Mar 2023 02:06:30 +0800 Zheng Wang wrote:
> > Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
>
> You must CC all people involved in a commit if you put it as Fixes.
Hi Jakub,

Get it.

> Are you using the get_maintainer.pl script?
> How do you call in exactly?

Yes, I used this script to find developers involved but It seems that
I unintentionally forgot to CC some people.

I apologize for my offense for everyone exclued in the list.

Thanks,
Zheng

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Lee Jones]

On Sun, 12 Mar 2023, 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, there 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>
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> v3:
> - fix typo in commit message
> v2:
> - stop dev_watchdog so that handle no more timeout work suggested by Yunsheng Lin,
> add an empty line to make code clear suggested by Sergey Shtylyov
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 4 ++++
>  1 file changed, 4 insertions(+)

Was a follow-up to this ever sent?

-- 
Lee Jones [李琼斯]

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Lee Jones]

On Sun, 12 Mar 2023, 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, there 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>
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> v3:
> - fix typo in commit message
> v2:
> - stop dev_watchdog so that handle no more timeout work suggested by Yunsheng Lin,
> add an empty line to make code clear suggested by Sergey Shtylyov
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 4 ++++
>  1 file changed, 4 insertions(+)

For better or worse, it looks like this issue was assigned a CVE.

Are we expecting v4 or was it resolved in another way?

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 0f54849a3823..eb63ea788e19 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2892,6 +2892,10 @@ 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);
> +	
>  	/* Stop PTP Clock driver */
>  	if (info->ccc_gac)
>  		ravb_ptp_stop(ndev);
> -- 
> 2.25.1
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Jakub Kicinski]

On Mon, 10 Jul 2023 12:42:53 +0100 Lee Jones wrote:
> For better or worse, it looks like this issue was assigned a CVE.

Ugh, what a joke. 

Sergey, could you take a look at fixing this properly?

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Sergey Shtylyov]

On 7/10/23 7:15 PM, Jakub Kicinski wrote:
[...]

>> For better or worse, it looks like this issue was assigned a CVE.
> 
> Ugh, what a joke. 
> 
> Sergey, could you take a look at fixing this properly?

   OK, started looking at it again...
   I have no h/w anymore but I'm hoping to find a tester on #renesas-soc...

MBR, Sergey

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Lee Jones]

On Mon, 10 Jul 2023, Jakub Kicinski wrote:

> On Mon, 10 Jul 2023 12:42:53 +0100 Lee Jones wrote:
> > For better or worse, it looks like this issue was assigned a CVE.
> 
> Ugh, what a joke. 

I think that's putting it politely. :)

-- 
Lee Jones [李琼斯]

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Zheng Hacker]

Sorry for my late reply. I'll see what I can do later.

Lee Jones <lee@kernel.org> 于2023年7月12日周三 19:56写道：
>
> On Mon, 10 Jul 2023, Jakub Kicinski wrote:
>
> > On Mon, 10 Jul 2023 12:42:53 +0100 Lee Jones wrote:
> > > For better or worse, it looks like this issue was assigned a CVE.
> >
> > Ugh, what a joke.
>
> I think that's putting it politely. :)
>
> --
> Lee Jones [李琼斯]

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Sergey Shtylyov]

On 7/15/23 7:07 PM, Zheng Hacker wrote:

> Sorry for my late reply. I'll see what I can do later.

   That's good to hear!
   Because I'm now only able to look at it during weekends...

> Lee Jones <lee@kernel.org> 于2023年7月12日周三 19:56写道：
>>
>> On Mon, 10 Jul 2023, Jakub Kicinski wrote:
>>
>>> On Mon, 10 Jul 2023 12:42:53 +0100 Lee Jones wrote:
>>>> For better or worse, it looks like this issue was assigned a CVE.
>>>
>>> Ugh, what a joke.
>>
>> I think that's putting it politely. :)
>>
>> --
>> Lee Jones [李琼斯]

MBR, Sergey

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Zheng Hacker]

Hello,

This bug is found by static analysis. I'm sorry that my friends apply
for a CVE number before we really fix it. We made a list about the
bugs we have submitted and wouldn't disclose them before the fix. But
we had a inconsistent situation last month. And we applied it by
mistake foe we thought we had fixed it. And so sorry about my late
reply, I'll see the patch right now.

Best regards,
Zheng Wang

Sergey Shtylyov <s.shtylyov@omp.ru> 于2023年7月16日周日 04:48写道：
>
> On 7/15/23 7:07 PM, Zheng Hacker wrote:
>
> > Sorry for my late reply. I'll see what I can do later.
>
>    That's good to hear!
>    Because I'm now only able to look at it during weekends...
>
> > Lee Jones <lee@kernel.org> 于2023年7月12日周三 19:56写道：
> >>
> >> On Mon, 10 Jul 2023, Jakub Kicinski wrote:
> >>
> >>> On Mon, 10 Jul 2023 12:42:53 +0100 Lee Jones wrote:
> >>>> For better or worse, it looks like this issue was assigned a CVE.
> >>>
> >>> Ugh, what a joke.
> >>
> >> I think that's putting it politely. :)
> >>
> >> --
> >> Lee Jones [李琼斯]
>
> MBR, Sergey

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Zheng Hacker]

Hello,

After reviewing the code, I think it's better to put the code in
ravb_remove. For the ravb_remove is bound with the device and
ravb_close is bound with the file. We may not call ravb_close if
there's no file opened.

Thanks,
Zheng

Zheng Hacker <hackerzheng666@gmail.com> 于2023年7月16日周日 10:11写道：
>
> Hello,
>
> This bug is found by static analysis. I'm sorry that my friends apply
> for a CVE number before we really fix it. We made a list about the
> bugs we have submitted and wouldn't disclose them before the fix. But
> we had a inconsistent situation last month. And we applied it by
> mistake foe we thought we had fixed it. And so sorry about my late
> reply, I'll see the patch right now.
>
> Best regards,
> Zheng Wang
>
> Sergey Shtylyov <s.shtylyov@omp.ru> 于2023年7月16日周日 04:48写道：
> >
> > On 7/15/23 7:07 PM, Zheng Hacker wrote:
> >
> > > Sorry for my late reply. I'll see what I can do later.
> >
> >    That's good to hear!
> >    Because I'm now only able to look at it during weekends...
> >
> > > Lee Jones <lee@kernel.org> 于2023年7月12日周三 19:56写道：
> > >>
> > >> On Mon, 10 Jul 2023, Jakub Kicinski wrote:
> > >>
> > >>> On Mon, 10 Jul 2023 12:42:53 +0100 Lee Jones wrote:
> > >>>> For better or worse, it looks like this issue was assigned a CVE.
> > >>>
> > >>> Ugh, what a joke.
> > >>
> > >> I think that's putting it politely. :)
> > >>
> > >> --
> > >> Lee Jones [李琼斯]
> >
> > MBR, Sergey

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Lee Jones]

On Sun, 16 Jul 2023, Zheng Hacker wrote:
> Zheng Hacker <hackerzheng666@gmail.com> 于2023年7月16日周日 10:11写道：
> >
> > Hello,
> >
> > This bug is found by static analysis. I'm sorry that my friends apply
> > for a CVE number before we really fix it. We made a list about the
> > bugs we have submitted and wouldn't disclose them before the fix. But
> > we had a inconsistent situation last month. And we applied it by
> > mistake foe we thought we had fixed it. And so sorry about my late
> > reply, I'll see the patch right now.
> >
> > Best regards,
> > Zheng Wang
> >
> > Sergey Shtylyov <s.shtylyov@omp.ru> 于2023年7月16日周日 04:48写道：
> > >
> > > On 7/15/23 7:07 PM, Zheng Hacker wrote:
> > >
> > > > Sorry for my late reply. I'll see what I can do later.
> > >
> > >    That's good to hear!
> > >    Because I'm now only able to look at it during weekends...
> > >
> > > > Lee Jones <lee@kernel.org> 于2023年7月12日周三 19:56写道：
> > > >>
> > > >> On Mon, 10 Jul 2023, Jakub Kicinski wrote:
> > > >>
> > > >>> On Mon, 10 Jul 2023 12:42:53 +0100 Lee Jones wrote:
> > > >>>> For better or worse, it looks like this issue was assigned a CVE.
> > > >>>
> > > >>> Ugh, what a joke.
> > > >>
> > > >> I think that's putting it politely. :)
>
> After reviewing the code, I think it's better to put the code in
> ravb_remove. For the ravb_remove is bound with the device and
> ravb_close is bound with the file. We may not call ravb_close if
> there's no file opened.

When you do submit this, would you be kind enough to Cc me please?

-- 
Lee Jones [李琼斯]

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Zheng Hacker]

Lee Jones <lee@kernel.org> 于2023年7月17日周一 21:04写道：
>
> On Sun, 16 Jul 2023, Zheng Hacker wrote:
> > Zheng Hacker <hackerzheng666@gmail.com> 于2023年7月16日周日 10:11写道：
> > >
> > > Hello,
> > >
> > > This bug is found by static analysis. I'm sorry that my friends apply
> > > for a CVE number before we really fix it. We made a list about the
> > > bugs we have submitted and wouldn't disclose them before the fix. But
> > > we had a inconsistent situation last month. And we applied it by
> > > mistake foe we thought we had fixed it. And so sorry about my late
> > > reply, I'll see the patch right now.
> > >
> > > Best regards,
> > > Zheng Wang
> > >
> > > Sergey Shtylyov <s.shtylyov@omp.ru> 于2023年7月16日周日 04:48写道：
> > > >
> > > > On 7/15/23 7:07 PM, Zheng Hacker wrote:
> > > >
> > > > > Sorry for my late reply. I'll see what I can do later.
> > > >
> > > >    That's good to hear!
> > > >    Because I'm now only able to look at it during weekends...
> > > >
> > > > > Lee Jones <lee@kernel.org> 于2023年7月12日周三 19:56写道：
> > > > >>
> > > > >> On Mon, 10 Jul 2023, Jakub Kicinski wrote:
> > > > >>
> > > > >>> On Mon, 10 Jul 2023 12:42:53 +0100 Lee Jones wrote:
> > > > >>>> For better or worse, it looks like this issue was assigned a CVE.
> > > > >>>
> > > > >>> Ugh, what a joke.
> > > > >>
> > > > >> I think that's putting it politely. :)
> >
> > After reviewing the code, I think it's better to put the code in
> > ravb_remove. For the ravb_remove is bound with the device and
> > ravb_close is bound with the file. We may not call ravb_close if
> > there's no file opened.
>
> When you do submit this, would you be kind enough to Cc me please?
>

Oh sorry for my rudeness. I use reply to all in gmail and it didn't
add new people from conversation.

MBR,
Zheng
> --
> Lee Jones [李琼斯]

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Sergey Shtylyov]

Hello!

On 3/13/23 6:32 AM, Yunsheng Lin wrote:
[...]

>>> On 2023/3/12 2:06, 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, there 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>
>>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>>> ---
>>>> v3:
>>>> - fix typo in commit message
>>>> v2:
>>>> - stop dev_watchdog so that handle no more timeout work suggested by Yunsheng Lin,
>>>> add an empty line to make code clear suggested by Sergey Shtylyov
>>>> ---
>>>>  drivers/net/ethernet/renesas/ravb_main.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>>> index 0f54849a3823..eb63ea788e19 100644
>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>> @@ -2892,6 +2892,10 @@ 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);
>>>
>>> LGTM.
>>> Reviewed-by: Yunsheng Lin <linyunsheng@huawei.com>
>>>
>>> As noted by Sergey, ravb_remove() and ravb_close() may
>>> share the same handling, but may require some refactoring
>>> in order to do that. So for a fix, it seems the easiest
>>> way to just add the handling here.
>>
>> Dear Yunsheng,
>>
>> I think Sergey is right for I've seen other drivers' same handling
>> logic. Do you think we should try to move the cancel-work-related code
>> from ravb_remove to ravb_close funtion?
>> Appreciate for your precise advice.
> 
> As Sergey question "can ravb_remove() be called without ravb_close()
> having been called on the bound devices?"
> If I understand it correctly, I think ravb_remove() can be called
> without ravb_close() having been called on the bound devices. I am
> happy to be corrected if I am wrong.

   Yes, correct. It's ravb_remove() that calls unregister_netdev()
which results in calling ravb_close() on the opened devices...

> Yes, you can call *_close() directly in *_remove(), but that may
> require some refactoring and a lot of testing.

   No need to do that I think, as it's called anyways...

> Also, if you found the bug through some static analysis, it may
> be better to make it clear in the commit log and share some info
> about the static analysis, which I suppose it is a tool?

   Agreed. :-)

>> Best regards,
>> Zheng

MBR, Sergey

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Lee Jones]

On Mon, 17 Jul 2023, Lee Jones wrote:

> On Sun, 16 Jul 2023, Zheng Hacker wrote:
> > Zheng Hacker <hackerzheng666@gmail.com> 于2023年7月16日周日 10:11写道：
> > >
> > > Hello,
> > >
> > > This bug is found by static analysis. I'm sorry that my friends apply
> > > for a CVE number before we really fix it. We made a list about the
> > > bugs we have submitted and wouldn't disclose them before the fix. But
> > > we had a inconsistent situation last month. And we applied it by
> > > mistake foe we thought we had fixed it. And so sorry about my late
> > > reply, I'll see the patch right now.
> > >
> > > Best regards,
> > > Zheng Wang
> > >
> > > Sergey Shtylyov <s.shtylyov@omp.ru> 于2023年7月16日周日 04:48写道：
> > > >
> > > > On 7/15/23 7:07 PM, Zheng Hacker wrote:
> > > >
> > > > > Sorry for my late reply. I'll see what I can do later.
> > > >
> > > >    That's good to hear!
> > > >    Because I'm now only able to look at it during weekends...
> > > >
> > > > > Lee Jones <lee@kernel.org> 于2023年7月12日周三 19:56写道：
> > > > >>
> > > > >> On Mon, 10 Jul 2023, Jakub Kicinski wrote:
> > > > >>
> > > > >>> On Mon, 10 Jul 2023 12:42:53 +0100 Lee Jones wrote:
> > > > >>>> For better or worse, it looks like this issue was assigned a CVE.
> > > > >>>
> > > > >>> Ugh, what a joke.
> > > > >>
> > > > >> I think that's putting it politely. :)
> >
> > After reviewing the code, I think it's better to put the code in
> > ravb_remove. For the ravb_remove is bound with the device and
> > ravb_close is bound with the file. We may not call ravb_close if
> > there's no file opened.
> 
> When you do submit this, would you be kind enough to Cc me please?

Could I trouble you for an update on this please?

Have you submitted v4 yet?

-- 
Lee Jones [李琼斯]

Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Zheng Hacker]

Lee Jones <lee@kernel.org> 于2023年7月24日周一 17:21写道：
>
> On Mon, 17 Jul 2023, Lee Jones wrote:
>
> > On Sun, 16 Jul 2023, Zheng Hacker wrote:
> > > Zheng Hacker <hackerzheng666@gmail.com> 于2023年7月16日周日 10:11写道：
> > > >
> > > > Hello,
> > > >
> > > > This bug is found by static analysis. I'm sorry that my friends apply
> > > > for a CVE number before we really fix it. We made a list about the
> > > > bugs we have submitted and wouldn't disclose them before the fix. But
> > > > we had a inconsistent situation last month. And we applied it by
> > > > mistake foe we thought we had fixed it. And so sorry about my late
> > > > reply, I'll see the patch right now.
> > > >
> > > > Best regards,
> > > > Zheng Wang
> > > >
> > > > Sergey Shtylyov <s.shtylyov@omp.ru> 于2023年7月16日周日 04:48写道：
> > > > >
> > > > > On 7/15/23 7:07 PM, Zheng Hacker wrote:
> > > > >
> > > > > > Sorry for my late reply. I'll see what I can do later.
> > > > >
> > > > >    That's good to hear!
> > > > >    Because I'm now only able to look at it during weekends...
> > > > >
> > > > > > Lee Jones <lee@kernel.org> 于2023年7月12日周三 19:56写道：
> > > > > >>
> > > > > >> On Mon, 10 Jul 2023, Jakub Kicinski wrote:
> > > > > >>
> > > > > >>> On Mon, 10 Jul 2023 12:42:53 +0100 Lee Jones wrote:
> > > > > >>>> For better or worse, it looks like this issue was assigned a CVE.
> > > > > >>>
> > > > > >>> Ugh, what a joke.
> > > > > >>
> > > > > >> I think that's putting it politely. :)
> > >
> > > After reviewing the code, I think it's better to put the code in
> > > ravb_remove. For the ravb_remove is bound with the device and
> > > ravb_close is bound with the file. We may not call ravb_close if
> > > there's no file opened.
> >
> > When you do submit this, would you be kind enough to Cc me please?
>
> Could I trouble you for an update on this please?
>
> Have you submitted v4 yet?

Sorry, will do right now.

Best regards,
Zheng
>
> --
> Lee Jones [李琼斯]