| From b196d88aba8ac72b775137854121097f4c4c6862 Mon Sep 17 00:00:00 2001 |
| From: Jason Wang <jasowang@redhat.com> |
| Date: Fri, 11 May 2018 10:49:25 +0800 |
| Subject: tun: fix use after free for ptr_ring |
| |
| From: Jason Wang <jasowang@redhat.com> |
| |
| commit b196d88aba8ac72b775137854121097f4c4c6862 upstream. |
| |
| 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"). |
| |
| Backport Note :- |
| Comparison with the upstream patch: |
| [1] A "semantic revert" of the changes made in |
| 4df0bfc799("tun: fix a memory leak for tfile->tx_array"). |
| 4df0bfc799 was applied upstream, and then skb array was changed |
| to use ptr_ring. The upstream patch then removes the changes introduced |
| by 4df0bfc799. This backport does the same; "revert" the changes |
| made by 4df0bfc799. |
| [2] xdp_rxq_info_unreg() being called in relevant locations |
| As xdp_rxq_info related patches are not present in 4.14, these |
| changes are not needed in the backport. |
| [3] An instance of ptr_ring_init needs to be replaced by skb_array_init |
| Inside tun_attach() |
| [4] ptr_ring_cleanup needs to be replaced by skb_array_cleanup |
| Inside tun_chr_close() |
| |
| Note that the backport for 7063efd33b ("tuntap: fix use after free during release") |
| needs to be applied on top of this patch. |
| |
| 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> |
| Signed-off-by: David S. Miller <davem@davemloft.net> |
| Signed-off-by: Zubin Mithra <zsm@chromium.org> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| drivers/net/tun.c | 21 +++++++-------------- |
| 1 file changed, 7 insertions(+), 14 deletions(-) |
| |
| --- a/drivers/net/tun.c |
| +++ b/drivers/net/tun.c |
| @@ -534,14 +534,6 @@ static void tun_queue_purge(struct tun_f |
| skb_queue_purge(&tfile->sk.sk_error_queue); |
| } |
| |
| -static void tun_cleanup_tx_array(struct tun_file *tfile) |
| -{ |
| - if (tfile->tx_array.ring.queue) { |
| - skb_array_cleanup(&tfile->tx_array); |
| - memset(&tfile->tx_array, 0, sizeof(tfile->tx_array)); |
| - } |
| -} |
| - |
| static void __tun_detach(struct tun_file *tfile, bool clean) |
| { |
| struct tun_file *ntfile; |
| @@ -583,7 +575,6 @@ static void __tun_detach(struct tun_file |
| tun->dev->reg_state == NETREG_REGISTERED) |
| unregister_netdevice(tun->dev); |
| } |
| - tun_cleanup_tx_array(tfile); |
| sock_put(&tfile->sk); |
| } |
| } |
| @@ -623,13 +614,11 @@ static void tun_detach_all(struct net_de |
| /* Drop read queue */ |
| tun_queue_purge(tfile); |
| sock_put(&tfile->sk); |
| - tun_cleanup_tx_array(tfile); |
| } |
| list_for_each_entry_safe(tfile, tmp, &tun->disabled, next) { |
| tun_enable_queue(tfile); |
| tun_queue_purge(tfile); |
| sock_put(&tfile->sk); |
| - tun_cleanup_tx_array(tfile); |
| } |
| BUG_ON(tun->numdisabled != 0); |
| |
| @@ -675,7 +664,7 @@ static int tun_attach(struct tun_struct |
| } |
| |
| if (!tfile->detached && |
| - skb_array_init(&tfile->tx_array, dev->tx_queue_len, GFP_KERNEL)) { |
| + skb_array_resize(&tfile->tx_array, dev->tx_queue_len, GFP_KERNEL)) { |
| err = -ENOMEM; |
| goto out; |
| } |
| @@ -2624,6 +2613,11 @@ static int tun_chr_open(struct inode *in |
| &tun_proto, 0); |
| if (!tfile) |
| return -ENOMEM; |
| + if (skb_array_init(&tfile->tx_array, 0, GFP_KERNEL)) { |
| + sk_free(&tfile->sk); |
| + return -ENOMEM; |
| + } |
| + |
| RCU_INIT_POINTER(tfile->tun, NULL); |
| tfile->flags = 0; |
| tfile->ifindex = 0; |
| @@ -2644,8 +2638,6 @@ static int tun_chr_open(struct inode *in |
| |
| sock_set_flag(&tfile->sk, SOCK_ZEROCOPY); |
| |
| - memset(&tfile->tx_array, 0, sizeof(tfile->tx_array)); |
| - |
| return 0; |
| } |
| |
| @@ -2654,6 +2646,7 @@ static int tun_chr_close(struct inode *i |
| struct tun_file *tfile = file->private_data; |
| |
| tun_detach(tfile, true); |
| + skb_array_cleanup(&tfile->tx_array); |
| |
| return 0; |
| } |