vhost: prevent modification of an active ring

PATCH New Michael S. Tsirkin 1 participant 5 messages Dec 20, 2009 1 message not in the archive

Subsystems (2)

virtio core virtio host (vhost)

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Michael S. Tsirkin

author maintainer

2009-12-20 17:16 UTC #1 lore raw

PATCH 1/3 vhost: prevent modification of an active ring

We don't support changing ring size or log base while ring is running. If user violates these rules, he might get his memory silently corrupted. It's better to be explicit, and fail such modification attempts with an error.
To make these "vq running" checks in ioctl context robust, this patch also moves all vq flushes to under device mutex.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/vhost/net.c   |    6 +++---
 drivers/vhost/vhost.c |   29 ++++++++++++++++++++++++-----
 2 files changed, 27 insertions(+), 8 deletions(-)

drivers/vhost/net.c +3 -3

index d6db10c..1b509a0 100644
@@ -509,12 +509,10 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
 	vhost_net_enable_vq(n, vq);
 	mutex_unlock(&vq->mutex);
 done:
-	mutex_unlock(&n->dev.mutex);
 	if (oldsock) {
 		vhost_net_flush_vq(n, index);
 		fput(oldsock->file);
 	}
-	return r;
 err:
 	mutex_unlock(&n->dev.mutex);
 	return r;

@@ -554,8 +552,8 @@ static void vhost_net_set_features(struct vhost_net *n, u64 features)
 		n->vqs[i].hdr_size = hdr_size;
 		mutex_unlock(&n->vqs[i].mutex);
 	}
-	mutex_unlock(&n->dev.mutex);
 	vhost_net_flush(n);
+	mutex_unlock(&n->dev.mutex);
 }
 
 static long vhost_net_ioctl(struct file *f, unsigned int ioctl,

@@ -587,8 +585,10 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
 	case VHOST_RESET_OWNER:
 		return vhost_net_reset_owner(n);
 	default:
+		mutex_lock(&n->dev.mutex);
 		r = vhost_dev_ioctl(&n->dev, ioctl, arg);
 		vhost_net_flush(n);
+		mutex_unlock(&n->dev.mutex);
 		return r;
 	}
 }

drivers/vhost/vhost.c +24 -5

index e7b4dea..29f1675 100644
@@ -288,6 +288,12 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
 
 	switch (ioctl) {
 	case VHOST_SET_VRING_NUM:
+		/* Resizing ring with an active backend?
+		 * You don't want to do that. */
+		if (vq->private_data) {
+			r = -EBUSY;
+			break;
+		}
 		r = copy_from_user(&s, argp, sizeof s);
 		if (r < 0)
 			break;

@@ -298,6 +304,12 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
 		vq->num = s.num;
 		break;
 	case VHOST_SET_VRING_BASE:
+		/* Moving base with an active backend?
+		 * You don't want to do that. */
+		if (vq->private_data) {
+			r = -EBUSY;
+			break;
+		}
 		r = copy_from_user(&s, argp, sizeof s);
 		if (r < 0)
 			break;

@@ -413,6 +425,7 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
 	return r;
 }
 
+/* Caller must have device mutex */
 long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, unsigned long arg)
 {
 	void __user *argp = (void __user *)arg;

@@ -422,7 +435,6 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, unsigned long arg)
 	long r;
 	int i, fd;
 
-	mutex_lock(&d->mutex);
 	/* If you are not the owner, you can become one */
 	if (ioctl == VHOST_SET_OWNER) {
 		r = vhost_dev_set_owner(d);

@@ -447,9 +459,17 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, unsigned long arg)
 			break;
 		}
 		for (i = 0; i < d->nvqs; ++i) {
-			mutex_lock(&d->vqs[i].mutex);
-			d->vqs[i].log_base = (void __user *)(unsigned long)p;
-			mutex_unlock(&d->vqs[i].mutex);
+			struct vhost_virtqueue *vq;
+			vq = d->vqs + i;
+			mutex_lock(&vq->mutex);
+			/* Moving log base with an active backend?
+			 * You don't want to do that. */
+			if (vq->private_data && (vq->log_used ||
+			     vhost_has_feature(d, VHOST_F_LOG_ALL)))
+				r = -EBUSY;
+			else
+				vq->log_base = (void __user *)(unsigned long)p;
+			mutex_unlock(&vq->mutex);
 		}
 		break;
 	case VHOST_SET_LOG_FD:

