Eric W. Biederman wrote:
Patrick McHardy <kaber@xxxxxxxxx> writes:
When creating the device using tunctl the sk->sk_sleep poiner is
set to the read_wait completion of the file opened by tunctl, but
it is not refreshed when attaching to lguest or released when
closing the file, causing a stale pointer dereference in
tun_sock_write_space().
Eric, please review. Thanks.
That looks a little better. Certainly as the socket currently
lives with the tun_struct instead of the tun_file it make sense.
I'm not at all certain it makes sense for the socket to live in
tun_struct instead of tun_file.
I happened to glance at the code about a week ago, and realized that
the introduction of the socket had done horribly things to the
guarantees I had introduced, and I haven't had a chance to think
through and figure out what the code should be doing.
I am certain that:
opening a tap device and then "ip link del tap0" while holding
the tap open leads into a territory of madness right now.
And apparently so does reattaching to an existing tun device.
Patrick I'm not seeing anything in the particular patch you pointed
out that would cause crashes.
It might have been a different patch or a combination, I assumed it
was your patch since git annotate pointed to it and it was a very
recent change.
Other lurking bugs aside your patch appears slightly off.
tun->sk->sk_sleep in __tun_detach is correct.
Setting sk_sleep on both paths to tun_attach instead
of in tun_attach is wrong. You are performing the assignment
before we complete the permission checks into tun_attach, which
means there is no guarantee that the tun_attach will succeed.
I see. How about this patch instead? It moves the sk_sleep assignment
to tun_attach, after the permission checks took place.
Thanks.
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index a1b0697..4c5ae95 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -155,6 +155,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file)
err = 0;
tfile->tun = tun;
tun->tfile = tfile;
+ tun->sk->sk_sleep = &tfile->read_wait;
dev_hold(tun->dev);
atomic_inc(&tfile->count);
@@ -173,6 +174,8 @@ static void __tun_detach(struct tun_struct *tun)
tun->tfile = NULL;
netif_tx_unlock_bh(tun->dev);
+ tun->sk->sk_sleep = NULL;
+
/* Drop read queue */
skb_queue_purge(&tun->readq);
@@ -861,7 +864,6 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
struct sock *sk;
struct tun_struct *tun;
struct net_device *dev;
- struct tun_file *tfile = file->private_data;
int err;
dev = __dev_get_by_name(net, ifr->ifr_name);
@@ -925,7 +927,6 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
sk->sk_write_space = tun_sock_write_space;
sk->sk_destruct = tun_sock_destruct;
sk->sk_sndbuf = INT_MAX;
- sk->sk_sleep = &tfile->read_wait;
tun->sk = sk;
container_of(sk, struct tun_sock, sk)->tun = tun;
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/virtualization
