ACK: [PATCH 0/1] vhost/vsock: fix use-after-free in network stack callers

[Kleber Souza]

On 12/18/18 2:07 AM, Tyler Hicks wrote:
> From: Stefan Hajnoczi <stefanha at redhat.com>
>
> If the network stack calls .send_pkt()/.cancel_pkt() during .release(),
> a struct vhost_vsock use-after-free is possible.  This occurs because
> .release() does not wait for other CPUs to stop using struct
> vhost_vsock.
>
> Switch to an RCU-enabled hashtable (indexed by guest CID) so that
> .release() can wait for other CPUs by calling synchronize_rcu().  This
> also eliminates vhost_vsock_lock acquisition in the data path so it
> could have a positive effect on performance.
>
> This is CVE-2018-14625 "kernel: use-after-free Read in vhost_transport_send_pkt".
>
> Cc: stable at vger.kernel.org
> Reported-and-tested-by: syzbot+bd391451452fb0b93039 at syzkaller.appspotmail.com
> Reported-by: syzbot+e3e074963495f92a89ed at syzkaller.appspotmail.com
> Reported-by: syzbot+d5a0a170c5069658b141 at syzkaller.appspotmail.com
> Signed-off-by: Stefan Hajnoczi <stefanha at redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> Acked-by: Jason Wang <jasowang at redhat.com>
>
> CVE-2018-14625
>
> (cherry picked from commit 834e772c8db0c6a275d75315d90aba4ebbb1e249)
> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>

Clean cherry-pick.


Acked-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>