@@ -483,7 +503,6 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, unsigned long arg)
 		break;
 	}
 done:
-	mutex_unlock(&d->mutex);
 	return r;
 }

Signature · 1 line

1.6.6.rc1.43.gf55cc

Michael S. Tsirkin

author maintainer

2009-12-20 17:19 UTC #2 lore raw

PATCH 1/3 vhost: prevent modification of an active ring

In reply to a message not in the archive

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/vhost/net.c   |    6 +++---
 drivers/vhost/vhost.c |   29 ++++++++++++++++++++++++-----
 2 files changed, 27 insertions(+), 8 deletions(-)

drivers/vhost/net.c +3 -3

index d6db10c..1b509a0 100644
@@ -509,12 +509,10 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
 	vhost_net_enable_vq(n, vq);
 	mutex_unlock(&vq->mutex);
 done:
-	mutex_unlock(&n->dev.mutex);
 	if (oldsock) {
 		vhost_net_flush_vq(n, index);
 		fput(oldsock->file);
 	}
-	return r;
 err:
 	mutex_unlock(&n->dev.mutex);
 	return r;

@@ -554,8 +552,8 @@ static void vhost_net_set_features(struct vhost_net *n, u64 features)
 		n->vqs[i].hdr_size = hdr_size;
 		mutex_unlock(&n->vqs[i].mutex);
 	}
-	mutex_unlock(&n->dev.mutex);
 	vhost_net_flush(n);
+	mutex_unlock(&n->dev.mutex);
 }
 
 static long vhost_net_ioctl(struct file *f, unsigned int ioctl,

@@ -587,8 +585,10 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
 	case VHOST_RESET_OWNER:
 		return vhost_net_reset_owner(n);
 	default:
+		mutex_lock(&n->dev.mutex);
 		r = vhost_dev_ioctl(&n->dev, ioctl, arg);
 		vhost_net_flush(n);
+		mutex_unlock(&n->dev.mutex);
 		return r;
 	}
 }

drivers/vhost/vhost.c +24 -5

index e7b4dea..29f1675 100644
@@ -288,6 +288,12 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
 
 	switch (ioctl) {
 	case VHOST_SET_VRING_NUM:
+		/* Resizing ring with an active backend?
+		 * You don't want to do that. */
+		if (vq->private_data) {
+			r = -EBUSY;
+			break;
+		}
 		r = copy_from_user(&s, argp, sizeof s);
 		if (r < 0)
 			break;

@@ -298,6 +304,12 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
 		vq->num = s.num;
 		break;
 	case VHOST_SET_VRING_BASE:
+		/* Moving base with an active backend?
+		 * You don't want to do that. */
+		if (vq->private_data) {
+			r = -EBUSY;
+			break;
+		}
 		r = copy_from_user(&s, argp, sizeof s);
 		if (r < 0)
 			break;

@@ -413,6 +425,7 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
 	return r;
 }
 
+/* Caller must have device mutex */
 long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, unsigned long arg)
 {
 	void __user *argp = (void __user *)arg;

@@ -422,7 +435,6 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, unsigned long arg)
 	long r;
 	int i, fd;
 
-	mutex_lock(&d->mutex);
 	/* If you are not the owner, you can become one */
 	if (ioctl == VHOST_SET_OWNER) {
 		r = vhost_dev_set_owner(d);

@@ -447,9 +459,17 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, unsigned long arg)
 			break;
 		}
 		for (i = 0; i < d->nvqs; ++i) {
-			mutex_lock(&d->vqs[i].mutex);
-			d->vqs[i].log_base = (void __user *)(unsigned long)p;
-			mutex_unlock(&d->vqs[i].mutex);
+			struct vhost_virtqueue *vq;
+			vq = d->vqs + i;
+			mutex_lock(&vq->mutex);
+			/* Moving log base with an active backend?
+			 * You don't want to do that. */
+			if (vq->private_data && (vq->log_used ||
+			     vhost_has_feature(d, VHOST_F_LOG_ALL)))
+				r = -EBUSY;
+			else
+				vq->log_base = (void __user *)(unsigned long)p;
+			mutex_unlock(&vq->mutex);
 		}
 		break;
 	case VHOST_SET_LOG_FD:

