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 <[email protected]>
---
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
On Mon, 14 Jun 2021 23:37:12 +0800
Dongliang Mu <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]> wrote:
>
> On Mon, 14 Jun 2021 23:37:12 +0800
> Dongliang Mu <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]> wrote:
>
> On Tue, Jun 15, 2021 at 5:44 PM Greg KH <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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 <[email protected]> wrote:
> >
> > On Tue, Jun 15, 2021 at 5:44 PM Greg KH <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > ---
> > > > > > 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 <[email protected]> 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 <[email protected]> wrote:
> > >
> > > On Tue, Jun 15, 2021 at 5:44 PM Greg KH <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > > ---
> > > > > > > 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 <[email protected]> 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 <[email protected]> wrote:
> > > >
> > > > On Tue, Jun 15, 2021 at 5:44 PM Greg KH <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > > > ---
> > > > > > > > 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 <[email protected]> wrote:
> On Tue, Jun 15, 2021 at 12:00 AM Pavel Skripkin
> <[email protected]> wrote:
> >
> > On Mon, 14 Jun 2021 23:37:12 +0800
> > Dongliang Mu <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 <[email protected]> wrote:
>
> On Tue, 15 Jun 2021 07:01:13 +0800
> Dongliang Mu <[email protected]> wrote:
>
> > On Tue, Jun 15, 2021 at 12:00 AM Pavel Skripkin
> > <[email protected]> wrote:
> > >
> > > On Mon, 14 Jun 2021 23:37:12 +0800
> > > Dongliang Mu <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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