[PATCH 2/3] vhost: add access_ok checks

[Michael S. Tsirkin]

On biarch kernels, it is not safe to do copy from/to user, even with use_mm,
unless we checked the address range with access_ok previously.  Implement these
checks to enforce safe memory accesses.

Reported-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
---
 drivers/vhost/net.c   |   17 ++++++-
 drivers/vhost/vhost.c |  111 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/vhost/vhost.h |    2 +
 3 files changed, 127 insertions(+), 3 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 1b509a0..1aacd8c 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -493,6 +493,12 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
 	}
 	vq = n->vqs + index;
 	mutex_lock(&vq->mutex);
+
+	/* Verify that ring has been setup correctly. */
+	if (!vhost_vq_access_ok(vq)) {
+		r = -EFAULT;
+		goto err;
+	}
 	sock = get_socket(fd);
 	if (IS_ERR(sock)) {
 		r = PTR_ERR(sock);
@@ -539,12 +545,17 @@ done:
 	return err;
 }
 
-static void vhost_net_set_features(struct vhost_net *n, u64 features)
+static int vhost_net_set_features(struct vhost_net *n, u64 features)
 {
 	size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
 		sizeof(struct virtio_net_hdr) : 0;
 	int i;
 	mutex_lock(&n->dev.mutex);
+	if ((features & (1 << VHOST_F_LOG_ALL)) &&
+	    !vhost_log_access_ok(&n->dev)) {
+		mutex_unlock(&n->dev.mutex);
+		return -EFAULT;
+	}
 	n->dev.acked_features = features;
 	smp_wmb();
 	for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
@@ -554,6 +565,7 @@ static void vhost_net_set_features(struct vhost_net *n, u64 features)
 	}
 	vhost_net_flush(n);
 	mutex_unlock(&n->dev.mutex);
+	return 0;
 }
 
 static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
@@ -580,8 +592,7 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
 			return r;
 		if (features & ~VHOST_FEATURES)
 			return -EOPNOTSUPP;
-		vhost_net_set_features(n, features);
-		return 0;
+		return vhost_net_set_features(n, features);
 	case VHOST_RESET_OWNER:
 		return vhost_net_reset_owner(n);
 	default:
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 29f1675..33e06bf 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -224,6 +224,91 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
 	dev->mm = NULL;
 }
 
