| 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 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 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, 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
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(+)