* [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind @ 2021-06-14 15:37 Dongliang Mu 2021-06-14 16:00 ` Pavel Skripkin 2021-06-15 7:38 ` Greg KH 0 siblings, 2 replies; 13+ messages in thread From: Dongliang Mu @ 2021-06-14 15:37 UTC (permalink / raw) To: steve.glendinning, davem, kuba, paskripkin Cc: netdev, linux-usb, linux-kernel, Dongliang Mu The commit 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") fails to clean up the work scheduled in smsc75xx_reset-> smsc75xx_set_multicast, which leads to use-after-free if the work is scheduled to start after the deallocation. In addition, this patch also removes one dangling pointer - dev->data[0]. This patch calls cancel_work_sync to cancel the schedule work and set the dangling pointer to NULL. Fixes: 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> --- drivers/net/usb/smsc75xx.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c index b286993da67c..f81740fcc8d5 100644 --- a/drivers/net/usb/smsc75xx.c +++ b/drivers/net/usb/smsc75xx.c @@ -1504,7 +1504,10 @@ static int smsc75xx_bind(struct usbnet *dev, struct usb_interface *intf) return 0; err: + cancel_work_sync(&pdata->set_multicast); kfree(pdata); + pdata = NULL; + dev->data[0] = 0; return ret; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind 2021-06-14 15:37 [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind Dongliang Mu @ 2021-06-14 16:00 ` Pavel Skripkin 2021-06-14 23:01 ` Dongliang Mu 2021-06-15 7:38 ` Greg KH 1 sibling, 1 reply; 13+ messages in thread From: Pavel Skripkin @ 2021-06-14 16:00 UTC (permalink / raw) To: Dongliang Mu Cc: steve.glendinning, davem, kuba, netdev, linux-usb, linux-kernel On Mon, 14 Jun 2021 23:37:12 +0800 Dongliang Mu <mudongliangabcd@gmail.com> wrote: > The commit 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > fails to clean up the work scheduled in smsc75xx_reset-> > smsc75xx_set_multicast, which leads to use-after-free if the work is > scheduled to start after the deallocation. In addition, this patch > also removes one dangling pointer - dev->data[0]. > > This patch calls cancel_work_sync to cancel the schedule work and set > the dangling pointer to NULL. > > Fixes: 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > --- > drivers/net/usb/smsc75xx.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c > index b286993da67c..f81740fcc8d5 100644 > --- a/drivers/net/usb/smsc75xx.c > +++ b/drivers/net/usb/smsc75xx.c > @@ -1504,7 +1504,10 @@ static int smsc75xx_bind(struct usbnet *dev, > struct usb_interface *intf) return 0; > > err: > + cancel_work_sync(&pdata->set_multicast); > kfree(pdata); > + pdata = NULL; > + dev->data[0] = 0; > return ret; > } > Hi, Dongliang! Just my thougth about this patch: INIT_WORK(&pdata->set_multicast, smsc75xx_deferred_multicast_write); does not queue anything, it just initalizes list structure and assigns callback function. The actual work sheduling happens in smsc75xx_set_multicast() which is smsc75xx_netdev_ops member. In case of any error in smsc75xx_bind() the device registration fails and smsc75xx_netdev_ops won't be registered, so, i guess, there is no chance of UAF. Am I missing something? :) With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind 2021-06-14 16:00 ` Pavel Skripkin @ 2021-06-14 23:01 ` Dongliang Mu 2021-06-15 13:31 ` Pavel Skripkin 0 siblings, 1 reply; 13+ messages in thread From: Dongliang Mu @ 2021-06-14 23:01 UTC (permalink / raw) To: Pavel Skripkin Cc: Steve Glendinning, David S. Miller, Jakub Kicinski, netdev, linux-usb, linux-kernel On Tue, Jun 15, 2021 at 12:00 AM Pavel Skripkin <paskripkin@gmail.com> wrote: > > On Mon, 14 Jun 2021 23:37:12 +0800 > Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > > The commit 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > > fails to clean up the work scheduled in smsc75xx_reset-> > > smsc75xx_set_multicast, which leads to use-after-free if the work is > > scheduled to start after the deallocation. In addition, this patch > > also removes one dangling pointer - dev->data[0]. > > > > This patch calls cancel_work_sync to cancel the schedule work and set > > the dangling pointer to NULL. > > > > Fixes: 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > --- > > drivers/net/usb/smsc75xx.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c > > index b286993da67c..f81740fcc8d5 100644 > > --- a/drivers/net/usb/smsc75xx.c > > +++ b/drivers/net/usb/smsc75xx.c > > @@ -1504,7 +1504,10 @@ static int smsc75xx_bind(struct usbnet *dev, > > struct usb_interface *intf) return 0; > > > > err: > > + cancel_work_sync(&pdata->set_multicast); > > kfree(pdata); > > + pdata = NULL; > > + dev->data[0] = 0; > > return ret; > > } > > > > Hi, Dongliang! > > Just my thougth about this patch: > > INIT_WORK(&pdata->set_multicast, smsc75xx_deferred_multicast_write); > does not queue anything, it just initalizes list structure and assigns > callback function. The actual work sheduling happens in > smsc75xx_set_multicast() which is smsc75xx_netdev_ops member. > Yes, you are right. However, as written in the commit message, smsc75xx_set_multicast will be called by smsc75xx_reset [1]. If smsc75xx_set_multicast is called before any check failure occurs, this work(set_multicast) will be queued into the global list with schedule_work(&pdata->set_multicast); [2] At last, if the pdata or dev->data[0] is freed before the set_multicast really executes, it may lead to a UAF. Is this correct? BTW, even if the above is true, I don't know if I call the API ``cancel_work_sync(&pdata->set_multicast)'' properly if the schedule_work is not called. [1] https://elixir.bootlin.com/linux/latest/source/drivers/net/usb/smsc75xx.c#L1322 [2] https://elixir.bootlin.com/linux/latest/source/drivers/net/usb/smsc75xx.c#L583 > In case of any error in smsc75xx_bind() the device registration fails > and smsc75xx_netdev_ops won't be registered, so, i guess, there is no > chance of UAF. > > > Am I missing something? :) > > > > With regards, > Pavel Skripkin ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind 2021-06-14 23:01 ` Dongliang Mu @ 2021-06-15 13:31 ` Pavel Skripkin 2021-06-16 2:16 ` Dongliang Mu 0 siblings, 1 reply; 13+ messages in thread From: Pavel Skripkin @ 2021-06-15 13:31 UTC (permalink / raw) To: Dongliang Mu Cc: Steve Glendinning, David S. Miller, Jakub Kicinski, netdev, linux-usb, linux-kernel On Tue, 15 Jun 2021 07:01:13 +0800 Dongliang Mu <mudongliangabcd@gmail.com> wrote: > On Tue, Jun 15, 2021 at 12:00 AM Pavel Skripkin > <paskripkin@gmail.com> wrote: > > > > On Mon, 14 Jun 2021 23:37:12 +0800 > > Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > > > > The commit 46a8b29c6306 ("net: usb: fix memory leak in > > > smsc75xx_bind") fails to clean up the work scheduled in > > > smsc75xx_reset-> smsc75xx_set_multicast, which leads to > > > use-after-free if the work is scheduled to start after the > > > deallocation. In addition, this patch also removes one dangling > > > pointer - dev->data[0]. > > > > > > This patch calls cancel_work_sync to cancel the schedule work and > > > set the dangling pointer to NULL. > > > > > > Fixes: 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > > --- > > > drivers/net/usb/smsc75xx.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/net/usb/smsc75xx.c > > > b/drivers/net/usb/smsc75xx.c index b286993da67c..f81740fcc8d5 > > > 100644 --- a/drivers/net/usb/smsc75xx.c > > > +++ b/drivers/net/usb/smsc75xx.c > > > @@ -1504,7 +1504,10 @@ static int smsc75xx_bind(struct usbnet > > > *dev, struct usb_interface *intf) return 0; > > > > > > err: > > > + cancel_work_sync(&pdata->set_multicast); > > > kfree(pdata); > > > + pdata = NULL; > > > + dev->data[0] = 0; > > > return ret; > > > } > > > > > > > Hi, Dongliang! > > > > Just my thougth about this patch: > > > > INIT_WORK(&pdata->set_multicast, smsc75xx_deferred_multicast_write); > > does not queue anything, it just initalizes list structure and > > assigns callback function. The actual work sheduling happens in > > smsc75xx_set_multicast() which is smsc75xx_netdev_ops member. > > > > Yes, you are right. However, as written in the commit message, > smsc75xx_set_multicast will be called by smsc75xx_reset [1]. > > If smsc75xx_set_multicast is called before any check failure occurs, > this work(set_multicast) will be queued into the global list with > > schedule_work(&pdata->set_multicast); [2] Ah, I missed it, sorry :) Maybe, small optimization for error handling path like: cancel_work: cancel_work_sync(&pdata->set_multicast); dev->data[0] = 0; free_pdata: kfree(pdata); return ret; is suitbale here. > > At last, if the pdata or dev->data[0] is freed before the > set_multicast really executes, it may lead to a UAF. Is this correct? > > BTW, even if the above is true, I don't know if I call the API > ``cancel_work_sync(&pdata->set_multicast)'' properly if the > schedule_work is not called. > Yeah, it will be ok. > [1] > https://elixir.bootlin.com/linux/latest/source/drivers/net/usb/smsc75xx.c#L1322 > > [2] > https://elixir.bootlin.com/linux/latest/source/drivers/net/usb/smsc75xx.c#L583 > > > In case of any error in smsc75xx_bind() the device registration > > fails and smsc75xx_netdev_ops won't be registered, so, i guess, > > there is no chance of UAF. > > > > > > Am I missing something? :) > > > > > > > > With regards, > > Pavel Skripkin With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind 2021-06-15 13:31 ` Pavel Skripkin @ 2021-06-16 2:16 ` Dongliang Mu 0 siblings, 0 replies; 13+ messages in thread From: Dongliang Mu @ 2021-06-16 2:16 UTC (permalink / raw) To: Pavel Skripkin Cc: Steve Glendinning, David S. Miller, Jakub Kicinski, netdev, linux-usb, linux-kernel On Tue, Jun 15, 2021 at 9:31 PM Pavel Skripkin <paskripkin@gmail.com> wrote: > > On Tue, 15 Jun 2021 07:01:13 +0800 > Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > > On Tue, Jun 15, 2021 at 12:00 AM Pavel Skripkin > > <paskripkin@gmail.com> wrote: > > > > > > On Mon, 14 Jun 2021 23:37:12 +0800 > > > Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > > > > > > The commit 46a8b29c6306 ("net: usb: fix memory leak in > > > > smsc75xx_bind") fails to clean up the work scheduled in > > > > smsc75xx_reset-> smsc75xx_set_multicast, which leads to > > > > use-after-free if the work is scheduled to start after the > > > > deallocation. In addition, this patch also removes one dangling > > > > pointer - dev->data[0]. > > > > > > > > This patch calls cancel_work_sync to cancel the schedule work and > > > > set the dangling pointer to NULL. > > > > > > > > Fixes: 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > > > --- > > > > drivers/net/usb/smsc75xx.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/drivers/net/usb/smsc75xx.c > > > > b/drivers/net/usb/smsc75xx.c index b286993da67c..f81740fcc8d5 > > > > 100644 --- a/drivers/net/usb/smsc75xx.c > > > > +++ b/drivers/net/usb/smsc75xx.c > > > > @@ -1504,7 +1504,10 @@ static int smsc75xx_bind(struct usbnet > > > > *dev, struct usb_interface *intf) return 0; > > > > > > > > err: > > > > + cancel_work_sync(&pdata->set_multicast); > > > > kfree(pdata); > > > > + pdata = NULL; > > > > + dev->data[0] = 0; > > > > return ret; > > > > } > > > > > > > > > > Hi, Dongliang! > > > > > > Just my thougth about this patch: > > > > > > INIT_WORK(&pdata->set_multicast, smsc75xx_deferred_multicast_write); > > > does not queue anything, it just initalizes list structure and > > > assigns callback function. The actual work sheduling happens in > > > smsc75xx_set_multicast() which is smsc75xx_netdev_ops member. > > > > > > > Yes, you are right. However, as written in the commit message, > > smsc75xx_set_multicast will be called by smsc75xx_reset [1]. > > > > If smsc75xx_set_multicast is called before any check failure occurs, > > this work(set_multicast) will be queued into the global list with > > > > schedule_work(&pdata->set_multicast); [2] > > Ah, I missed it, sorry :) > > Maybe, small optimization for error handling path like: > > cancel_work: > cancel_work_sync(&pdata->set_multicast); > dev->data[0] = 0; > free_pdata: > kfree(pdata); > return ret; > > > is suitbale here. I agree with this style of error handling. However, I need to adjust the location of dev->data[0] = 0 after kfree(pdata) because if there still leaves a dangling pointer it directly goes to free_pdata. > > > > > At last, if the pdata or dev->data[0] is freed before the > > set_multicast really executes, it may lead to a UAF. Is this correct? > > > > BTW, even if the above is true, I don't know if I call the API > > ``cancel_work_sync(&pdata->set_multicast)'' properly if the > > schedule_work is not called. > > > > Yeah, it will be ok. Thanks for the confirmation. I've tested it under the previous kernel crash. It works fine. I will send a v2 patch quickly. > > > [1] > > https://elixir.bootlin.com/linux/latest/source/drivers/net/usb/smsc75xx.c#L1322 > > > > [2] > > https://elixir.bootlin.com/linux/latest/source/drivers/net/usb/smsc75xx.c#L583 > > > > > In case of any error in smsc75xx_bind() the device registration > > > fails and smsc75xx_netdev_ops won't be registered, so, i guess, > > > there is no chance of UAF. > > > > > > > > > Am I missing something? :) > > > > > > > > > > > > With regards, > > > Pavel Skripkin > > > > > With regards, > Pavel Skripkin ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind 2021-06-14 15:37 [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind Dongliang Mu 2021-06-14 16:00 ` Pavel Skripkin @ 2021-06-15 7:38 ` Greg KH 2021-06-15 7:56 ` Dongliang Mu 1 sibling, 1 reply; 13+ messages in thread From: Greg KH @ 2021-06-15 7:38 UTC (permalink / raw) To: Dongliang Mu Cc: steve.glendinning, davem, kuba, paskripkin, netdev, linux-usb, linux-kernel On Mon, Jun 14, 2021 at 11:37:12PM +0800, Dongliang Mu wrote: > The commit 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > fails to clean up the work scheduled in smsc75xx_reset-> > smsc75xx_set_multicast, which leads to use-after-free if the work is > scheduled to start after the deallocation. In addition, this patch also > removes one dangling pointer - dev->data[0]. > > This patch calls cancel_work_sync to cancel the schedule work and set > the dangling pointer to NULL. > > Fixes: 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > --- > drivers/net/usb/smsc75xx.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c > index b286993da67c..f81740fcc8d5 100644 > --- a/drivers/net/usb/smsc75xx.c > +++ b/drivers/net/usb/smsc75xx.c > @@ -1504,7 +1504,10 @@ static int smsc75xx_bind(struct usbnet *dev, struct usb_interface *intf) > return 0; > > err: > + cancel_work_sync(&pdata->set_multicast); > kfree(pdata); > + pdata = NULL; Why do you have to set pdata to NULL afterward? thanks, greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind 2021-06-15 7:38 ` Greg KH @ 2021-06-15 7:56 ` Dongliang Mu 2021-06-15 9:44 ` Greg KH 0 siblings, 1 reply; 13+ messages in thread From: Dongliang Mu @ 2021-06-15 7:56 UTC (permalink / raw) To: Greg KH Cc: Steve Glendinning, David S. Miller, Jakub Kicinski, Pavel Skripkin, netdev, linux-usb, linux-kernel On Tue, Jun 15, 2021 at 3:38 PM Greg KH <greg@kroah.com> wrote: > > On Mon, Jun 14, 2021 at 11:37:12PM +0800, Dongliang Mu wrote: > > The commit 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > > fails to clean up the work scheduled in smsc75xx_reset-> > > smsc75xx_set_multicast, which leads to use-after-free if the work is > > scheduled to start after the deallocation. In addition, this patch also > > removes one dangling pointer - dev->data[0]. > > > > This patch calls cancel_work_sync to cancel the schedule work and set > > the dangling pointer to NULL. > > > > Fixes: 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > --- > > drivers/net/usb/smsc75xx.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c > > index b286993da67c..f81740fcc8d5 100644 > > --- a/drivers/net/usb/smsc75xx.c > > +++ b/drivers/net/usb/smsc75xx.c > > @@ -1504,7 +1504,10 @@ static int smsc75xx_bind(struct usbnet *dev, struct usb_interface *intf) > > return 0; > > > > err: > > + cancel_work_sync(&pdata->set_multicast); > > kfree(pdata); > > + pdata = NULL; > > Why do you have to set pdata to NULL afterward? > It does not have to. pdata will be useless when the function exits. I just referred to the implementation of smsc75xx_unbind. > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind 2021-06-15 7:56 ` Dongliang Mu @ 2021-06-15 9:44 ` Greg KH 2021-06-15 10:10 ` Dongliang Mu 0 siblings, 1 reply; 13+ messages in thread From: Greg KH @ 2021-06-15 9:44 UTC (permalink / raw) To: Dongliang Mu Cc: Steve Glendinning, David S. Miller, Jakub Kicinski, Pavel Skripkin, netdev, linux-usb, linux-kernel On Tue, Jun 15, 2021 at 03:56:32PM +0800, Dongliang Mu wrote: > On Tue, Jun 15, 2021 at 3:38 PM Greg KH <greg@kroah.com> wrote: > > > > On Mon, Jun 14, 2021 at 11:37:12PM +0800, Dongliang Mu wrote: > > > The commit 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > > > fails to clean up the work scheduled in smsc75xx_reset-> > > > smsc75xx_set_multicast, which leads to use-after-free if the work is > > > scheduled to start after the deallocation. In addition, this patch also > > > removes one dangling pointer - dev->data[0]. > > > > > > This patch calls cancel_work_sync to cancel the schedule work and set > > > the dangling pointer to NULL. > > > > > > Fixes: 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > > --- > > > drivers/net/usb/smsc75xx.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c > > > index b286993da67c..f81740fcc8d5 100644 > > > --- a/drivers/net/usb/smsc75xx.c > > > +++ b/drivers/net/usb/smsc75xx.c > > > @@ -1504,7 +1504,10 @@ static int smsc75xx_bind(struct usbnet *dev, struct usb_interface *intf) > > > return 0; > > > > > > err: > > > + cancel_work_sync(&pdata->set_multicast); > > > kfree(pdata); > > > + pdata = NULL; > > > > Why do you have to set pdata to NULL afterward? > > > > It does not have to. pdata will be useless when the function exits. I > just referred to the implementation of smsc75xx_unbind. It's wrong there too :) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind 2021-06-15 9:44 ` Greg KH @ 2021-06-15 10:10 ` Dongliang Mu 2021-06-15 10:24 ` Dongliang Mu 0 siblings, 1 reply; 13+ messages in thread From: Dongliang Mu @ 2021-06-15 10:10 UTC (permalink / raw) To: Greg KH Cc: Steve Glendinning, David S. Miller, Jakub Kicinski, Pavel Skripkin, netdev, linux-usb, linux-kernel On Tue, Jun 15, 2021 at 5:44 PM Greg KH <greg@kroah.com> wrote: > > On Tue, Jun 15, 2021 at 03:56:32PM +0800, Dongliang Mu wrote: > > On Tue, Jun 15, 2021 at 3:38 PM Greg KH <greg@kroah.com> wrote: > > > > > > On Mon, Jun 14, 2021 at 11:37:12PM +0800, Dongliang Mu wrote: > > > > The commit 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > > > > fails to clean up the work scheduled in smsc75xx_reset-> > > > > smsc75xx_set_multicast, which leads to use-after-free if the work is > > > > scheduled to start after the deallocation. In addition, this patch also > > > > removes one dangling pointer - dev->data[0]. > > > > > > > > This patch calls cancel_work_sync to cancel the schedule work and set > > > > the dangling pointer to NULL. > > > > > > > > Fixes: 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > > > --- > > > > drivers/net/usb/smsc75xx.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c > > > > index b286993da67c..f81740fcc8d5 100644 > > > > --- a/drivers/net/usb/smsc75xx.c > > > > +++ b/drivers/net/usb/smsc75xx.c > > > > @@ -1504,7 +1504,10 @@ static int smsc75xx_bind(struct usbnet *dev, struct usb_interface *intf) > > > > return 0; > > > > > > > > err: > > > > + cancel_work_sync(&pdata->set_multicast); > > > > kfree(pdata); > > > > + pdata = NULL; > > > > > > Why do you have to set pdata to NULL afterward? > > > > > > > It does not have to. pdata will be useless when the function exits. I > > just referred to the implementation of smsc75xx_unbind. > > It's wrong there too :) /: I will fix such two sites in the v2 patch. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind 2021-06-15 10:10 ` Dongliang Mu @ 2021-06-15 10:24 ` Dongliang Mu 2021-06-15 11:12 ` Greg KH 0 siblings, 1 reply; 13+ messages in thread From: Dongliang Mu @ 2021-06-15 10:24 UTC (permalink / raw) To: Greg KH Cc: Steve Glendinning, David S. Miller, Jakub Kicinski, Pavel Skripkin, netdev, linux-usb, linux-kernel On Tue, Jun 15, 2021 at 6:10 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > On Tue, Jun 15, 2021 at 5:44 PM Greg KH <greg@kroah.com> wrote: > > > > On Tue, Jun 15, 2021 at 03:56:32PM +0800, Dongliang Mu wrote: > > > On Tue, Jun 15, 2021 at 3:38 PM Greg KH <greg@kroah.com> wrote: > > > > > > > > On Mon, Jun 14, 2021 at 11:37:12PM +0800, Dongliang Mu wrote: > > > > > The commit 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > > > > > fails to clean up the work scheduled in smsc75xx_reset-> > > > > > smsc75xx_set_multicast, which leads to use-after-free if the work is > > > > > scheduled to start after the deallocation. In addition, this patch also > > > > > removes one dangling pointer - dev->data[0]. > > > > > > > > > > This patch calls cancel_work_sync to cancel the schedule work and set > > > > > the dangling pointer to NULL. > > > > > > > > > > Fixes: 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > > > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > > > > --- > > > > > drivers/net/usb/smsc75xx.c | 3 +++ > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c > > > > > index b286993da67c..f81740fcc8d5 100644 > > > > > --- a/drivers/net/usb/smsc75xx.c > > > > > +++ b/drivers/net/usb/smsc75xx.c > > > > > @@ -1504,7 +1504,10 @@ static int smsc75xx_bind(struct usbnet *dev, struct usb_interface *intf) > > > > > return 0; > > > > > > > > > > err: > > > > > + cancel_work_sync(&pdata->set_multicast); > > > > > kfree(pdata); > > > > > + pdata = NULL; > > > > > > > > Why do you have to set pdata to NULL afterward? > > > > > > > > > > It does not have to. pdata will be useless when the function exits. I > > > just referred to the implementation of smsc75xx_unbind. > > > > It's wrong there too :) > > /: I will fix such two sites in the v2 patch. Hi gregkh, If the schedule_work is not invoked, can I call ``cancel_work_sync(&pdata->set_multicast)''? If not, is there any method to verify if the schedule_work is already called? Best regards, Dongliang Mu ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind 2021-06-15 10:24 ` Dongliang Mu @ 2021-06-15 11:12 ` Greg KH 2021-06-15 12:07 ` Dongliang Mu 0 siblings, 1 reply; 13+ messages in thread From: Greg KH @ 2021-06-15 11:12 UTC (permalink / raw) To: Dongliang Mu Cc: Steve Glendinning, David S. Miller, Jakub Kicinski, Pavel Skripkin, netdev, linux-usb, linux-kernel On Tue, Jun 15, 2021 at 06:24:17PM +0800, Dongliang Mu wrote: > On Tue, Jun 15, 2021 at 6:10 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > > > On Tue, Jun 15, 2021 at 5:44 PM Greg KH <greg@kroah.com> wrote: > > > > > > On Tue, Jun 15, 2021 at 03:56:32PM +0800, Dongliang Mu wrote: > > > > On Tue, Jun 15, 2021 at 3:38 PM Greg KH <greg@kroah.com> wrote: > > > > > > > > > > On Mon, Jun 14, 2021 at 11:37:12PM +0800, Dongliang Mu wrote: > > > > > > The commit 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > > > > > > fails to clean up the work scheduled in smsc75xx_reset-> > > > > > > smsc75xx_set_multicast, which leads to use-after-free if the work is > > > > > > scheduled to start after the deallocation. In addition, this patch also > > > > > > removes one dangling pointer - dev->data[0]. > > > > > > > > > > > > This patch calls cancel_work_sync to cancel the schedule work and set > > > > > > the dangling pointer to NULL. > > > > > > > > > > > > Fixes: 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > > > > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > > > > > --- > > > > > > drivers/net/usb/smsc75xx.c | 3 +++ > > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > > > diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c > > > > > > index b286993da67c..f81740fcc8d5 100644 > > > > > > --- a/drivers/net/usb/smsc75xx.c > > > > > > +++ b/drivers/net/usb/smsc75xx.c > > > > > > @@ -1504,7 +1504,10 @@ static int smsc75xx_bind(struct usbnet *dev, struct usb_interface *intf) > > > > > > return 0; > > > > > > > > > > > > err: > > > > > > + cancel_work_sync(&pdata->set_multicast); > > > > > > kfree(pdata); > > > > > > + pdata = NULL; > > > > > > > > > > Why do you have to set pdata to NULL afterward? > > > > > > > > > > > > > It does not have to. pdata will be useless when the function exits. I > > > > just referred to the implementation of smsc75xx_unbind. > > > > > > It's wrong there too :) > > > > /: I will fix such two sites in the v2 patch. > > Hi gregkh, > > If the schedule_work is not invoked, can I call > ``cancel_work_sync(&pdata->set_multicast)''? Why can you not call this then? Did you try it and see? thanks, greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind 2021-06-15 11:12 ` Greg KH @ 2021-06-15 12:07 ` Dongliang Mu 2021-06-15 13:03 ` Greg KH 0 siblings, 1 reply; 13+ messages in thread From: Dongliang Mu @ 2021-06-15 12:07 UTC (permalink / raw) To: Greg KH Cc: Steve Glendinning, David S. Miller, Jakub Kicinski, Pavel Skripkin, netdev, linux-usb, linux-kernel On Tue, Jun 15, 2021 at 7:12 PM Greg KH <greg@kroah.com> wrote: > > On Tue, Jun 15, 2021 at 06:24:17PM +0800, Dongliang Mu wrote: > > On Tue, Jun 15, 2021 at 6:10 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > > > > > On Tue, Jun 15, 2021 at 5:44 PM Greg KH <greg@kroah.com> wrote: > > > > > > > > On Tue, Jun 15, 2021 at 03:56:32PM +0800, Dongliang Mu wrote: > > > > > On Tue, Jun 15, 2021 at 3:38 PM Greg KH <greg@kroah.com> wrote: > > > > > > > > > > > > On Mon, Jun 14, 2021 at 11:37:12PM +0800, Dongliang Mu wrote: > > > > > > > The commit 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > > > > > > > fails to clean up the work scheduled in smsc75xx_reset-> > > > > > > > smsc75xx_set_multicast, which leads to use-after-free if the work is > > > > > > > scheduled to start after the deallocation. In addition, this patch also > > > > > > > removes one dangling pointer - dev->data[0]. > > > > > > > > > > > > > > This patch calls cancel_work_sync to cancel the schedule work and set > > > > > > > the dangling pointer to NULL. > > > > > > > > > > > > > > Fixes: 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > > > > > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > > > > > > --- > > > > > > > drivers/net/usb/smsc75xx.c | 3 +++ > > > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c > > > > > > > index b286993da67c..f81740fcc8d5 100644 > > > > > > > --- a/drivers/net/usb/smsc75xx.c > > > > > > > +++ b/drivers/net/usb/smsc75xx.c > > > > > > > @@ -1504,7 +1504,10 @@ static int smsc75xx_bind(struct usbnet *dev, struct usb_interface *intf) > > > > > > > return 0; > > > > > > > > > > > > > > err: > > > > > > > + cancel_work_sync(&pdata->set_multicast); > > > > > > > kfree(pdata); > > > > > > > + pdata = NULL; > > > > > > > > > > > > Why do you have to set pdata to NULL afterward? > > > > > > > > > > > > > > > > It does not have to. pdata will be useless when the function exits. I > > > > > just referred to the implementation of smsc75xx_unbind. > > > > > > > > It's wrong there too :) > > > > > > /: I will fix such two sites in the v2 patch. > > > > Hi gregkh, > > > > If the schedule_work is not invoked, can I call > > ``cancel_work_sync(&pdata->set_multicast)''? > > Why can you not call this then? I don't know the internal of schedule_work and cancel_work_sync, so I ask this question to confirm my patch does not introduce any new issues. > > Did you try it and see? Yes, I thought up a method and tested it in my local workspace. First, I reproduced the memory leak in smsc75xx_bind [1] since the PoC triggered an error before schedule_work. Then, I merged two patches, and run the PoC. The result showed that my patch does not trigger any new issues even the schedule_work is not called. [1] https://syzkaller.appspot.com/bug?id=c978ec308a1b89089a17ff48183d70b4c840dfb0 > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind 2021-06-15 12:07 ` Dongliang Mu @ 2021-06-15 13:03 ` Greg KH 0 siblings, 0 replies; 13+ messages in thread From: Greg KH @ 2021-06-15 13:03 UTC (permalink / raw) To: Dongliang Mu Cc: Steve Glendinning, David S. Miller, Jakub Kicinski, Pavel Skripkin, netdev, linux-usb, linux-kernel On Tue, Jun 15, 2021 at 08:07:10PM +0800, Dongliang Mu wrote: > On Tue, Jun 15, 2021 at 7:12 PM Greg KH <greg@kroah.com> wrote: > > > > On Tue, Jun 15, 2021 at 06:24:17PM +0800, Dongliang Mu wrote: > > > On Tue, Jun 15, 2021 at 6:10 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > > > > > > > On Tue, Jun 15, 2021 at 5:44 PM Greg KH <greg@kroah.com> wrote: > > > > > > > > > > On Tue, Jun 15, 2021 at 03:56:32PM +0800, Dongliang Mu wrote: > > > > > > On Tue, Jun 15, 2021 at 3:38 PM Greg KH <greg@kroah.com> wrote: > > > > > > > > > > > > > > On Mon, Jun 14, 2021 at 11:37:12PM +0800, Dongliang Mu wrote: > > > > > > > > The commit 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > > > > > > > > fails to clean up the work scheduled in smsc75xx_reset-> > > > > > > > > smsc75xx_set_multicast, which leads to use-after-free if the work is > > > > > > > > scheduled to start after the deallocation. In addition, this patch also > > > > > > > > removes one dangling pointer - dev->data[0]. > > > > > > > > > > > > > > > > This patch calls cancel_work_sync to cancel the schedule work and set > > > > > > > > the dangling pointer to NULL. > > > > > > > > > > > > > > > > Fixes: 46a8b29c6306 ("net: usb: fix memory leak in smsc75xx_bind") > > > > > > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > > > > > > > --- > > > > > > > > drivers/net/usb/smsc75xx.c | 3 +++ > > > > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > > > > > > > diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c > > > > > > > > index b286993da67c..f81740fcc8d5 100644 > > > > > > > > --- a/drivers/net/usb/smsc75xx.c > > > > > > > > +++ b/drivers/net/usb/smsc75xx.c > > > > > > > > @@ -1504,7 +1504,10 @@ static int smsc75xx_bind(struct usbnet *dev, struct usb_interface *intf) > > > > > > > > return 0; > > > > > > > > > > > > > > > > err: > > > > > > > > + cancel_work_sync(&pdata->set_multicast); > > > > > > > > kfree(pdata); > > > > > > > > + pdata = NULL; > > > > > > > > > > > > > > Why do you have to set pdata to NULL afterward? > > > > > > > > > > > > > > > > > > > It does not have to. pdata will be useless when the function exits. I > > > > > > just referred to the implementation of smsc75xx_unbind. > > > > > > > > > > It's wrong there too :) > > > > > > > > /: I will fix such two sites in the v2 patch. > > > > > > Hi gregkh, > > > > > > If the schedule_work is not invoked, can I call > > > ``cancel_work_sync(&pdata->set_multicast)''? > > > > Why can you not call this then? > > I don't know the internal of schedule_work and cancel_work_sync, so I > ask this question to confirm my patch does not introduce any new > issues. Please see the documentation for this function for all of the details. It is in kernel/workqueue.c ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-06-16 2:17 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-14 15:37 [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind Dongliang Mu 2021-06-14 16:00 ` Pavel Skripkin 2021-06-14 23:01 ` Dongliang Mu 2021-06-15 13:31 ` Pavel Skripkin 2021-06-16 2:16 ` Dongliang Mu 2021-06-15 7:38 ` Greg KH 2021-06-15 7:56 ` Dongliang Mu 2021-06-15 9:44 ` Greg KH 2021-06-15 10:10 ` Dongliang Mu 2021-06-15 10:24 ` Dongliang Mu 2021-06-15 11:12 ` Greg KH 2021-06-15 12:07 ` Dongliang Mu 2021-06-15 13:03 ` Greg KH
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.