Patrick McHardy <kaber@xxxxxxxxx> writes: > 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. 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@xxxxxxxxxxxx> 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; _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/virtualization
