[net,V2] tun: fix use after free for ptr_ring

Message ID 1526006965-9124-1-git-send-email-jasowang@redhat.com
State Accepted
Delegated to: David Miller
Headers show
Series
  • [net,V2] tun: fix use after free for ptr_ring
Related show

Commit Message

Jason Wang May 11, 2018, 2:49 a.m.
We used to initialize ptr_ring during TUNSETIFF, this is because its
size depends on the tx_queue_len of netdevice. And we try to clean it
up when socket were detached from netdevice. A race were spotted when
trying to do uninit during a read which will lead a use after free for
pointer ring. Solving this by always initialize a zero size ptr_ring
in open() and do resizing during TUNSETIFF, and then we can safely do
cleanup during close(). With this, there's no need for the workaround
that was introduced by commit 4df0bfc79904 ("tun: fix a memory leak
for tfile->tx_array").

Reported-by: syzbot+e8b902c3c3fadf0a9dba@syzkaller.appspotmail.com
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Fixes: 1576d9860599 ("tun: switch to use skb array for tx")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
Changes from v1:
- free ptr_ring during close()
- use tun_ptr_free() during resie for safety
---
 drivers/net/tun.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

Comments

Cong Wang May 11, 2018, 5:39 p.m. | #1
On Thu, May 10, 2018 at 7:49 PM, Jason Wang <jasowang@redhat.com> wrote:
>  static void __tun_detach(struct tun_file *tfile, bool clean)
>  {
>         struct tun_file *ntfile;
> @@ -736,7 +727,8 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
>                             tun->dev->reg_state == NETREG_REGISTERED)
>                                 unregister_netdevice(tun->dev);
>                 }
> -               tun_cleanup_tx_ring(tfile);
> +               if (tun)
> +                       xdp_rxq_info_unreg(&tfile->xdp_rxq);
>                 sock_put(&tfile->sk);
>         }
>  }
> @@ -783,14 +775,14 @@ static void tun_detach_all(struct net_device *dev)
>                 tun_napi_del(tun, tfile);
>                 /* Drop read queue */
>                 tun_queue_purge(tfile);
> +               xdp_rxq_info_unreg(&tfile->xdp_rxq);
>                 sock_put(&tfile->sk);
> -               tun_cleanup_tx_ring(tfile);
>         }
>         list_for_each_entry_safe(tfile, tmp, &tun->disabled, next) {
>                 tun_enable_queue(tfile);
>                 tun_queue_purge(tfile);
> +               xdp_rxq_info_unreg(&tfile->xdp_rxq);
>                 sock_put(&tfile->sk);
> -               tun_cleanup_tx_ring(tfile);

Are you sure this is safe?

xdp_rxq_info_unreg() can't be called more than once either,
please make sure the warning that commit c13da21cdb80
("tun: avoid calling xdp_rxq_info_unreg() twice") fixed will not
show up again.
Michael S. Tsirkin May 11, 2018, 6:19 p.m. | #2
On Fri, May 11, 2018 at 10:49:25AM +0800, Jason Wang wrote:
> We used to initialize ptr_ring during TUNSETIFF, this is because its
> size depends on the tx_queue_len of netdevice. And we try to clean it
> up when socket were detached from netdevice. A race were spotted when
> trying to do uninit during a read which will lead a use after free for
> pointer ring. Solving this by always initialize a zero size ptr_ring
> in open() and do resizing during TUNSETIFF, and then we can safely do
> cleanup during close(). With this, there's no need for the workaround
> that was introduced by commit 4df0bfc79904 ("tun: fix a memory leak
> for tfile->tx_array").
> 
> Reported-by: syzbot+e8b902c3c3fadf0a9dba@syzkaller.appspotmail.com
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Fixes: 1576d9860599 ("tun: switch to use skb array for tx")
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

and will you send the revert pls then?


> ---
> Changes from v1:
> - free ptr_ring during close()
> - use tun_ptr_free() during resie for safety
> ---
>  drivers/net/tun.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index ef33950..9fbbb32 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -681,15 +681,6 @@ static void tun_queue_purge(struct tun_file *tfile)
>  	skb_queue_purge(&tfile->sk.sk_error_queue);
>  }
>  
> -static void tun_cleanup_tx_ring(struct tun_file *tfile)
> -{
> -	if (tfile->tx_ring.queue) {
> -		ptr_ring_cleanup(&tfile->tx_ring, tun_ptr_free);
> -		xdp_rxq_info_unreg(&tfile->xdp_rxq);
> -		memset(&tfile->tx_ring, 0, sizeof(tfile->tx_ring));
> -	}
> -}
> -
>  static void __tun_detach(struct tun_file *tfile, bool clean)
>  {
>  	struct tun_file *ntfile;
> @@ -736,7 +727,8 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
>  			    tun->dev->reg_state == NETREG_REGISTERED)
>  				unregister_netdevice(tun->dev);
>  		}
> -		tun_cleanup_tx_ring(tfile);
> +		if (tun)
> +			xdp_rxq_info_unreg(&tfile->xdp_rxq);
>  		sock_put(&tfile->sk);
>  	}
>  }
> @@ -783,14 +775,14 @@ static void tun_detach_all(struct net_device *dev)
>  		tun_napi_del(tun, tfile);
>  		/* Drop read queue */
>  		tun_queue_purge(tfile);
> +		xdp_rxq_info_unreg(&tfile->xdp_rxq);
>  		sock_put(&tfile->sk);
> -		tun_cleanup_tx_ring(tfile);
>  	}
>  	list_for_each_entry_safe(tfile, tmp, &tun->disabled, next) {
>  		tun_enable_queue(tfile);
>  		tun_queue_purge(tfile);
> +		xdp_rxq_info_unreg(&tfile->xdp_rxq);
>  		sock_put(&tfile->sk);
> -		tun_cleanup_tx_ring(tfile);
>  	}
>  	BUG_ON(tun->numdisabled != 0);
>  
> @@ -834,7 +826,8 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
>  	}
>  
>  	if (!tfile->detached &&
> -	    ptr_ring_init(&tfile->tx_ring, dev->tx_queue_len, GFP_KERNEL)) {
> +	    ptr_ring_resize(&tfile->tx_ring, dev->tx_queue_len,
> +			    GFP_KERNEL, tun_ptr_free)) {
>  		err = -ENOMEM;
>  		goto out;
>  	}
> @@ -3219,6 +3212,11 @@ static int tun_chr_open(struct inode *inode, struct file * file)
>  					    &tun_proto, 0);
>  	if (!tfile)
>  		return -ENOMEM;
> +	if (ptr_ring_init(&tfile->tx_ring, 0, GFP_KERNEL)) {
> +		sk_free(&tfile->sk);
> +		return -ENOMEM;
> +	}
> +
>  	RCU_INIT_POINTER(tfile->tun, NULL);
>  	tfile->flags = 0;
>  	tfile->ifindex = 0;
> @@ -3239,8 +3237,6 @@ static int tun_chr_open(struct inode *inode, struct file * file)
>  
>  	sock_set_flag(&tfile->sk, SOCK_ZEROCOPY);
>  
> -	memset(&tfile->tx_ring, 0, sizeof(tfile->tx_ring));
> -
>  	return 0;
>  }
>  
> @@ -3249,6 +3245,7 @@ static int tun_chr_close(struct inode *inode, struct file *file)
>  	struct tun_file *tfile = file->private_data;
>  
>  	tun_detach(tfile, true);
> +	ptr_ring_cleanup(&tfile->tx_ring, tun_ptr_free);
>  
>  	return 0;
>  }
> -- 
> 2.7.4
David Miller May 14, 2018, 12:18 a.m. | #3
From: Jason Wang <jasowang@redhat.com>
Date: Fri, 11 May 2018 10:49:25 +0800

