[PATCH 2/3] tun: free skb in early errors

[wexu]

From: Wei Xu 

tun_recvmsg() supports accepting skb by msg_control after
commit ac77cfd4258f ("tun: support receiving skb through msg_control"),
the skb if presented should be freed no matter how far it can go
along, otherwise it would be leaked.

This patch fixes several missed cases.

Signed-off-by: Wei Xu 
Reported-by: Matthew Rosato 

---
 drivers/net/tun.c | 24 ++--
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 9574900..4f4a842 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1952,8 +1952,11 @@ static ssize_t tun_do_read(struct tun_struct *tun, 
struct tun_file *tfile,
 
tun_debug(KERN_INFO, tun, "tun_do_read\n");
 
-   if (!iov_iter_count(to))
+   if (!iov_iter_count(to)) {
+   if (skb)
+   kfree_skb(skb);
return 0;
+   }
 
if (!skb) {
/* Read frames from ring */
@@ -2069,22 +2072,24 @@ static int tun_recvmsg(struct socket *sock, struct 
msghdr *m, size_t total_len,
 {
struct tun_file *tfile = container_of(sock, struct tun_file, socket);
struct tun_struct *tun = tun_get(tfile);
+   struct sk_buff *skb = m->msg_control;
int ret;
 
-   if (!tun)
-   return -EBADFD;
+   if (!tun) {
+   ret = -EBADFD;
+   goto out_free_skb;
+   }
 
if (flags & ~(MSG_DONTWAIT|MSG_TRUNC|MSG_ERRQUEUE)) {
ret = -EINVAL;
-   goto out;
+   goto out_put_tun;
}
if (flags & MSG_ERRQUEUE) {
ret = sock_recv_errqueue(sock->sk, m, total_len,
 SOL_PACKET, TUN_TX_TIMESTAMP);
goto out;
}
-   ret = tun_do_read(tun, tfile, >msg_iter, flags & MSG_DONTWAIT,
- m->msg_control);
+   ret = tun_do_read(tun, tfile, >msg_iter, flags & MSG_DONTWAIT, skb);
if (ret > (ssize_t)total_len) {
m->msg_flags |= MSG_TRUNC;
ret = flags & MSG_TRUNC ? ret : total_len;
@@ -2092,6 +2097,13 @@ static int tun_recvmsg(struct socket *sock, struct 
msghdr *m, size_t total_len,
 out:
tun_put(tun);
return ret;
+
+out_put_tun:
+   tun_put(tun);
+out_free_skb:
+   if (skb)
+   kfree_skb(skb);
+   return ret;
 }
 
 static int tun_peek_len(struct socket *sock)
-- 
1.8.3.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 2/3] tun: free skb in early errors

[wexu]

From: Wei Xu 

tun_recvmsg() supports accepting skb by msg_control after
commit ac77cfd4258f ("tun: support receiving skb through msg_control"),
the skb if presented should be freed within the function, otherwise it
would be leaked.

Signed-off-by: Wei Xu 
Reported-by: Matthew Rosato 
---
 drivers/net/tun.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 6a7bde9..5563430 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -2067,14 +2067,17 @@ static int tun_recvmsg(struct socket *sock, struct 
msghdr *m, size_t total_len,
 {
struct tun_file *tfile = container_of(sock, struct tun_file, socket);
struct tun_struct *tun = tun_get(tfile);
+   struct sk_buff *skb = m->msg_control;
int ret;
 
-   if (!tun)
-   return -EBADFD;
+   if (!tun) {
+   ret = -EBADFD;
+   goto out_free_skb;
+   }
 
if (flags & ~(MSG_DONTWAIT|MSG_TRUNC|MSG_ERRQUEUE)) {
ret = -EINVAL;
-   goto out;
+   goto out_free_skb;
}
if (flags & MSG_ERRQUEUE) {
ret = sock_recv_errqueue(sock->sk, m, total_len,
@@ -2087,6 +2090,11 @@ static int tun_recvmsg(struct socket *sock, struct 
msghdr *m, size_t total_len,
m->msg_flags |= MSG_TRUNC;
ret = flags & MSG_TRUNC ? ret : total_len;
}
+   goto out;
+
+out_free_skb:
+   if (skb)
+   kfree_skb(skb);
 out:
tun_put(tun);
return ret;
-- 
1.8.3.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3] tun: free skb in early errors

[Michael S. Tsirkin]

On Fri, Dec 01, 2017 at 05:10:37AM -0500, w...@redhat.com wrote:
> From: Wei Xu 
> 
> tun_recvmsg() supports accepting skb by msg_control after
> commit ac77cfd4258f ("tun: support receiving skb through msg_control"),
> the skb if presented should be freed no matter how far it can go
> along, otherwise it would be leaked.
> 
> This patch fixes several missed cases.
> 
> Signed-off-by: Wei Xu 
> Reported-by: Matthew Rosato 


Acked-by: Michael S. Tsirkin 

> ---
>  drivers/net/tun.c | 24 ++--
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 9574900..4f4a842 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1952,8 +1952,11 @@ static ssize_t tun_do_read(struct tun_struct *tun, 
> struct tun_file *tfile,
>  
>   tun_debug(KERN_INFO, tun, "tun_do_read\n");
>  
> - if (!iov_iter_count(to))
> + if (!iov_iter_count(to)) {
> + if (skb)
> + kfree_skb(skb);
>   return 0;
> + }
>  
>   if (!skb) {
>   /* Read frames from ring */
> @@ -2069,22 +2072,24 @@ static int tun_recvmsg(struct socket *sock, struct 
> msghdr *m, size_t total_len,
>  {
>   struct tun_file *tfile = container_of(sock, struct tun_file, socket);
>   struct tun_struct *tun = tun_get(tfile);
> + struct sk_buff *skb = m->msg_control;
>   int ret;
>  
> - if (!tun)
> - return -EBADFD;
> + if (!tun) {
> + ret = -EBADFD;
> + goto out_free_skb;
> + }
>  
>   if (flags & ~(MSG_DONTWAIT|MSG_TRUNC|MSG_ERRQUEUE)) {
>   ret = -EINVAL;
> - goto out;
> + goto out_put_tun;
>   }
>   if (flags & MSG_ERRQUEUE) {
>   ret = sock_recv_errqueue(sock->sk, m, total_len,
>SOL_PACKET, TUN_TX_TIMESTAMP);
>   goto out;
>   }
> - ret = tun_do_read(tun, tfile, >msg_iter, flags & MSG_DONTWAIT,
> -   m->msg_control);
> + ret = tun_do_read(tun, tfile, >msg_iter, flags & MSG_DONTWAIT, skb);
>   if (ret > (ssize_t)total_len) {
>   m->msg_flags |= MSG_TRUNC;
>   ret = flags & MSG_TRUNC ? ret : total_len;
> @@ -2092,6 +2097,13 @@ static int tun_recvmsg(struct socket *sock, struct 
> msghdr *m, size_t total_len,
>  out:
>   tun_put(tun);
>   return ret;
> +
> +out_put_tun:
> + tun_put(tun);
> +out_free_skb:
> + if (skb)
> + kfree_skb(skb);
> + return ret;
>  }
>  
>  static int tun_peek_len(struct socket *sock)
> -- 
> 1.8.3.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3] tun: free skb in early errors

[Michael S. Tsirkin]

On Fri, Dec 01, 2017 at 03:07:44PM +0800, Jason Wang wrote:
> 
> 
> On 2017年12月01日 13:54, w...@redhat.com wrote:
> > From: Wei Xu 
> > 
> > tun_recvmsg() supports accepting skb by msg_control after
> > commit ac77cfd4258f ("tun: support receiving skb through msg_control"),
> > the skb if presented should be freed within the function, otherwise it
> > would be leaked.
> > 
> > Signed-off-by: Wei Xu 
> > Reported-by: Matthew Rosato 
> > ---
> >   drivers/net/tun.c | 14 +++---
> >   1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 6a7bde9..5563430 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -2067,14 +2067,17 @@ static int tun_recvmsg(struct socket *sock, struct 
> > msghdr *m, size_t total_len,
> >   {
> > struct tun_file *tfile = container_of(sock, struct tun_file, socket);
> > struct tun_struct *tun = tun_get(tfile);
> > +   struct sk_buff *skb = m->msg_control;
> > int ret;
> > -   if (!tun)
> > -   return -EBADFD;
> > +   if (!tun) {
> > +   ret = -EBADFD;
> > +   goto out_free_skb;
> 
> Unfortunately, you can't to there since tun is NULL.

Right, this should just be kfree_skb(skb); return -EBADFD;

> 
> > +   }
> > if (flags & ~(MSG_DONTWAIT|MSG_TRUNC|MSG_ERRQUEUE)) {
> > ret = -EINVAL;
> > -   goto out;
> > +   goto out_free_skb;
> > }
> > if (flags & MSG_ERRQUEUE) {
> > ret = sock_recv_errqueue(sock->sk, m, total_len,
> > @@ -2087,6 +2090,11 @@ static int tun_recvmsg(struct socket *sock, struct 
> > msghdr *m, size_t total_len,
> > m->msg_flags |= MSG_TRUNC;
> > ret = flags & MSG_TRUNC ? ret : total_len;
> > }
> > +   goto out;
> 
> We usually don't use goto in the case of success, and you need deal with the
> case skb != NULL but iov_iter_count(to) == 0 in tun_do_read().
> 
> Thanks

I agree, the way to lay this out is:


tun_put(tun);
return ret;

err:
tun_put(tun);
err_tun:
if (skb)
kfree_skb(skb);
return ret;




> > +
> > +out_free_skb:
> > +   if (skb)
> > +   kfree_skb(skb);
> >   out:
> > tun_put(tun);
> > return ret;
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3] tun: free skb in early errors

[Jason Wang]

On 2017年12月01日 13:54, w...@redhat.com wrote:

From: Wei Xu 

tun_recvmsg() supports accepting skb by msg_control after
commit ac77cfd4258f ("tun: support receiving skb through msg_control"),
the skb if presented should be freed within the function, otherwise it
would be leaked.

Signed-off-by: Wei Xu 
Reported-by: Matthew Rosato 
---
  drivers/net/tun.c | 14 +++---
  1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 6a7bde9..5563430 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -2067,14 +2067,17 @@ static int tun_recvmsg(struct socket *sock, struct 
msghdr *m, size_t total_len,
  {
struct tun_file *tfile = container_of(sock, struct tun_file, socket);
struct tun_struct *tun = tun_get(tfile);
+   struct sk_buff *skb = m->msg_control;
int ret;
  
-	if (!tun)

-   return -EBADFD;
+   if (!tun) {
+   ret = -EBADFD;
+   goto out_free_skb;


Unfortunately, you can't to there since tun is NULL.



+   }
  
  	if (flags & ~(MSG_DONTWAIT|MSG_TRUNC|MSG_ERRQUEUE)) {

ret = -EINVAL;
-   goto out;
+   goto out_free_skb;
}
if (flags & MSG_ERRQUEUE) {
ret = sock_recv_errqueue(sock->sk, m, total_len,
@@ -2087,6 +2090,11 @@ static int tun_recvmsg(struct socket *sock, struct 
msghdr *m, size_t total_len,
m->msg_flags |= MSG_TRUNC;
ret = flags & MSG_TRUNC ? ret : total_len;
}
+   goto out;


We usually don't use goto in the case of success, and you need deal with 
the case skb != NULL but iov_iter_count(to) == 0 in tun_do_read().


Thanks


+
+out_free_skb:
+   if (skb)
+   kfree_skb(skb);
  out:
tun_put(tun);
return ret;


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization