* [PATCH net] tuntap: correctly handle error in tun_set_iff()
@ 2013-09-11 8:24 Jason Wang
2013-09-11 9:38 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Jason Wang @ 2013-09-11 8:24 UTC (permalink / raw)
To: davem, netdev, wannes.rombouts, linux-kernel
Cc: Jason Wang, Michael S. Tsirkin
Commit c8d68e6be1c3b242f1c598595830890b65cea64a (tuntap: multiqueue support)
only call free_netdev() on err in tun_set_iff(). This causes several issues:
- memory of tun security were leaked
- use after free since the flow gc timer was not deleted and the tfile were not
detached
This patch solves the above issues.
Reported-by: Wannes Rombouts <wannes.rombouts@epitech.eu>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
The patch were also needed for stable 3.8+.
---
drivers/net/tun.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index a639de8..e5fb5d3 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1641,11 +1641,11 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
INIT_LIST_HEAD(&tun->disabled);
err = tun_attach(tun, file, false);
if (err < 0)
- goto err_free_dev;
+ goto err_free_flow;
err = register_netdevice(tun->dev);
if (err < 0)
- goto err_free_dev;
+ goto err_detach;
if (device_create_file(&tun->dev->dev, &dev_attr_tun_flags) ||
device_create_file(&tun->dev->dev, &dev_attr_owner) ||
@@ -1689,6 +1689,11 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
strcpy(ifr->ifr_name, tun->dev->name);
return 0;
+err_detach:
+ tun_detach_all(dev);
+err_free_flow:
+ tun_flow_uninit(tun);
+ security_tun_dev_free_security(tun->security);
err_free_dev:
free_netdev(dev);
return err;
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] tuntap: correctly handle error in tun_set_iff()
2013-09-11 8:24 [PATCH net] tuntap: correctly handle error in tun_set_iff() Jason Wang
@ 2013-09-11 9:38 ` Michael S. Tsirkin
2013-09-11 9:55 ` Jason Wang
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2013-09-11 9:38 UTC (permalink / raw)
To: Jason Wang; +Cc: davem, netdev, wannes.rombouts, linux-kernel
On Wed, Sep 11, 2013 at 04:24:05PM +0800, Jason Wang wrote:
> Commit c8d68e6be1c3b242f1c598595830890b65cea64a (tuntap: multiqueue support)
> only call free_netdev() on err in tun_set_iff(). This causes several issues:
>
> - memory of tun security were leaked
Not just tun security - sock reference too (didn't detach)
> - use after free since the flow gc timer was not deleted and the tfile were not
> detached
>
> This patch solves the above issues.
>
> Reported-by: Wannes Rombouts <wannes.rombouts@epitech.eu>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> The patch were also needed for stable 3.8+.
> ---
> drivers/net/tun.c | 9 +++++++--
> 1 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index a639de8..e5fb5d3 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1641,11 +1641,11 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
> INIT_LIST_HEAD(&tun->disabled);
> err = tun_attach(tun, file, false);
> if (err < 0)
> - goto err_free_dev;
> + goto err_free_flow;
>
> err = register_netdevice(tun->dev);
> if (err < 0)
> - goto err_free_dev;
> + goto err_detach;
>
> if (device_create_file(&tun->dev->dev, &dev_attr_tun_flags) ||
> device_create_file(&tun->dev->dev, &dev_attr_owner) ||
> @@ -1689,6 +1689,11 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
> strcpy(ifr->ifr_name, tun->dev->name);
> return 0;
>
> +err_detach:
> + tun_detach_all(dev);
> +err_free_flow:
> + tun_flow_uninit(tun);
> + security_tun_dev_free_security(tun->security);
This bit looks wrong: if flow_init fails, we
need security_tun_dev_free_security, don't we?
I think we need:
+err_free_sec: security_tun_dev_free_security(tun->security);
and goto here on flow_init failures.
> err_free_dev:
Pls shift this one 1 space left for consistency.
> free_netdev(dev);
> return err;
> --
> 1.7.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] tuntap: correctly handle error in tun_set_iff()
2013-09-11 9:38 ` Michael S. Tsirkin
@ 2013-09-11 9:55 ` Jason Wang
2013-09-11 10:08 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Jason Wang @ 2013-09-11 9:55 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: davem, netdev, wannes rombouts, linux-kernel
----- Original Message -----
> On Wed, Sep 11, 2013 at 04:24:05PM +0800, Jason Wang wrote:
> > Commit c8d68e6be1c3b242f1c598595830890b65cea64a (tuntap: multiqueue
> > support)
> > only call free_netdev() on err in tun_set_iff(). This causes several
> > issues:
> >
> > - memory of tun security were leaked
>
> Not just tun security - sock reference too (didn't detach)
>
Yes, I mention it in the next item.
> > - use after free since the flow gc timer was not deleted and the tfile were
> > not
> > detached
> >
> > This patch solves the above issues.
> >
> > Reported-by: Wannes Rombouts <wannes.rombouts@epitech.eu>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> > The patch were also needed for stable 3.8+.
> > ---
> > drivers/net/tun.c | 9 +++++++--
> > 1 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index a639de8..e5fb5d3 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -1641,11 +1641,11 @@ static int tun_set_iff(struct net *net, struct file
> > *file, struct ifreq *ifr)
> > INIT_LIST_HEAD(&tun->disabled);
> > err = tun_attach(tun, file, false);
> > if (err < 0)
> > - goto err_free_dev;
> > + goto err_free_flow;
> >
> > err = register_netdevice(tun->dev);
> > if (err < 0)
> > - goto err_free_dev;
> > + goto err_detach;
> >
> > if (device_create_file(&tun->dev->dev, &dev_attr_tun_flags) ||
> > device_create_file(&tun->dev->dev, &dev_attr_owner) ||
> > @@ -1689,6 +1689,11 @@ static int tun_set_iff(struct net *net, struct file
> > *file, struct ifreq *ifr)
> > strcpy(ifr->ifr_name, tun->dev->name);
> > return 0;
> >
> > +err_detach:
> > + tun_detach_all(dev);
> > +err_free_flow:
> > + tun_flow_uninit(tun);
> > + security_tun_dev_free_security(tun->security);
>
> This bit looks wrong: if flow_init fails, we
> need security_tun_dev_free_security, don't we?
> I think we need:
> +err_free_sec: security_tun_dev_free_security(tun->security);
> and goto here on flow_init failures.
tun_flow_init() always succeed. So we're ok here.
>
> > err_free_dev:
>
> Pls shift this one 1 space left for consistency.
>
Ok.
> > free_netdev(dev);
> > return err;
> > --
> > 1.7.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] tuntap: correctly handle error in tun_set_iff()
2013-09-11 9:55 ` Jason Wang
@ 2013-09-11 10:08 ` Michael S. Tsirkin
2013-09-11 10:18 ` Jason Wang
2013-09-11 10:58 ` Michael S. Tsirkin
0 siblings, 2 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2013-09-11 10:08 UTC (permalink / raw)
To: Jason Wang; +Cc: davem, netdev, wannes rombouts, linux-kernel
On Wed, Sep 11, 2013 at 05:55:04AM -0400, Jason Wang wrote:
>
>
> ----- Original Message -----
> > On Wed, Sep 11, 2013 at 04:24:05PM +0800, Jason Wang wrote:
> > > Commit c8d68e6be1c3b242f1c598595830890b65cea64a (tuntap: multiqueue
> > > support)
> > > only call free_netdev() on err in tun_set_iff(). This causes several
> > > issues:
> > >
> > > - memory of tun security were leaked
> >
> > Not just tun security - sock reference too (didn't detach)
> >
>
> Yes, I mention it in the next item.
That line's a bit too long btw.
Keep it under 70 chars for commit logs.
> > > - use after free since the flow gc timer was not deleted and the tfile were
> > > not
> > > detached
> > >
> > > This patch solves the above issues.
> > >
> > > Reported-by: Wannes Rombouts <wannes.rombouts@epitech.eu>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > > The patch were also needed for stable 3.8+.
> > > ---
> > > drivers/net/tun.c | 9 +++++++--
> > > 1 files changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > > index a639de8..e5fb5d3 100644
> > > --- a/drivers/net/tun.c
> > > +++ b/drivers/net/tun.c
> > > @@ -1641,11 +1641,11 @@ static int tun_set_iff(struct net *net, struct file
> > > *file, struct ifreq *ifr)
> > > INIT_LIST_HEAD(&tun->disabled);
> > > err = tun_attach(tun, file, false);
> > > if (err < 0)
> > > - goto err_free_dev;
> > > + goto err_free_flow;
> > >
> > > err = register_netdevice(tun->dev);
> > > if (err < 0)
> > > - goto err_free_dev;
> > > + goto err_detach;
> > >
> > > if (device_create_file(&tun->dev->dev, &dev_attr_tun_flags) ||
> > > device_create_file(&tun->dev->dev, &dev_attr_owner) ||
> > > @@ -1689,6 +1689,11 @@ static int tun_set_iff(struct net *net, struct file
> > > *file, struct ifreq *ifr)
> > > strcpy(ifr->ifr_name, tun->dev->name);
> > > return 0;
> > >
> > > +err_detach:
> > > + tun_detach_all(dev);
> > > +err_free_flow:
> > > + tun_flow_uninit(tun);
> > > + security_tun_dev_free_security(tun->security);
> >
> > This bit looks wrong: if flow_init fails, we
> > need security_tun_dev_free_security, don't we?
> > I think we need:
> > +err_free_sec: security_tun_dev_free_security(tun->security);
> > and goto here on flow_init failures.
>
> tun_flow_init() always succeed. So we're ok here.
True. So please make it return void then.
> >
> > > err_free_dev:
> >
> > Pls shift this one 1 space left for consistency.
> >
>
> Ok.
> > > free_netdev(dev);
> > > return err;
> > > --
> > > 1.7.1
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] tuntap: correctly handle error in tun_set_iff()
2013-09-11 10:08 ` Michael S. Tsirkin
@ 2013-09-11 10:18 ` Jason Wang
2013-09-11 10:58 ` Michael S. Tsirkin
1 sibling, 0 replies; 6+ messages in thread
From: Jason Wang @ 2013-09-11 10:18 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: davem, netdev, wannes rombouts, linux-kernel
----- Original Message -----
> On Wed, Sep 11, 2013 at 05:55:04AM -0400, Jason Wang wrote:
> >
> >
> > ----- Original Message -----
> > > On Wed, Sep 11, 2013 at 04:24:05PM +0800, Jason Wang wrote:
> > > > Commit c8d68e6be1c3b242f1c598595830890b65cea64a (tuntap: multiqueue
> > > > support)
> > > > only call free_netdev() on err in tun_set_iff(). This causes several
> > > > issues:
> > > >
> > > > - memory of tun security were leaked
> > >
> > > Not just tun security - sock reference too (didn't detach)
> > >
> >
> > Yes, I mention it in the next item.
>
> That line's a bit too long btw.
> Keep it under 70 chars for commit logs.
>
Ok.
> > > > - use after free since the flow gc timer was not deleted and the tfile
> > > > were
> > > > not
> > > > detached
> > > >
> > > > This patch solves the above issues.
> > > >
> > > > Reported-by: Wannes Rombouts <wannes.rombouts@epitech.eu>
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > ---
> > > > The patch were also needed for stable 3.8+.
> > > > ---
> > > > drivers/net/tun.c | 9 +++++++--
> > > > 1 files changed, 7 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > > > index a639de8..e5fb5d3 100644
> > > > --- a/drivers/net/tun.c
> > > > +++ b/drivers/net/tun.c
> > > > @@ -1641,11 +1641,11 @@ static int tun_set_iff(struct net *net, struct
> > > > file
> > > > *file, struct ifreq *ifr)
> > > > INIT_LIST_HEAD(&tun->disabled);
> > > > err = tun_attach(tun, file, false);
> > > > if (err < 0)
> > > > - goto err_free_dev;
> > > > + goto err_free_flow;
> > > >
> > > > err = register_netdevice(tun->dev);
> > > > if (err < 0)
> > > > - goto err_free_dev;
> > > > + goto err_detach;
> > > >
> > > > if (device_create_file(&tun->dev->dev, &dev_attr_tun_flags) ||
> > > > device_create_file(&tun->dev->dev, &dev_attr_owner) ||
> > > > @@ -1689,6 +1689,11 @@ static int tun_set_iff(struct net *net, struct
> > > > file
> > > > *file, struct ifreq *ifr)
> > > > strcpy(ifr->ifr_name, tun->dev->name);
> > > > return 0;
> > > >
> > > > +err_detach:
> > > > + tun_detach_all(dev);
> > > > +err_free_flow:
> > > > + tun_flow_uninit(tun);
> > > > + security_tun_dev_free_security(tun->security);
> > >
> > > This bit looks wrong: if flow_init fails, we
> > > need security_tun_dev_free_security, don't we?
> > > I think we need:
> > > +err_free_sec: security_tun_dev_free_security(tun->security);
> > > and goto here on flow_init failures.
> >
> > tun_flow_init() always succeed. So we're ok here.
>
> True. So please make it return void then.
>
It has already returned void.
> > >
> > > > err_free_dev:
> > >
> > > Pls shift this one 1 space left for consistency.
> > >
> >
> > Ok.
> > > > free_netdev(dev);
> > > > return err;
> > > > --
> > > > 1.7.1
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-kernel"
> > > in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at http://www.tux.org/lkml/
> > >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] tuntap: correctly handle error in tun_set_iff()
2013-09-11 10:08 ` Michael S. Tsirkin
2013-09-11 10:18 ` Jason Wang
@ 2013-09-11 10:58 ` Michael S. Tsirkin
1 sibling, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2013-09-11 10:58 UTC (permalink / raw)
To: Jason Wang; +Cc: davem, netdev, wannes rombouts, linux-kernel
On Wed, Sep 11, 2013 at 01:08:04PM +0300, Michael S. Tsirkin wrote:
> On Wed, Sep 11, 2013 at 05:55:04AM -0400, Jason Wang wrote:
> >
> >
> > ----- Original Message -----
> > > On Wed, Sep 11, 2013 at 04:24:05PM +0800, Jason Wang wrote:
> > > > Commit c8d68e6be1c3b242f1c598595830890b65cea64a (tuntap: multiqueue
> > > > support)
> > > > only call free_netdev() on err in tun_set_iff(). This causes several
> > > > issues:
> > > >
> > > > - memory of tun security were leaked
> > >
> > > Not just tun security - sock reference too (didn't detach)
> > >
> >
> > Yes, I mention it in the next item.
>
> That line's a bit too long btw.
> Keep it under 70 chars for commit logs.
>
> > > > - use after free since the flow gc timer was not deleted and the tfile were
> > > > not
> > > > detached
> > > >
> > > > This patch solves the above issues.
> > > >
> > > > Reported-by: Wannes Rombouts <wannes.rombouts@epitech.eu>
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > ---
> > > > The patch were also needed for stable 3.8+.
> > > > ---
> > > > drivers/net/tun.c | 9 +++++++--
> > > > 1 files changed, 7 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > > > index a639de8..e5fb5d3 100644
> > > > --- a/drivers/net/tun.c
> > > > +++ b/drivers/net/tun.c
> > > > @@ -1641,11 +1641,11 @@ static int tun_set_iff(struct net *net, struct file
> > > > *file, struct ifreq *ifr)
> > > > INIT_LIST_HEAD(&tun->disabled);
> > > > err = tun_attach(tun, file, false);
> > > > if (err < 0)
> > > > - goto err_free_dev;
> > > > + goto err_free_flow;
> > > >
> > > > err = register_netdevice(tun->dev);
> > > > if (err < 0)
> > > > - goto err_free_dev;
> > > > + goto err_detach;
> > > >
> > > > if (device_create_file(&tun->dev->dev, &dev_attr_tun_flags) ||
> > > > device_create_file(&tun->dev->dev, &dev_attr_owner) ||
> > > > @@ -1689,6 +1689,11 @@ static int tun_set_iff(struct net *net, struct file
> > > > *file, struct ifreq *ifr)
> > > > strcpy(ifr->ifr_name, tun->dev->name);
> > > > return 0;
> > > >
> > > > +err_detach:
> > > > + tun_detach_all(dev);
> > > > +err_free_flow:
> > > > + tun_flow_uninit(tun);
> > > > + security_tun_dev_free_security(tun->security);
> > >
> > > This bit looks wrong: if flow_init fails, we
> > > need security_tun_dev_free_security, don't we?
> > > I think we need:
> > > +err_free_sec: security_tun_dev_free_security(tun->security);
> > > and goto here on flow_init failures.
> >
> > tun_flow_init() always succeed. So we're ok here.
>
> True. So please make it return void then.
Heh I was looking at a stale tree, this is already fixed
in commit 944a1376b.
> > >
> > > > err_free_dev:
> > >
> > > Pls shift this one 1 space left for consistency.
> > >
> >
> > Ok.
> > > > free_netdev(dev);
> > > > return err;
> > > > --
> > > > 1.7.1
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at http://www.tux.org/lkml/
> > >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-09-11 10:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-11 8:24 [PATCH net] tuntap: correctly handle error in tun_set_iff() Jason Wang
2013-09-11 9:38 ` Michael S. Tsirkin
2013-09-11 9:55 ` Jason Wang
2013-09-11 10:08 ` Michael S. Tsirkin
2013-09-11 10:18 ` Jason Wang
2013-09-11 10:58 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).