Re: [PATCH 4/5] lguest: use KVM hypercalls

[Patrick McHardy <kaber-dcUjhNyLwpNeoWH0uzbU5w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 1894 bytes --]

Eric W. Biederman wrote:
> Patrick McHardy <kaber-dcUjhNyLwpNeoWH0uzbU5w@public.gmane.org> 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.


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1170 bytes --]

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;

[-- Attachment #3: Type: text/plain, Size: 158 bytes --]

_______________________________________________
Lguest mailing list
Lguest-mnsaURCQ41sdnm+yROfE0A@public.gmane.org
https://ozlabs.org/mailman/listinfo/lguest