@@ -483,7 +503,6 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, unsigned long arg)
 		break;
 	}
 done:
-	mutex_unlock(&d->mutex);
 	return r;
 }

Signature · 1 line

1.6.6.rc1.43.gf55cc

Michael S. Tsirkin

author maintainer

2009-12-20 17:16 UTC #3 lore raw

PATCH 2/3 vhost: add access_ok checks

In reply to a message not in the archive

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 <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/vhost/net.c   |   17 ++++++-
 drivers/vhost/vhost.c |  111 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/vhost/vhost.h |    2 +
 3 files changed, 127 insertions(+), 3 deletions(-)

drivers/vhost/net.c +14 -3

index 1b509a0..1aacd8c 100644
@@ -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:

drivers/vhost/vhost.c +111 -0

129 lines not on this page · raw message

drivers/vhost/vhost.h +2 -0

index d1f0453..44591ba 100644
@@ -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,

Signature · 1 line

1.6.6.rc1.43.gf55cc

Michael S. Tsirkin

author maintainer

2009-12-20 17:19 UTC #4 lore raw

PATCH 2/3 vhost: add access_ok checks

In reply to a message not in the archive

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 <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/vhost/net.c   |   17 ++++++-
 drivers/vhost/vhost.c |  111 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/vhost/vhost.h |    2 +
 3 files changed, 127 insertions(+), 3 deletions(-)

drivers/vhost/net.c +14 -3

index 1b509a0..1aacd8c 100644
@@ -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:

drivers/vhost/vhost.c +111 -0

129 lines not on this page · raw message

drivers/vhost/vhost.h +2 -0

index d1f0453..44591ba 100644
@@ -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,

Signature · 1 line

1.6.6.rc1.43.gf55cc

Michael S. Tsirkin

author maintainer

2009-12-20 17:16 UTC #5 lore raw

PATCH 3/3 vhost: make default mapping empty by default

In reply to a message not in the archive

vhost now validates each region with access_ok in calling process context before access. Since this fails on a full 64 bit 1:1 mapping that vhost had by default, we can't support such a mapping: users will have to set up a table with real addresses that actually matches their address space.
Make the default mapping empty.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/vhost/vhost.c |   13 +++----------
 1 files changed, 3 insertions(+), 10 deletions(-)

drivers/vhost/vhost.c +3 ~~-10~~

index 33e06bf..2b65d9b 100644
@@ -170,21 +170,14 @@ long vhost_dev_reset_owner(struct vhost_dev *dev)
 {
 	struct vhost_memory *memory;
 
-	/* Restore memory to default 1:1 mapping. */
-	memory = kmalloc(offsetof(struct vhost_memory, regions) +
-			 2 * sizeof *memory->regions, GFP_KERNEL);
+	/* Restore memory to default empty mapping. */
+	memory = kmalloc(offsetof(struct vhost_memory, regions), GFP_KERNEL);
 	if (!memory)
 		return -ENOMEM;
 
 	vhost_dev_cleanup(dev);
 
-	memory->nregions = 2;
-	memory->regions[0].guest_phys_addr = 1;
-	memory->regions[0].userspace_addr = 1;
-	memory->regions[0].memory_size = ~0ULL;
-	memory->regions[1].guest_phys_addr = 0;
-	memory->regions[1].userspace_addr = 0;
-	memory->regions[1].memory_size = 1;
+	memory->nregions = 0;
 	dev->memory = memory;
 	return 0;
 }

Signature · 1 line

1.6.6.rc1.43.gf55cc