| Message ID | 160139701999.162128.2399875915342200263.stgit@bahia.lan |
|---|---|
| Headers | show |
| Series | vhost: Skip access checks on GIOVAs | expand |
On Tue, Sep 29, 2020 at 06:30:20PM +0200, Greg Kurz wrote: > This series addresses some misuse around vring addresses provided by > userspace when using an IOTLB device. The misuse cause failures of > the VHOST_SET_VRING_ADDR ioctl on POWER, which in turn causes QEMU > to crash at migration time. > > While digging some more I realized that log_access_ok() can also be > passed a GIOVA (vq->log_addr) even though log_used() will never log > anything at that address. I could observe addresses beyond the end > of the log bitmap being passed to access_ok(), but it didn't have any > impact because the addresses were still acceptable from an access_ok() > standpoint. Adding a second patch to fix that anyway. > > Note that I've also posted a patch for QEMU so that it skips the used > structure GIOVA when allocating the log bitmap. Otherwise QEMU fails to > allocate it because POWER puts GIOVAs very high in the address space (ie. > over 0x800000000000000ULL). > > https://patchwork.ozlabs.org/project/qemu-devel/patch/160105498386.68108.2145229309875282336.stgit@bahia.lan/ I queued this. Jason, can you ack please? > v2: > - patch 1: move the (vq->ioltb) check from vhost_vq_access_ok() to > vq_access_ok() as suggested by MST > - patch 2: new patch > > --- > > Greg Kurz (2): > vhost: Don't call access_ok() when using IOTLB > vhost: Don't call log_access_ok() when using IOTLB > > > drivers/vhost/vhost.c | 32 ++++++++++++++++++++++++-------- > 1 file changed, 24 insertions(+), 8 deletions(-) > > -- > Greg
On 2020/9/30 上午12:30, Greg Kurz wrote: > When the IOTLB device is enabled, the vring addresses we get > from userspace are GIOVAs. It is thus wrong to pass them down > to access_ok() which only takes HVAs. > > Access validation is done at prefetch time with IOTLB. Teach > vq_access_ok() about that by moving the (vq->iotlb) check > from vhost_vq_access_ok() to vq_access_ok(). This prevents > vhost_vring_set_addr() to fail when verifying the accesses. > No behavior change for vhost_vq_access_ok(). > > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1883084 > Fixes: 6b1e6cc7855b ("vhost: new device IOTLB API") > Cc: jasowang@redhat.com > CC: stable@vger.kernel.org # 4.14+ > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > drivers/vhost/vhost.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index b45519ca66a7..c3b49975dc28 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -1290,6 +1290,11 @@ static bool vq_access_ok(struct vhost_virtqueue *vq, unsigned int num, > vring_used_t __user *used) > > { > + /* If an IOTLB device is present, the vring addresses are > + * GIOVAs. Access validation occurs at prefetch time. */ > + if (vq->iotlb) > + return true; > + > return access_ok(desc, vhost_get_desc_size(vq, num)) && > access_ok(avail, vhost_get_avail_size(vq, num)) && > access_ok(used, vhost_get_used_size(vq, num)); > @@ -1383,10 +1388,6 @@ bool vhost_vq_access_ok(struct vhost_virtqueue *vq) > if (!vq_log_access_ok(vq, vq->log_base)) > return false; > > - /* Access validation occurs at prefetch time with IOTLB */ > - if (vq->iotlb) > - return true; > - > return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used); > } > EXPORT_SYMBOL_GPL(vhost_vq_access_ok); > Acked-by: Jason Wang <jasowang@redhat.com> Thanks
This series addresses some misuse around vring addresses provided by userspace when using an IOTLB device. The misuse cause failures of the VHOST_SET_VRING_ADDR ioctl on POWER, which in turn causes QEMU to crash at migration time. While digging some more I realized that log_access_ok() can also be passed a GIOVA (vq->log_addr) even though log_used() will never log anything at that address. I could observe addresses beyond the end of the log bitmap being passed to access_ok(), but it didn't have any impact because the addresses were still acceptable from an access_ok() standpoint. Adding a second patch to fix that anyway. Note that I've also posted a patch for QEMU so that it skips the used structure GIOVA when allocating the log bitmap. Otherwise QEMU fails to allocate it because POWER puts GIOVAs very high in the address space (ie. over 0x800000000000000ULL). https://patchwork.ozlabs.org/project/qemu-devel/patch/160105498386.68108.2145229309875282336.stgit@bahia.lan/ v2: - patch 1: move the (vq->ioltb) check from vhost_vq_access_ok() to vq_access_ok() as suggested by MST - patch 2: new patch --- Greg Kurz (2): vhost: Don't call access_ok() when using IOTLB vhost: Don't call log_access_ok() when using IOTLB drivers/vhost/vhost.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) -- Greg