Skip to content

Commit 949989b

Browse files
davem330davem330
committed
Merge branch 'vhost-fix-vhost_vq_access_ok-log-check'
Stefan Hajnoczi says: ==================== vhost: fix vhost_vq_access_ok() log check v3: * Rebased onto net/master and resolved conflict [DaveM] v2: * Rewrote the conditional to make the vq access check clearer [Linus] * Added Patch 2 to make the return type consistent and harder to misuse [Linus] The first patch fixes the vhost virtqueue access check which was recently broken. The second patch replaces the int return type with bool to prevent future bugs. ==================== Acked-by: Jason Wang <[email protected]> Acked-by: Michael S. Tsirkin <[email protected]> Signed-off-by: David S. Miller <[email protected]>
2 parents 7ced6c9 + ddd3d40 commit 949989b

File tree

2 files changed

+38
-36
lines changed

2 files changed

+38
-36
lines changed

‎drivers/vhost/vhost.c

Lines changed: 36 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -641,14 +641,14 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
641641
}
642642
EXPORT_SYMBOL_GPL(vhost_dev_cleanup);
643643

644-
static int log_access_ok(void __user *log_base, u64 addr, unsigned long sz)
644+
static bool log_access_ok(void __user *log_base, u64 addr, unsigned long sz)
645645
{
646646
u64 a = addr / VHOST_PAGE_SIZE / 8;
647647

648648
/* Make sure 64 bit math will not overflow. */
649649
if (a > ULONG_MAX - (unsigned long)log_base ||
650650
a + (unsigned long)log_base > ULONG_MAX)
651-
return 0;
651+
return false;
652652

653653
return access_ok(VERIFY_WRITE, log_base + a,
654654
(sz + VHOST_PAGE_SIZE * 8 - 1) / VHOST_PAGE_SIZE / 8);
@@ -661,30 +661,30 @@ static bool vhost_overflow(u64 uaddr, u64 size)
661661
}
662662

663663
/* Caller should have vq mutex and device mutex. */
664-
static int vq_memory_access_ok(void __user *log_base, struct vhost_umem *umem,
665-
int log_all)
664+
static bool vq_memory_access_ok(void __user *log_base, struct vhost_umem *umem,
665+
int log_all)
666666
{
667667
struct vhost_umem_node *node;
668668

669669
if (!umem)
670-
return 0;
670+
return false;
671671

672672
list_for_each_entry(node, &umem->umem_list, link) {
673673
unsigned long a = node->userspace_addr;
674674

675675
if (vhost_overflow(node->userspace_addr, node->size))
676-
return 0;
676+
return false;
677677

678678

679679
if (!access_ok(VERIFY_WRITE, (void __user *)a,
680680
node->size))
681-
return 0;
681+
return false;
682682
else if (log_all && !log_access_ok(log_base,
683683
node->start,
684684
node->size))
685-
return 0;
685+
return false;
686686
}
687-
return 1;
687+
return true;
688688
}
689689