+static int log_access_ok(void __user *log_base, u64 addr, unsigned long sz)
+{
+	u64 a = addr / VHOST_PAGE_SIZE / 8;
+	/* Make sure 64 bit math will not overflow. */
+	if (a > ULONG_MAX - (unsigned long)log_base ||
+	    a + (unsigned long)log_base > ULONG_MAX)
+		return -EFAULT;
+
+	return access_ok(VERIFY_WRITE, log_base + a,
+			 (sz + VHOST_PAGE_SIZE * 8 - 1) / VHOST_PAGE_SIZE / 8);
+}
+
+/* Caller should have vq mutex and device mutex. */
+static int vq_memory_access_ok(struct vhost_virtqueue *vq, struct vhost_memory *mem,
+			       int log_all)
+{
+	int i;
+	for (i = 0; i < mem->nregions; ++i) {
+		struct vhost_memory_region *m = mem->regions + i;
+		unsigned long a = m->userspace_addr;
+		if (m->memory_size > ULONG_MAX)
+			return 0;
+		else if (!access_ok(VERIFY_WRITE, (void __user *)a,
+				    m->memory_size))
+			return 0;
+		else if (log_all && !log_access_ok(vq->log_base,
+						   m->guest_phys_addr,
+						   m->memory_size))
+			return 0;
+	}
+	return 1;
+}
+
+/* Can we switch to this memory table? */
+/* Caller should have device mutex but not vq mutex */
+static int memory_access_ok(struct vhost_dev *d, struct vhost_memory *mem,
+			    int log_all)
+{
+	int i;
+	for (i = 0; i < d->nvqs; ++i) {
+		int ok;
+		mutex_lock(&d->vqs[i].mutex);
+		/* If ring is not running, will check when it's enabled. */
+		if (&d->vqs[i].private_data)
+			ok = vq_memory_access_ok(&d->vqs[i], mem, log_all);
+		else
+			ok = 1;
+		mutex_unlock(&d->vqs[i].mutex);
+		if (!ok)
+			return 0;
+	}
+	return 1;
+}
+
+static int vq_access_ok(unsigned int num,
+			struct vring_desc __user *desc,
+			struct vring_avail __user *avail,
+			struct vring_used __user *used)
+{
+	return access_ok(VERIFY_READ, desc, num * sizeof *desc) &&
+	       access_ok(VERIFY_READ, avail,
+			 sizeof *avail + num * sizeof *avail->ring) &&
+	       access_ok(VERIFY_WRITE, used,
+			sizeof *used + num * sizeof *used->ring);
+}
+
+/* Can we log writes? */
+/* Caller should have device mutex but not vq mutex */
+int vhost_log_access_ok(struct vhost_dev *dev)
+{
+	return memory_access_ok(dev, dev->memory, 1);
+}
+
+/* Can we start vq? */
+/* Caller should have vq mutex and device mutex */
+int vhost_vq_access_ok(struct vhost_virtqueue *vq)
+{
+	return vq_access_ok(vq->num, vq->desc, vq->avail, vq->used) &&
+		vq_memory_access_ok(vq, vq->dev->memory,
+			    vhost_has_feature(vq->dev, VHOST_F_LOG_ALL)) &&
+		(!vq->log_used || log_access_ok(vq->log_base, vq->log_addr,
+					sizeof *vq->used +
+					vq->num * sizeof *vq->used->ring));
+}
+
 static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
 {
 	struct vhost_memory mem, *newmem, *oldmem;
@@ -247,6 +332,9 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
 		kfree(newmem);
 		return r;
 	}
+
+	if (!memory_access_ok(d, newmem, vhost_has_feature(d, VHOST_F_LOG_ALL)))
+		return -EFAULT;
 	oldmem = d->memory;
 	rcu_assign_pointer(d->memory, newmem);
 	synchronize_rcu();
@@ -348,6 +436,29 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
 			r = -EINVAL;
 			break;
 		}
+
+		/* We only verify access here if backend is configured.
+		 * If it is not, we don't as size might not have been setup.
+		 * We will verify when backend is configured. */
+		if (vq->private_data) {
+			if (!vq_access_ok(vq->num,
+				(void __user *)(unsigned long)a.desc_user_addr,
+				(void __user *)(unsigned long)a.avail_user_addr,
+				(void __user *)(unsigned long)a.used_user_addr)) {
+				r = -EINVAL;
+				break;
+			}
+
+			/* Also validate log access for used ring if enabled. */
+			if ((a.flags & (0x1 << VHOST_VRING_F_LOG)) &&
+			    !log_access_ok(vq->log_base, a.log_guest_addr,
+					   sizeof *vq->used +
+					   vq->num * sizeof *vq->used->ring)) {
+				r = -EINVAL;
+				break;
+			}
+		}
+
 		r = init_used(vq, (struct vring_used __user *)(unsigned long)
 			      a.used_user_addr);
 		if (r)
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index d1f0453..44591ba 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -117,6 +117,8 @@ long vhost_dev_check_owner(struct vhost_dev *);
 long vhost_dev_reset_owner(struct vhost_dev *);
 void vhost_dev_cleanup(struct vhost_dev *);
 long vhost_dev_ioctl(struct vhost_dev *, unsigned int ioctl, unsigned long arg);
+int vhost_vq_access_ok(struct vhost_virtqueue *vq);
+int vhost_log_access_ok(struct vhost_dev *);
 
 unsigned vhost_get_vq_desc(struct vhost_dev *, struct vhost_virtqueue *,
 			   struct iovec iov[], unsigned int iov_count,
-- 
1.6.6.rc1.43.gf55cc

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/