| Message ID | 160139704424.162128.7839027287942194310.stgit@bahia.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vhost: Skip access checks on GIOVAs | expand |
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; > >
On Sat, 3 Oct 2020 09:58:59 +0800 Jason Wang <jasowang@redhat.com> wrote: > > 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? > No reason indeed but I'll fix this in a preliminary patch, and send a v2 shortly. Cheers, -- Greg > Thanks > > > > return -EINVAL; > > > > >
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)) return -EINVAL;
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(-)