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

[Eric W. Biederman]

Patrick McHardy <kaber at trash.net> writes:

> Eric W. Biederman wrote:
>> Patrick McHardy <kaber at trash.net> 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.

No problem.  tun was remarkably active this last while.
Sounds like a testament to virtual environments.

>> 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.

Acked-by: "Eric W. Biederman" <ebiederm at xmission.com>

Looks good.

> 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;