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