> We used to initialize ptr_ring during TUNSETIFF, this is because its
> size depends on the tx_queue_len of netdevice. And we try to clean it
> up when socket were detached from netdevice. A race were spotted when
> trying to do uninit during a read which will lead a use after free for
> pointer ring. Solving this by always initialize a zero size ptr_ring
> in open() and do resizing during TUNSETIFF, and then we can safely do
> cleanup during close(). With this, there's no need for the workaround
> that was introduced by commit 4df0bfc79904 ("tun: fix a memory leak
> for tfile->tx_array").
> 
> Reported-by: syzbot+e8b902c3c3fadf0a9dba@syzkaller.appspotmail.com
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Fixes: 1576d9860599 ("tun: switch to use skb array for tx")
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Jason, please address Cong Wang's concerns.

Thank you.
Jason Wang May 14, 2018, 1:52 a.m. | #4
On 2018年05月12日 01:39, Cong Wang wrote:
> On Thu, May 10, 2018 at 7:49 PM, Jason Wang <jasowang@redhat.com> wrote:
>>   static void __tun_detach(struct tun_file *tfile, bool clean)
>>   {
>>          struct tun_file *ntfile;
>> @@ -736,7 +727,8 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
>>                              tun->dev->reg_state == NETREG_REGISTERED)
>>                                  unregister_netdevice(tun->dev);
>>                  }
>> -               tun_cleanup_tx_ring(tfile);
>> +               if (tun)
>> +                       xdp_rxq_info_unreg(&tfile->xdp_rxq);
>>                  sock_put(&tfile->sk);
>>          }
>>   }
>> @@ -783,14 +775,14 @@ static void tun_detach_all(struct net_device *dev)
>>                  tun_napi_del(tun, tfile);
>>                  /* Drop read queue */
>>                  tun_queue_purge(tfile);
>> +               xdp_rxq_info_unreg(&tfile->xdp_rxq);
>>                  sock_put(&tfile->sk);
>> -               tun_cleanup_tx_ring(tfile);
>>          }
>>          list_for_each_entry_safe(tfile, tmp, &tun->disabled, next) {
>>                  tun_enable_queue(tfile);
>>                  tun_queue_purge(tfile);
>> +               xdp_rxq_info_unreg(&tfile->xdp_rxq);
>>                  sock_put(&tfile->sk);
>> -               tun_cleanup_tx_ring(tfile);
> Are you sure t is safe?
>
> xdp_rxq_info_unreg() can't be called more than once either,
> please make sure the warning that commit c13da21cdb80
> ("tun: avoid calling xdp_rxq_info_unreg() twice") fixed will not
> show up again.

I think it's safe. xdp_rxq_info_unreg() will be called when socket were 
detached from netdevice, and there's only two possible paths: release() 
and uninit(). We've synced them through rtnl lock.

Thanks
David Miller May 14, 2018, 6:48 p.m. | #5
From: Jason Wang <jasowang@redhat.com>
Date: Fri, 11 May 2018 10:49:25 +0800

> We used to initialize ptr_ring during TUNSETIFF, this is because its
> size depends on the tx_queue_len of netdevice. And we try to clean it
> up when socket were detached from netdevice. A race were spotted when
> trying to do uninit during a read which will lead a use after free for
> pointer ring. Solving this by always initialize a zero size ptr_ring
> in open() and do resizing during TUNSETIFF, and then we can safely do
> cleanup during close(). With this, there's no need for the workaround
> that was introduced by commit 4df0bfc79904 ("tun: fix a memory leak
> for tfile->tx_array").
> 
> Reported-by: syzbot+e8b902c3c3fadf0a9dba@syzkaller.appspotmail.com
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Fixes: 1576d9860599 ("tun: switch to use skb array for tx")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> Changes from v1:
> - free ptr_ring during close()
> - use tun_ptr_free() during resie for safety

Applied and queued up for -stable, thanks Jason.

Patch

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index ef33950..9fbbb32 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -681,15 +681,6 @@  static void tun_queue_purge(struct tun_file *tfile)
 	skb_queue_purge(&tfile->sk.sk_error_queue);
 }
 
-static void tun_cleanup_tx_ring(struct tun_file *tfile)
-{
-	if (tfile->tx_ring.queue) {
-		ptr_ring_cleanup(&tfile->tx_ring, tun_ptr_free);
-		xdp_rxq_info_unreg(&tfile->xdp_rxq);
-		memset(&tfile->tx_ring, 0, sizeof(tfile->tx_ring));
-	}
-}
-
 static void __tun_detach(struct tun_file *tfile, bool clean)
 {
 	struct tun_file *ntfile;
@@ -736,7 +727,8 @@  static void __tun_detach(struct tun_file *tfile, bool clean)
 			    tun->dev->reg_state == NETREG_REGISTERED)
 				unregister_netdevice(tun->dev);
 		}
