* Re: [PATCH v2 0/2] vhost: Skip access checks on GIOVAs [not found] <160139701999.162128.2399875915342200263.stgit@bahia.lan> @ 2020-10-01 12:46 ` Michael S. Tsirkin [not found] ` <160139703153.162128.16860679176471296230.stgit@bahia.lan> [not found] ` <160139704424.162128.7839027287942194310.stgit@bahia.lan> 2 siblings, 0 replies; 3+ messages in thread From: Michael S. Tsirkin @ 2020-10-01 12:46 UTC (permalink / raw) To: Greg Kurz Cc: kvm, netdev, Laurent Vivier, qemu-devel, virtualization, David Gibson 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 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 3+ messages in thread
[parent not found: <160139703153.162128.16860679176471296230.stgit@bahia.lan>]
* Re: [PATCH v2 1/2] vhost: Don't call access_ok() when using IOTLB [not found] ` <160139703153.162128.16860679176471296230.stgit@bahia.lan> @ 2020-10-03 1:51 ` Jason Wang 0 siblings, 0 replies; 3+ messages in thread From: Jason Wang @ 2020-10-03 1:51 UTC (permalink / raw) To: Greg Kurz, Michael S. Tsirkin Cc: kvm, netdev, Laurent Vivier, qemu-devel, virtualization, David Gibson 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 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 3+ messages in thread
[parent not found: <160139704424.162128.7839027287942194310.stgit@bahia.lan>]
* Re: [PATCH v2 2/2] vhost: Don't call log_access_ok() when using IOTLB [not found] ` <160139704424.162128.7839027287942194310.stgit@bahia.lan> @ 2020-10-03 1:58 ` Jason Wang 0 siblings, 0 replies; 3+ messages in thread From: Jason Wang @ 2020-10-03 1:58 UTC (permalink / raw) To: Greg Kurz, Michael S. Tsirkin Cc: kvm, netdev, Laurent Vivier, qemu-devel, virtualization, David Gibson On 2020/9/30 上午12:30, Greg Kurz wrote: > When the IOTLB device is enabled, the log_guest_addr that is passed by > userspace to the VHOST_SET_VRING_ADDR ioctl, and which is then written > to vq->log_addr, is a GIOVA. All writes to this address are translated > by log_user() to writes to an HVA, and then ultimately logged through > the corresponding GPAs in log_write_hva(). No logging will ever occur > with vq->log_addr in this case. It is thus wrong to pass vq->log_addr > and log_guest_addr to log_access_vq() which assumes they are actual > GPAs. > > Introduce a new vq_log_used_access_ok() helper that only checks accesses > to the log for the used structure when there isn't an IOTLB device around. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > drivers/vhost/vhost.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index c3b49975dc28..5996e32fa818 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -1370,6 +1370,20 @@ bool vhost_log_access_ok(struct vhost_dev *dev) > } > EXPORT_SYMBOL_GPL(vhost_log_access_ok); > > +static bool vq_log_used_access_ok(struct vhost_virtqueue *vq, > + void __user *log_base, > + bool log_used, > + u64 log_addr, > + size_t log_size) > +{ > + /* If an IOTLB device is present, log_addr is a GIOVA that > + * will never be logged by log_used(). */ > + if (vq->iotlb) > + return true; > + > + return !log_used || log_access_ok(log_base, log_addr, log_size); > +} > + > /* Verify access for write logging. */ > /* Caller should have vq mutex and device mutex */ > static bool vq_log_access_ok(struct vhost_virtqueue *vq, > @@ -1377,8 +1391,8 @@ static bool vq_log_access_ok(struct vhost_virtqueue *vq, > { > return vq_memory_access_ok(log_base, vq->umem, > vhost_has_feature(vq, VHOST_F_LOG_ALL)) && > - (!vq->log_used || log_access_ok(log_base, vq->log_addr, > - vhost_get_used_size(vq, vq->num))); > + vq_log_used_access_ok(vq, log_base, vq->log_used, vq->log_addr, > + vhost_get_used_size(vq, vq->num)); > } > > /* Can we start vq? */ > @@ -1517,8 +1531,9 @@ static long vhost_vring_set_addr(struct vhost_dev *d, > return -EINVAL; > > /* 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, > + if (!vq_log_used_access_ok(vq, vq->log_base, > + a.flags & (0x1 << VHOST_VRING_F_LOG), > + a.log_guest_addr, > sizeof *vq->used + > vq->num * sizeof *vq->used->ring)) It looks to me that we should use vhost_get_used_size() which takes event into account. Any reason that we can't reuse vq_log_access_ok() here? Thanks > return -EINVAL; > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-03 1:59 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <160139701999.162128.2399875915342200263.stgit@bahia.lan>
2020-10-01 12:46 ` [PATCH v2 0/2] vhost: Skip access checks on GIOVAs Michael S. Tsirkin
[not found] ` <160139703153.162128.16860679176471296230.stgit@bahia.lan>
2020-10-03 1:51 ` [PATCH v2 1/2] vhost: Don't call access_ok() when using IOTLB Jason Wang
[not found] ` <160139704424.162128.7839027287942194310.stgit@bahia.lan>
2020-10-03 1:58 ` [PATCH v2 2/2] vhost: Don't call log_access_ok() " Jason Wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).