[PATCH] net: usb: fix possible use-after-free in smsc75xx_bind

[PATCH] net: usb: fix possible use-after-free in smsc75xx_bind

[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

Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind

[Pavel Skripkin]

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

Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind

[Dongliang Mu]

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

Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind

[Greg KH]

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

Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind

[Dongliang Mu]

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

Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind

[Greg KH]

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 :)

Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind

[Dongliang Mu]

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.

Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind

[Dongliang Mu]

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

Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind

[Greg KH]

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

Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind

[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

Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind

[Greg KH]

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

Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind

[Pavel Skripkin]

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

Re: [PATCH] net: usb: fix possible use-after-free in smsc75xx_bind

[Dongliang Mu]

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