* [PATCH] staging: rtl8712: check register_netdev() return value
@ 2020-12-09 15:01 shaojie.dong
2020-12-09 15:13 ` Greg KH
2020-12-09 17:46 ` Dan Carpenter
0 siblings, 2 replies; 7+ messages in thread
From: shaojie.dong @ 2020-12-09 15:01 UTC (permalink / raw)
To: Larry.Finger, florian.c.schilhabel, gregkh
Cc: devel, linux-kernel, shaojie.dong
From: "shaojie.dong" <shaojie.dong@isrc.iscas.ac.cn>
Function register_netdev() can fail, so we should check it's return value
Signed-off-by: shaojie.dong <shaojie.dong@isrc.iscas.ac.cn>
---
drivers/staging/rtl8712/hal_init.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8712/hal_init.c b/drivers/staging/rtl8712/hal_init.c
index 715f1fe8b..38a3e3d44 100644
--- a/drivers/staging/rtl8712/hal_init.c
+++ b/drivers/staging/rtl8712/hal_init.c
@@ -45,7 +45,10 @@ static void rtl871x_load_fw_cb(const struct firmware *firmware, void *context)
}
adapter->fw = firmware;
/* firmware available - start netdev */
- register_netdev(adapter->pnetdev);
+ if (register_netdev(adapter->pnetdev) != 0) {
+ netdev_err(adapter->pnetdev, "register_netdev() failed\n");
+ free_netdev(adapter->pnetdev);
+ }
complete(&adapter->rtl8712_fw_ready);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: rtl8712: check register_netdev() return value
2020-12-09 15:01 [PATCH] staging: rtl8712: check register_netdev() return value shaojie.dong
@ 2020-12-09 15:13 ` Greg KH
2020-12-10 15:15 ` shaojie.dong
2020-12-09 17:46 ` Dan Carpenter
1 sibling, 1 reply; 7+ messages in thread
From: Greg KH @ 2020-12-09 15:13 UTC (permalink / raw)
To: shaojie.dong; +Cc: Larry.Finger, florian.c.schilhabel, devel, linux-kernel
On Wed, Dec 09, 2020 at 11:01:24PM +0800, shaojie.dong@isrc.iscas.ac.cn wrote:
> From: "shaojie.dong" <shaojie.dong@isrc.iscas.ac.cn>
>
> Function register_netdev() can fail, so we should check it's return value
>
> Signed-off-by: shaojie.dong <shaojie.dong@isrc.iscas.ac.cn>
I doubt you sign your name with a '.' in it, right?
Please resend with the correct name, and using Capital letters where
needed.
> ---
> drivers/staging/rtl8712/hal_init.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8712/hal_init.c b/drivers/staging/rtl8712/hal_init.c
> index 715f1fe8b..38a3e3d44 100644
> --- a/drivers/staging/rtl8712/hal_init.c
> +++ b/drivers/staging/rtl8712/hal_init.c
> @@ -45,7 +45,10 @@ static void rtl871x_load_fw_cb(const struct firmware *firmware, void *context)
> }
> adapter->fw = firmware;
> /* firmware available - start netdev */
> - register_netdev(adapter->pnetdev);
> + if (register_netdev(adapter->pnetdev) != 0) {
> + netdev_err(adapter->pnetdev, "register_netdev() failed\n");
> + free_netdev(adapter->pnetdev);
> + }
Did you test this to see if this really properly cleans everything up?
And your if statement can be simplified, please do so.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: rtl8712: check register_netdev() return value
2020-12-09 15:01 [PATCH] staging: rtl8712: check register_netdev() return value shaojie.dong
2020-12-09 15:13 ` Greg KH
@ 2020-12-09 17:46 ` Dan Carpenter
2020-12-10 15:05 ` shaojie.dong
1 sibling, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2020-12-09 17:46 UTC (permalink / raw)
To: shaojie.dong
Cc: Larry.Finger, florian.c.schilhabel, gregkh, devel, linux-kernel
On Wed, Dec 09, 2020 at 11:01:24PM +0800, shaojie.dong@isrc.iscas.ac.cn wrote:
> From: "shaojie.dong" <shaojie.dong@isrc.iscas.ac.cn>
>
> Function register_netdev() can fail, so we should check it's return value
>
> Signed-off-by: shaojie.dong <shaojie.dong@isrc.iscas.ac.cn>
> ---
> drivers/staging/rtl8712/hal_init.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8712/hal_init.c b/drivers/staging/rtl8712/hal_init.c
> index 715f1fe8b..38a3e3d44 100644
> --- a/drivers/staging/rtl8712/hal_init.c
> +++ b/drivers/staging/rtl8712/hal_init.c
> @@ -45,7 +45,10 @@ static void rtl871x_load_fw_cb(const struct firmware *firmware, void *context)
> }
> adapter->fw = firmware;
> /* firmware available - start netdev */
> - register_netdev(adapter->pnetdev);
> + if (register_netdev(adapter->pnetdev) != 0) {
> + netdev_err(adapter->pnetdev, "register_netdev() failed\n");
> + free_netdev(adapter->pnetdev);
> + }
This function should not be calling register_netdev(). What does that
have to do with firmware? It should also not free_netdev() because
that will just lead to a use after free in the caller.
regards,
dan carpenter
> complete(&adapter->rtl8712_fw_ready);
> }
>
> --
> 2.17.1
>
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Re: [PATCH] staging: rtl8712: check register_netdev() return value
2020-12-09 17:46 ` Dan Carpenter
@ 2020-12-10 15:05 ` shaojie.dong
2020-12-10 15:16 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: shaojie.dong @ 2020-12-10 15:05 UTC (permalink / raw)
To: Dan Carpenter
Cc: Larry.Finger, florian.c.schilhabel, gregkh, devel, linux-kernel
Hi
>
> This function should not be calling register_netdev(). What does that
> have to do with firmware? It should also not free_netdev() because
> that will just lead to a use after free in the caller.
>
--> check code history author<larry.finger@lwfinger.net> changed synchronous firmware loading to asynchronous firmware loading
before this change, register_netdev() was not calling in firmware related function.
For asynchronous loading, maybe register_netdev() be calling in rtl871x_load_fw_cb() is to ensure the netdev be registered after firmware loading completed
--> for potential use after free issue
Could I only call "free_irq(adapter->pnetdev->irq, adapter->pnetdev)" when register_netdev() failed ?
If no need to change drivers/staging/rtl8712/hal_init.c file, I could give up my patch, thank you !
> -----原始邮件-----
> 发件人: "Dan Carpenter" <dan.carpenter@oracle.com>
> 发送时间: 2020-12-10 01:46:15 (星期四)
> 收件人: shaojie.dong@isrc.iscas.ac.cn
> 抄送: Larry.Finger@lwfinger.net, florian.c.schilhabel@googlemail.com, gregkh@linuxfoundation.org, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org
> 主题: Re: [PATCH] staging: rtl8712: check register_netdev() return value
>
> On Wed, Dec 09, 2020 at 11:01:24PM +0800, shaojie.dong@isrc.iscas.ac.cn wrote:
> > From: "shaojie.dong" <shaojie.dong@isrc.iscas.ac.cn>
> >
> > Function register_netdev() can fail, so we should check it's return value
> >
> > Signed-off-by: shaojie.dong <shaojie.dong@isrc.iscas.ac.cn>
> > ---
> > drivers/staging/rtl8712/hal_init.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/rtl8712/hal_init.c b/drivers/staging/rtl8712/hal_init.c
> > index 715f1fe8b..38a3e3d44 100644
> > --- a/drivers/staging/rtl8712/hal_init.c
> > +++ b/drivers/staging/rtl8712/hal_init.c
> > @@ -45,7 +45,10 @@ static void rtl871x_load_fw_cb(const struct firmware *firmware, void *context)
> > }
> > adapter->fw = firmware;
> > /* firmware available - start netdev */
> > - register_netdev(adapter->pnetdev);
> > + if (register_netdev(adapter->pnetdev) != 0) {
> > + netdev_err(adapter->pnetdev, "register_netdev() failed\n");
> > + free_netdev(adapter->pnetdev);
> > + }
>
> This function should not be calling register_netdev(). What does that
> have to do with firmware? It should also not free_netdev() because
> that will just lead to a use after free in the caller.
>
> regards,
> dan carpenter
>
> > complete(&adapter->rtl8712_fw_ready);
> > }
> >
> > --
> > 2.17.1
> >
> > _______________________________________________
> > devel mailing list
> > devel@linuxdriverproject.org
> > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
</shaojie.dong@isrc.iscas.ac.cn></shaojie.dong@isrc.iscas.ac.cn></dan.carpenter@oracle.com></larry.finger@lwfinger.net>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Re: [PATCH] staging: rtl8712: check register_netdev() return value
2020-12-09 15:13 ` Greg KH
@ 2020-12-10 15:15 ` shaojie.dong
0 siblings, 0 replies; 7+ messages in thread
From: shaojie.dong @ 2020-12-10 15:15 UTC (permalink / raw)
To: Greg KH; +Cc: Larry.Finger, florian.c.schilhabel, devel, linux-kernel
Hi
Thanks ! I will modify sign name correctly later
Sorry to say that I have no rtl8712 hardware, so that I could not test it.
From Dan Carpenter's email reply, "free_netdev(adapter->pnetdev)" function may cause use after free issue
So that I reply email to ensure if this return value should be check or how to handle this return value error
> -----原始邮件-----
> 发件人: "Greg KH" <gregkh@linuxfoundation.org>
> 发送时间: 2020-12-09 23:13:40 (星期三)
> 收件人: shaojie.dong@isrc.iscas.ac.cn
> 抄送: Larry.Finger@lwfinger.net, florian.c.schilhabel@googlemail.com, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org
> 主题: Re: [PATCH] staging: rtl8712: check register_netdev() return value
>
> On Wed, Dec 09, 2020 at 11:01:24PM +0800, shaojie.dong@isrc.iscas.ac.cn wrote:
> > From: "shaojie.dong" <shaojie.dong@isrc.iscas.ac.cn>
> >
> > Function register_netdev() can fail, so we should check it's return value
> >
> > Signed-off-by: shaojie.dong <shaojie.dong@isrc.iscas.ac.cn>
>
> I doubt you sign your name with a '.' in it, right?
>
> Please resend with the correct name, and using Capital letters where
> needed.
>
> > ---
> > drivers/staging/rtl8712/hal_init.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/rtl8712/hal_init.c b/drivers/staging/rtl8712/hal_init.c
> > index 715f1fe8b..38a3e3d44 100644
> > --- a/drivers/staging/rtl8712/hal_init.c
> > +++ b/drivers/staging/rtl8712/hal_init.c
> > @@ -45,7 +45,10 @@ static void rtl871x_load_fw_cb(const struct firmware *firmware, void *context)
> > }
> > adapter->fw = firmware;
> > /* firmware available - start netdev */
> > - register_netdev(adapter->pnetdev);
> > + if (register_netdev(adapter->pnetdev) != 0) {
> > + netdev_err(adapter->pnetdev, "register_netdev() failed\n");
> > + free_netdev(adapter->pnetdev);
> > + }
>
> Did you test this to see if this really properly cleans everything up?
>
> And your if statement can be simplified, please do so.
>
> thanks,
>
> greg k-h
</shaojie.dong@isrc.iscas.ac.cn></shaojie.dong@isrc.iscas.ac.cn></gregkh@linuxfoundation.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Re: [PATCH] staging: rtl8712: check register_netdev() return value
2020-12-10 15:05 ` shaojie.dong
@ 2020-12-10 15:16 ` Dan Carpenter
2020-12-10 15:21 ` shaojie.dong
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2020-12-10 15:16 UTC (permalink / raw)
To: shaojie.dong
Cc: Larry.Finger, florian.c.schilhabel, gregkh, devel, linux-kernel
On Thu, Dec 10, 2020 at 11:05:34PM +0800, shaojie.dong@isrc.iscas.ac.cn wrote:
> Hi
>
> >
> > This function should not be calling register_netdev(). What does that
> > have to do with firmware? It should also not free_netdev() because
> > that will just lead to a use after free in the caller.
> >
>
> --> check code history author<larry.finger@lwfinger.net> changed synchronous firmware loading to asynchronous firmware loading
> before this change, register_netdev() was not calling in firmware related function.
> For asynchronous loading, maybe register_netdev() be calling in rtl871x_load_fw_cb() is to ensure the netdev be registered after firmware loading completed
>
> --> for potential use after free issue
> Could I only call "free_irq(adapter->pnetdev->irq, adapter->pnetdev)" when register_netdev() failed ?
> If no need to change drivers/staging/rtl8712/hal_init.c file, I could give up my patch, thank you !
>
Cleaning this up is a bit complicated and requires reworking the
firmware loading and it requires testing. I don't think you have the
hardware to actually test this driver? Probably, just leave this code
for another day.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Re: Re: [PATCH] staging: rtl8712: check register_netdev() return value
2020-12-10 15:16 ` Dan Carpenter
@ 2020-12-10 15:21 ` shaojie.dong
0 siblings, 0 replies; 7+ messages in thread
From: shaojie.dong @ 2020-12-10 15:21 UTC (permalink / raw)
To: Dan Carpenter
Cc: Larry.Finger, florian.c.schilhabel, gregkh, devel, linux-kernel
Hi
I do not have rtl8712 hardware
So that I would remain this code and give up my patch
Thank you !
> -----原始邮件-----
> 发件人: "Dan Carpenter" <dan.carpenter@oracle.com>
> 发送时间: 2020-12-10 23:16:31 (星期四)
> 收件人: shaojie.dong@isrc.iscas.ac.cn
> 抄送: Larry.Finger@lwfinger.net, florian.c.schilhabel@googlemail.com, gregkh@linuxfoundation.org, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org
> 主题: Re: Re: [PATCH] staging: rtl8712: check register_netdev() return value
>
> On Thu, Dec 10, 2020 at 11:05:34PM +0800, shaojie.dong@isrc.iscas.ac.cn wrote:
> > Hi
> >
> > >
> > > This function should not be calling register_netdev(). What does that
> > > have to do with firmware? It should also not free_netdev() because
> > > that will just lead to a use after free in the caller.
> > >
> >
> > --> check code history author<larry.finger@lwfinger.net> changed synchronous firmware loading to asynchronous firmware loading
> > before this change, register_netdev() was not calling in firmware related function.
> > For asynchronous loading, maybe register_netdev() be calling in rtl871x_load_fw_cb() is to ensure the netdev be registered after firmware loading completed
> >
> > --> for potential use after free issue
> > Could I only call "free_irq(adapter->pnetdev->irq, adapter->pnetdev)" when register_netdev() failed ?
> > If no need to change drivers/staging/rtl8712/hal_init.c file, I could give up my patch, thank you !
> >
>
> Cleaning this up is a bit complicated and requires reworking the
> firmware loading and it requires testing. I don't think you have the
> hardware to actually test this driver? Probably, just leave this code
> for another day.
>
> regards,
> dan carpenter
</larry.finger@lwfinger.net></dan.carpenter@oracle.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-12-10 15:23 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-09 15:01 [PATCH] staging: rtl8712: check register_netdev() return value shaojie.dong
2020-12-09 15:13 ` Greg KH
2020-12-10 15:15 ` shaojie.dong
2020-12-09 17:46 ` Dan Carpenter
2020-12-10 15:05 ` shaojie.dong
2020-12-10 15:16 ` Dan Carpenter
2020-12-10 15:21 ` shaojie.dong
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).