> ---
>  drivers/vhost/vsock.c | 57 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 33 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index 34bc3ab40c6d..51879ed18652 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -15,6 +15,7 @@
>  #include <net/sock.h>
>  #include <linux/virtio_vsock.h>
>  #include <linux/vhost.h>
> +#include <linux/hashtable.h>
>  
>  #include <net/af_vsock.h>
>  #include "vhost.h"
> @@ -27,14 +28,14 @@ enum {
>  
>  /* Used to track all the vhost_vsock instances on the system. */
>  static DEFINE_SPINLOCK(vhost_vsock_lock);
> -static LIST_HEAD(vhost_vsock_list);
> +static DEFINE_READ_MOSTLY_HASHTABLE(vhost_vsock_hash, 8);
>  
>  struct vhost_vsock {
>  	struct vhost_dev dev;
>  	struct vhost_virtqueue vqs[2];
>  
> -	/* Link to global vhost_vsock_list, protected by vhost_vsock_lock */
> -	struct list_head list;
> +	/* Link to global vhost_vsock_hash, writes use vhost_vsock_lock */
> +	struct hlist_node hash;
>  
>  	struct vhost_work send_pkt_work;
>  	spinlock_t send_pkt_list_lock;
> @@ -50,11 +51,14 @@ static u32 vhost_transport_get_local_cid(void)
>  	return VHOST_VSOCK_DEFAULT_HOST_CID;
>  }
>  
> -static struct vhost_vsock *__vhost_vsock_get(u32 guest_cid)
> +/* Callers that dereference the return value must hold vhost_vsock_lock or the
> + * RCU read lock.
> + */
> +static struct vhost_vsock *vhost_vsock_get(u32 guest_cid)
>  {
>  	struct vhost_vsock *vsock;
>  
> -	list_for_each_entry(vsock, &vhost_vsock_list, list) {
> +	hash_for_each_possible_rcu(vhost_vsock_hash, vsock, hash, guest_cid) {
>  		u32 other_cid = vsock->guest_cid;
>  
>  		/* Skip instances that have no CID yet */
> @@ -69,17 +73,6 @@ static struct vhost_vsock *__vhost_vsock_get(u32 guest_cid)
>  	return NULL;
>  }
>  
> -static struct vhost_vsock *vhost_vsock_get(u32 guest_cid)
> -{
> -	struct vhost_vsock *vsock;
> -
> -	spin_lock_bh(&vhost_vsock_lock);
> -	vsock = __vhost_vsock_get(guest_cid);
> -	spin_unlock_bh(&vhost_vsock_lock);
> -
> -	return vsock;
> -}
> -
>  static void
>  vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
>  			    struct vhost_virtqueue *vq)
> @@ -210,9 +203,12 @@ vhost_transport_send_pkt(struct virtio_vsock_pkt *pkt)
>  	struct vhost_vsock *vsock;
>  	int len = pkt->len;
>  
> +	rcu_read_lock();
> +
>  	/* Find the vhost_vsock according to guest context id  */
>  	vsock = vhost_vsock_get(le64_to_cpu(pkt->hdr.dst_cid));
>  	if (!vsock) {
> +		rcu_read_unlock();
>  		virtio_transport_free_pkt(pkt);
>  		return -ENODEV;
>  	}
> @@ -225,6 +221,8 @@ vhost_transport_send_pkt(struct virtio_vsock_pkt *pkt)
>  	spin_unlock_bh(&vsock->send_pkt_list_lock);
>  
>  	vhost_work_queue(&vsock->dev, &vsock->send_pkt_work);
> +
> +	rcu_read_unlock();
>  	return len;
>  }
>  
> @@ -234,12 +232,15 @@ vhost_transport_cancel_pkt(struct vsock_sock *vsk)
>  	struct vhost_vsock *vsock;
>  	struct virtio_vsock_pkt *pkt, *n;
>  	int cnt = 0;
> +	int ret = -ENODEV;
>  	LIST_HEAD(freeme);
>  
> +	rcu_read_lock();
> +
>  	/* Find the vhost_vsock according to guest context id  */
>  	vsock = vhost_vsock_get(vsk->remote_addr.svm_cid);
>  	if (!vsock)
> -		return -ENODEV;
> +		goto out;
>  
>  	spin_lock_bh(&vsock->send_pkt_list_lock);
>  	list_for_each_entry_safe(pkt, n, &vsock->send_pkt_list, list) {
> @@ -265,7 +266,10 @@ vhost_transport_cancel_pkt(struct vsock_sock *vsk)
>  			vhost_poll_queue(&tx_vq->poll);
>  	}
>  
> -	return 0;
> +	ret = 0;
> +out:
> +	rcu_read_unlock();
> +	return ret;
>  }
>  
>  static struct virtio_vsock_pkt *
> @@ -533,10 +537,6 @@ static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
>  	spin_lock_init(&vsock->send_pkt_list_lock);
>  	INIT_LIST_HEAD(&vsock->send_pkt_list);
>  	vhost_work_init(&vsock->send_pkt_work, vhost_transport_send_pkt_work);
> -
> -	spin_lock_bh(&vhost_vsock_lock);
> -	list_add_tail(&vsock->list, &vhost_vsock_list);
> -	spin_unlock_bh(&vhost_vsock_lock);
>  	return 0;
>  
>  out:
> @@ -577,9 +577,13 @@ static int vhost_vsock_dev_release(struct inode *inode, struct file *file)
>  	struct vhost_vsock *vsock = file->private_data;
>  
>  	spin_lock_bh(&vhost_vsock_lock);
> -	list_del(&vsock->list);
> +	if (vsock->guest_cid)
> +		hash_del_rcu(&vsock->hash);
>  	spin_unlock_bh(&vhost_vsock_lock);
>  
> +	/* Wait for other CPUs to finish using vsock */
> +	synchronize_rcu();
> +
>  	/* Iterating over all connections for all CIDs to find orphans is
>  	 * inefficient.  Room for improvement here. */
>  	vsock_for_each_connected_socket(vhost_vsock_reset_orphans);
> @@ -620,12 +624,17 @@ static int vhost_vsock_set_cid(struct vhost_vsock *vsock, u64 guest_cid)
>  
>  	/* Refuse if CID is already in use */
>  	spin_lock_bh(&vhost_vsock_lock);
> -	other = __vhost_vsock_get(guest_cid);
> +	other = vhost_vsock_get(guest_cid);
>  	if (other && other != vsock) {
>  		spin_unlock_bh(&vhost_vsock_lock);
>  		return -EADDRINUSE;
>  	}
> +
> +	if (vsock->guest_cid)
> +		hash_del_rcu(&vsock->hash);
> +
>  	vsock->guest_cid = guest_cid;
> +	hash_add_rcu(vhost_vsock_hash, &vsock->hash, guest_cid);
>  	spin_unlock_bh(&vhost_vsock_lock);
>  
>  	return 0;