-		tun_cleanup_tx_ring(tfile);
+		if (tun)
+			xdp_rxq_info_unreg(&tfile->xdp_rxq);
 		sock_put(&tfile->sk);
 	}
 }
@@ -783,14 +775,14 @@  static void tun_detach_all(struct net_device *dev)
 		tun_napi_del(tun, tfile);
 		/* Drop read queue */
 		tun_queue_purge(tfile);
+		xdp_rxq_info_unreg(&tfile->xdp_rxq);
 		sock_put(&tfile->sk);
-		tun_cleanup_tx_ring(tfile);
 	}
 	list_for_each_entry_safe(tfile, tmp, &tun->disabled, next) {
 		tun_enable_queue(tfile);
 		tun_queue_purge(tfile);
+		xdp_rxq_info_unreg(&tfile->xdp_rxq);
 		sock_put(&tfile->sk);
-		tun_cleanup_tx_ring(tfile);
 	}
 	BUG_ON(tun->numdisabled != 0);
 
@@ -834,7 +826,8 @@  static int tun_attach(struct tun_struct *tun, struct file *file,
 	}
 
 	if (!tfile->detached &&
-	    ptr_ring_init(&tfile->tx_ring, dev->tx_queue_len, GFP_KERNEL)) {
+	    ptr_ring_resize(&tfile->tx_ring, dev->tx_queue_len,
+			    GFP_KERNEL, tun_ptr_free)) {
 		err = -ENOMEM;
 		goto out;
 	}
@@ -3219,6 +3212,11 @@  static int tun_chr_open(struct inode *inode, struct file * file)
 					    &tun_proto, 0);
 	if (!tfile)
 		return -ENOMEM;
+	if (ptr_ring_init(&tfile->tx_ring, 0, GFP_KERNEL)) {
+		sk_free(&tfile->sk);
+		return -ENOMEM;
+	}
+
 	RCU_INIT_POINTER(tfile->tun, NULL);
 	tfile->flags = 0;
 	tfile->ifindex = 0;
@@ -3239,8 +3237,6 @@  static int tun_chr_open(struct inode *inode, struct file * file)
 
 	sock_set_flag(&tfile->sk, SOCK_ZEROCOPY);
 
-	memset(&tfile->tx_ring, 0, sizeof(tfile->tx_ring));
-
 	return 0;
 }
 
@@ -3249,6 +3245,7 @@  static int tun_chr_close(struct inode *inode, struct file *file)
 	struct tun_file *tfile = file->private_data;
 
 	tun_detach(tfile, true);
+	ptr_ring_cleanup(&tfile->tx_ring, tun_ptr_free);
 
 	return 0;
 }