| Message ID | 20210614153712.2172662-1-mudongliangabcd@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: usb: fix possible use-after-free in smsc75xx_bind | expand |
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
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
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
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
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; }
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(+)