690690
static inline void __user *vhost_vq_meta_fetch(struct vhost_virtqueue *vq,
@@ -701,13 +701,13 @@ static inline void __user *vhost_vq_meta_fetch(struct vhost_virtqueue *vq,
701701

702702
/* Can we switch to this memory table? */
703703
/* Caller should have device mutex but not vq mutex */
704-
static int memory_access_ok(struct vhost_dev *d, struct vhost_umem *umem,
705-
int log_all)
704+
static bool memory_access_ok(struct vhost_dev *d, struct vhost_umem *umem,
705+
int log_all)
706706
{
707707
int i;
708708

709709
for (i = 0; i < d->nvqs; ++i) {
710-
int ok;
710+
bool ok;
711711
bool log;
712712

713713
mutex_lock(&d->vqs[i]->mutex);
@@ -717,12 +717,12 @@ static int memory_access_ok(struct vhost_dev *d, struct vhost_umem *umem,
717717
ok = vq_memory_access_ok(d->vqs[i]->log_base,
718718
umem, log);
719719
else
720-
ok = 1;
720+
ok = true;
721721
mutex_unlock(&d->vqs[i]->mutex);
722722
if (!ok)
723-
return 0;
723+
return false;
724724
}
725-
return 1;
725+
return true;
726726
}
727727

728728
static int translate_desc(struct vhost_virtqueue *vq, u64 addr, u32 len,
@@ -959,21 +959,21 @@ static void vhost_iotlb_notify_vq(struct vhost_dev *d,
959959
spin_unlock(&d->iotlb_lock);
960960
}
961961

962-
static int umem_access_ok(u64 uaddr, u64 size, int access)
962+
static bool umem_access_ok(u64 uaddr, u64 size, int access)
963963
{
964964
unsigned long a = uaddr;
965965

966966
/* Make sure 64 bit math will not overflow. */
967967
if (vhost_overflow(uaddr, size))
968-
return -EFAULT;
968+
return false;
969969

970970
if ((access & VHOST_ACCESS_RO) &&
971971
!access_ok(VERIFY_READ, (void __user *)a, size))
972-
return -EFAULT;
972+
return false;
973973
if ((access & VHOST_ACCESS_WO) &&
974974
!access_ok(VERIFY_WRITE, (void __user *)a, size))
975-
return -EFAULT;
976-
return 0;
975+
return false;
976+
return true;
977977
}
978978

979979
static int vhost_process_iotlb_msg(struct vhost_dev *dev,
@@ -988,7 +988,7 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
988988
ret = -EFAULT;
989989
break;
990990
}
991-
if (umem_access_ok(msg->uaddr, msg->size, msg->perm)) {
991+
if (!umem_access_ok(msg->uaddr, msg->size, msg->perm)) {
992992
ret = -EFAULT;
993993
break;
994994
}
@@ -1135,10 +1135,10 @@ static int vhost_iotlb_miss(struct vhost_virtqueue *vq, u64 iova, int access)
11351135
return 0;
11361136
}
11371137

1138-
static int vq_access_ok(struct vhost_virtqueue *vq, unsigned int num,
1139-
struct vring_desc __user *desc,
1140-
struct vring_avail __user *avail,
1141-
struct vring_used __user *used)
1138+
static bool vq_access_ok(struct vhost_virtqueue *vq, unsigned int num,
1139+
struct vring_desc __user *desc,
1140+
struct vring_avail __user *avail,
1141+
struct vring_used __user *used)
11421142

11431143
{
11441144
size_t s = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
@@ -1161,8 +1161,8 @@ static void vhost_vq_meta_update(struct vhost_virtqueue *vq,
11611161
vq->meta_iotlb[type] = node;
11621162
}
11631163

1164-
static int iotlb_access_ok(struct vhost_virtqueue *vq,
1165-
int access, u64 addr, u64 len, int type)
1164+
static bool iotlb_access_ok(struct vhost_virtqueue *vq,
1165+
int access, u64 addr, u64 len, int type)
11661166
{
11671167
const struct vhost_umem_node *node;
11681168
struct vhost_umem *umem = vq->iotlb;
@@ -1220,16 +1220,16 @@ EXPORT_SYMBOL_GPL(vq_iotlb_prefetch);
12201220

12211221
/* Can we log writes? */
12221222
/* Caller should have device mutex but not vq mutex */
1223-
int vhost_log_access_ok(struct vhost_dev *dev)
1223+
bool vhost_log_access_ok(struct vhost_dev *dev)
12241224
{
12251225
return memory_access_ok(dev, dev->umem, 1);
12261226
}
12271227
EXPORT_SYMBOL_GPL(vhost_log_access_ok);
12281228

12291229
/* Verify access for write logging. */
12301230
/* Caller should have vq mutex and device mutex */
1231-
static int vq_log_access_ok(struct vhost_virtqueue *vq,
1232-
void __user *log_base)
1231+
static bool vq_log_access_ok(struct vhost_virtqueue *vq,
1232+
void __user *log_base)
12331233
{
12341234
size_t s = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
12351235

@@ -1242,12 +1242,14 @@ static int vq_log_access_ok(struct vhost_virtqueue *vq,
12421242

12431243
/* Can we start vq? */
12441244
/* Caller should have vq mutex and device mutex */
1245-
int vhost_vq_access_ok(struct vhost_virtqueue *vq)
1245+
bool vhost_vq_access_ok(struct vhost_virtqueue *vq)
12461246
{
1247-
int ret = vq_log_access_ok(vq, vq->log_base);
1247+
if (!vq_log_access_ok(vq, vq->log_base))
1248+
return false;
12481249

1249-
if (ret || vq->iotlb)
1250-
return ret;
1250+
/* Access validation occurs at prefetch time with IOTLB */
1251+
if (vq->iotlb)
1252+
return true;
12511253

12521254
return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used);
12531255
}

‎drivers/vhost/vhost.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,8 +178,8 @@ void vhost_dev_cleanup(struct vhost_dev *);
178178
void vhost_dev_stop(struct vhost_dev *);
179179
long vhost_dev_ioctl(struct vhost_dev *, unsigned int ioctl, void __user *argp);
180180
long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp);
181-
int vhost_vq_access_ok(struct vhost_virtqueue *vq);
182-
int vhost_log_access_ok(struct vhost_dev *);
181+
bool vhost_vq_access_ok(struct vhost_virtqueue *vq);
182+
bool vhost_log_access_ok(struct vhost_dev *);
183183

184184
int vhost_get_vq_desc(struct vhost_virtqueue *,
185185
struct iovec iov[], unsigned int iov_count,

0 commit comments

Comments
 (0)