* [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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ messages in thread
* Re: [PATCH] staging: rtl8712: check register_netdev() return value
2020-12-06 15:59 shaojie.dong
2020-12-06 16:03 ` Greg KH
@ 2020-12-06 17:10 ` kernel test robot
1 sibling, 0 replies; 10+ messages in thread
From: kernel test robot @ 2020-12-06 17:10 UTC (permalink / raw)
To: shaojie.dong, Larry.Finger, florian.c.schilhabel, gregkh
Cc: kbuild-all, devel, linux-kernel, shaojie.dong
[-- Attachment #1: Type: text/plain, Size: 3276 bytes --]
Hi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on staging/staging-testing]
url: https://github.com/0day-ci/linux/commits/shaojie-dong-isrc-iscas-ac-cn/staging-rtl8712-check-register_netdev-return-value/20201207-000540
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 138f3e1265488a9163be7f379073297ba8545cca
config: arc-allmodconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/a5d44fc70b0f1b3d0a23e3c3bab16a04e4352ad2
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review shaojie-dong-isrc-iscas-ac-cn/staging-rtl8712-check-register_netdev-return-value/20201207-000540
git checkout a5d44fc70b0f1b3d0a23e3c3bab16a04e4352ad2
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from include/linux/device.h:15,
from include/linux/usb/ch9.h:36,
from include/linux/usb.h:6,
from drivers/staging/rtl8712/hal_init.c:19:
drivers/staging/rtl8712/hal_init.c: In function 'rtl871x_load_fw_cb':
>> drivers/staging/rtl8712/hal_init.c:49:12: error: 'udev' undeclared (first use in this function); did you mean 'cdev'?
49 | dev_err(&udev->dev, "r8712u: register_netdev() failed\n");
| ^~~~
include/linux/dev_printk.h:112:11: note: in definition of macro 'dev_err'
112 | _dev_err(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~
drivers/staging/rtl8712/hal_init.c:49:12: note: each undeclared identifier is reported only once for each function it appears in
49 | dev_err(&udev->dev, "r8712u: register_netdev() failed\n");
| ^~~~
include/linux/dev_printk.h:112:11: note: in definition of macro 'dev_err'
112 | _dev_err(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~
vim +49 drivers/staging/rtl8712/hal_init.c
31
32 static void rtl871x_load_fw_cb(const struct firmware *firmware, void *context)
33 {
34 struct _adapter *adapter = context;
35
36 if (!firmware) {
37 struct usb_device *udev = adapter->dvobjpriv.pusbdev;
38 struct usb_interface *usb_intf = adapter->pusb_intf;
39
40 dev_err(&udev->dev, "r8712u: Firmware request failed\n");
41 usb_put_dev(udev);
42 usb_set_intfdata(usb_intf, NULL);
43 complete(&adapter->rtl8712_fw_ready);
44 return;
45 }
46 adapter->fw = firmware;
47 /* firmware available - start netdev */
48 if (register_netdev(adapter->pnetdev) != 0)
> 49 dev_err(&udev->dev, "r8712u: register_netdev() failed\n");
50 complete(&adapter->rtl8712_fw_ready);
51 }
52
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66253 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] staging: rtl8712: check register_netdev() return value
2020-12-06 15:59 shaojie.dong
@ 2020-12-06 16:03 ` Greg KH
2020-12-06 17:10 ` kernel test robot
1 sibling, 0 replies; 10+ messages in thread
From: Greg KH @ 2020-12-06 16:03 UTC (permalink / raw)
To: shaojie.dong; +Cc: Larry.Finger, florian.c.schilhabel, devel, linux-kernel
On Sun, Dec 06, 2020 at 11:59:07PM +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
You just check it, you are not doing anything with it, which is just the
same as not checking this.
Please fix this properly.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] staging: rtl8712: check register_netdev() return value
@ 2020-12-06 15:59 shaojie.dong
2020-12-06 16:03 ` Greg KH
2020-12-06 17:10 ` kernel test robot
0 siblings, 2 replies; 10+ messages in thread
From: shaojie.dong @ 2020-12-06 15:59 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 | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8712/hal_init.c b/drivers/staging/rtl8712/hal_init.c
index 715f1fe8b..fbcc6de1b 100644
--- a/drivers/staging/rtl8712/hal_init.c
+++ b/drivers/staging/rtl8712/hal_init.c
@@ -45,7 +45,8 @@ 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)
+ dev_err(&udev->dev, "r8712u: register_netdev() failed\n");
complete(&adapter->rtl8712_fw_ready);
}
--
2.17.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-12-10 15:23 UTC | newest]
Thread overview: 10+ 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
-- strict thread matches above, loose matches on Subject: below --
2020-12-06 15:59 shaojie.dong
2020-12-06 16:03 ` Greg KH
2020-12-06 17:10 ` kernel test robot
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).