All of lore.kernel.org
 help / color / mirror / Atom feed
From: Zubin Mithra <zsm@chromium.org>
To: stable@vger.kernel.org
Cc: gregkh@linuxfoundation.org, jasowang@redhat.com,
	davem@davemloft.net, groeck@chromium.org, zsm@chromium.org
Subject: [PATCH v4.14.y] tun: fix use after free for ptr_ring
Date: Fri, 31 Aug 2018 14:36:42 -0700	[thread overview]
Message-ID: <20180831213642.251705-1-zsm@chromium.org> (raw)

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 :-
This is a backport of following 2 upstream patches(the second fixes the
first).
	b196d88aba ("tun: fix use after free for ptr_ring")
	7063efd33b ("tuntap: fix use after free during release")

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 fix 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.
[4] ptr_ring_cleanup needs to be replaced by skb_array_cleanup.
        b196d88ab places the cleanup function in tun_chr_close() only to
	later move it into __tun_detach in upstream commit
        7063efd33bb("tuntap: fix use after free during release"). So
	place skb_array_cleanup in __tun_detach.

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>
---
 drivers/net/tun.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index cb17ffadfc30..e0baea2dfd3c 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -534,14 +534,6 @@ static void tun_queue_purge(struct tun_file *tfile)
 	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,7 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
 			    tun->dev->reg_state == NETREG_REGISTERED)
 				unregister_netdevice(tun->dev);
 		}
-		tun_cleanup_tx_array(tfile);
+		skb_array_cleanup(&tfile->tx_array);
 		sock_put(&tfile->sk);
 	}
 }
@@ -623,13 +615,11 @@ static void tun_detach_all(struct net_device *dev)
 		/* 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 +665,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file, bool skip_filte
 	}
 
 	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 +2614,11 @@ static int tun_chr_open(struct inode *inode, struct file * file)
 					    &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 +2639,6 @@ static int tun_chr_open(struct inode *inode, struct file * file)
 
 	sock_set_flag(&tfile->sk, SOCK_ZEROCOPY);
 
-	memset(&tfile->tx_array, 0, sizeof(tfile->tx_array));
-
 	return 0;
 }
 
-- 
2.19.0.rc0.228.g281dcd1b4d0-goog

             reply	other threads:[~2018-09-01  1:46 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-31 21:36 Zubin Mithra [this message]
2018-09-01  3:53 ` [PATCH v4.14.y] tun: fix use after free for ptr_ring Greg KH

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180831213642.251705-1-zsm@chromium.org \
    --to=zsm@chromium.org \
    --cc=davem@davemloft.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=groeck@chromium.org \
    --cc=jasowang@redhat.com \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.