| Message ID | 1484625660-3312-1-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jan 17, 2017 at 12:01:00PM +0800, Jason Wang wrote: > We allow vhost to clear VIRITO_F_IOMMU_PLATFORM which is wrong since > VIRTIO_F_IOMMU_PLATFORM is mandatory for security. Fixing this by > enforce it after vdc->get_features(). > > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > hw/virtio/virtio-bus.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c > index d31cc00..a886011 100644 > --- a/hw/virtio/virtio-bus.c > +++ b/hw/virtio/virtio-bus.c > @@ -47,6 +47,7 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) > VirtioBusState *bus = VIRTIO_BUS(qbus); > VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus); > VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); > + bool has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM); > > DPRINTF("%s: plug device.\n", qbus->name); > > @@ -63,8 +64,8 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) > klass->device_plugged(qbus->parent, errp); > } > > - if (klass->get_dma_as != NULL && > - virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) { > + if (klass->get_dma_as != NULL && has_iommu) { > + virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM); > vdev->dma_as = klass->get_dma_as(qbus->parent); > } else { > vdev->dma_as = &address_space_memory; I suspect that's not enough, we must also fail or disable vhost (depending on the options), otherwise things won't work. > -- > 2.7.4
On 2017年01月17日 22:44, Michael S. Tsirkin wrote: > On Tue, Jan 17, 2017 at 12:01:00PM +0800, Jason Wang wrote: >> We allow vhost to clear VIRITO_F_IOMMU_PLATFORM which is wrong since >> VIRTIO_F_IOMMU_PLATFORM is mandatory for security. Fixing this by >> enforce it after vdc->get_features(). >> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> hw/virtio/virtio-bus.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c >> index d31cc00..a886011 100644 >> --- a/hw/virtio/virtio-bus.c >> +++ b/hw/virtio/virtio-bus.c >> @@ -47,6 +47,7 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) >> VirtioBusState *bus = VIRTIO_BUS(qbus); >> VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus); >> VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); >> + bool has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM); >> >> DPRINTF("%s: plug device.\n", qbus->name); >> >> @@ -63,8 +64,8 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) >> klass->device_plugged(qbus->parent, errp); >> } >> >> - if (klass->get_dma_as != NULL && >> - virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) { >> + if (klass->get_dma_as != NULL && has_iommu) { >> + virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM); >> vdev->dma_as = klass->get_dma_as(qbus->parent); >> } else { >> vdev->dma_as = &address_space_memory; > I suspect that's not enough, we must also fail or disable vhost > (depending on the options), otherwise things won't work. Looks like with the patch, VIRTIO_F_IOMMU_PLATFORM will be passed to vhost_set_features(). So if vhost backend does not support it, it will fall back to userspace (This may not work for vhost-user, but it's a bug existed even before this patch). Thanks
On Wed, Jan 18, 2017 at 10:42:48AM +0800, Jason Wang wrote: > > > On 2017年01月17日 22:44, Michael S. Tsirkin wrote: > > On Tue, Jan 17, 2017 at 12:01:00PM +0800, Jason Wang wrote: > > > We allow vhost to clear VIRITO_F_IOMMU_PLATFORM which is wrong since > > > VIRTIO_F_IOMMU_PLATFORM is mandatory for security. Fixing this by > > > enforce it after vdc->get_features(). > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > --- > > > hw/virtio/virtio-bus.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c > > > index d31cc00..a886011 100644 > > > --- a/hw/virtio/virtio-bus.c > > > +++ b/hw/virtio/virtio-bus.c > > > @@ -47,6 +47,7 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) > > > VirtioBusState *bus = VIRTIO_BUS(qbus); > > > VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus); > > > VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); > > > + bool has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM); > > > DPRINTF("%s: plug device.\n", qbus->name); > > > @@ -63,8 +64,8 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) > > > klass->device_plugged(qbus->parent, errp); > > > } > > > - if (klass->get_dma_as != NULL && > > > - virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) { > > > + if (klass->get_dma_as != NULL && has_iommu) { > > > + virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM); > > > vdev->dma_as = klass->get_dma_as(qbus->parent); > > > } else { > > > vdev->dma_as = &address_space_memory; > > I suspect that's not enough, we must also fail or disable vhost > > (depending on the options), otherwise things won't work. > > Looks like with the patch, VIRTIO_F_IOMMU_PLATFORM will be passed to > vhost_set_features(). Okay then. Could you please test on an old host kernel and confirm what happens? Pls mention this in commit log. > So if vhost backend does not support it, it will fall > back to userspace (This may not work for vhost-user, but it's a bug existed > even before this patch). > > Thanks I guess this is true but this suddenly makes it important to fix this. So I prefer a patchset where patch 2 addresses the fallback bug. In which way? Do you think we should just fail if vhost doesn't work? Does this apply to both userspace and kernel?
On 2017年01月19日 01:50, Michael S. Tsirkin wrote: > On Wed, Jan 18, 2017 at 10:42:48AM +0800, Jason Wang wrote: >> >> On 2017年01月17日 22:44, Michael S. Tsirkin wrote: >>> On Tue, Jan 17, 2017 at 12:01:00PM +0800, Jason Wang wrote: >>>> We allow vhost to clear VIRITO_F_IOMMU_PLATFORM which is wrong since >>>> VIRTIO_F_IOMMU_PLATFORM is mandatory for security. Fixing this by >>>> enforce it after vdc->get_features(). >>>> >>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>>> --- >>>> hw/virtio/virtio-bus.c | 5 +++-- >>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c >>>> index d31cc00..a886011 100644 >>>> --- a/hw/virtio/virtio-bus.c >>>> +++ b/hw/virtio/virtio-bus.c >>>> @@ -47,6 +47,7 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) >>>> VirtioBusState *bus = VIRTIO_BUS(qbus); >>>> VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus); >>>> VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); >>>> + bool has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM); >>>> DPRINTF("%s: plug device.\n", qbus->name); >>>> @@ -63,8 +64,8 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) >>>> klass->device_plugged(qbus->parent, errp); >>>> } >>>> - if (klass->get_dma_as != NULL && >>>> - virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) { >>>> + if (klass->get_dma_as != NULL && has_iommu) { >>>> + virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM); >>>> vdev->dma_as = klass->get_dma_as(qbus->parent); >>>> } else { >>>> vdev->dma_as = &address_space_memory; >>> I suspect that's not enough, we must also fail or disable vhost >>> (depending on the options), otherwise things won't work. >> Looks like with the patch, VIRTIO_F_IOMMU_PLATFORM will be passed to >> vhost_set_features(). > Okay then. Could you please test on an old host kernel and confirm > what happens? Pls mention this in commit log. Test were done before sending this patch :) Vhost fallback to userspace since it lacks VIRTIO_F_IOMMU_PLATFORM. > >> So if vhost backend does not support it, it will fall >> back to userspace (This may not work for vhost-user, but it's a bug existed >> even before this patch). >> >> Thanks > I guess this is true but this suddenly makes it important to > fix this. So I prefer a patchset where patch 2 addresses > the fallback bug. In which way? Do you think we should > just fail if vhost doesn't work? Does this apply to both > userspace and kernel? For 1.0 devices we could do this through NEEDS_RESET, but it doesn't work for legacy device. For kernel vhost, I'd prefer to keep the current fallback. For vhost-user, it seems not easy and we could not just terminate qemu in this case consider this kind of failure can only be detected during start not init. Thanks
On Thu, Jan 19, 2017 at 11:12:21AM +0800, Jason Wang wrote: > > > On 2017年01月19日 01:50, Michael S. Tsirkin wrote: > > On Wed, Jan 18, 2017 at 10:42:48AM +0800, Jason Wang wrote: > > > > > > On 2017年01月17日 22:44, Michael S. Tsirkin wrote: > > > > On Tue, Jan 17, 2017 at 12:01:00PM +0800, Jason Wang wrote: > > > > > We allow vhost to clear VIRITO_F_IOMMU_PLATFORM which is wrong since > > > > > VIRTIO_F_IOMMU_PLATFORM is mandatory for security. Fixing this by > > > > > enforce it after vdc->get_features(). > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > --- > > > > > hw/virtio/virtio-bus.c | 5 +++-- > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c > > > > > index d31cc00..a886011 100644 > > > > > --- a/hw/virtio/virtio-bus.c > > > > > +++ b/hw/virtio/virtio-bus.c > > > > > @@ -47,6 +47,7 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) > > > > > VirtioBusState *bus = VIRTIO_BUS(qbus); > > > > > VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus); > > > > > VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); > > > > > + bool has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM); > > > > > DPRINTF("%s: plug device.\n", qbus->name); > > > > > @@ -63,8 +64,8 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) > > > > > klass->device_plugged(qbus->parent, errp); > > > > > } > > > > > - if (klass->get_dma_as != NULL && > > > > > - virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) { > > > > > + if (klass->get_dma_as != NULL && has_iommu) { > > > > > + virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM); > > > > > vdev->dma_as = klass->get_dma_as(qbus->parent); > > > > > } else { > > > > > vdev->dma_as = &address_space_memory; > > > > I suspect that's not enough, we must also fail or disable vhost > > > > (depending on the options), otherwise things won't work. > > > Looks like with the patch, VIRTIO_F_IOMMU_PLATFORM will be passed to > > > vhost_set_features(). > > Okay then. Could you please test on an old host kernel and confirm > > what happens? Pls mention this in commit log. > > Test were done before sending this patch :) > > Vhost fallback to userspace since it lacks VIRTIO_F_IOMMU_PLATFORM. > > > > > > So if vhost backend does not support it, it will fall > > > back to userspace (This may not work for vhost-user, but it's a bug existed > > > even before this patch). > > > > > > Thanks > > I guess this is true but this suddenly makes it important to > > fix this. So I prefer a patchset where patch 2 addresses > > the fallback bug. In which way? Do you think we should > > just fail if vhost doesn't work? Does this apply to both > > userspace and kernel? > > For 1.0 devices we could do this through NEEDS_RESET, but it doesn't work > for legacy device. For kernel vhost, I'd prefer to keep the current > fallback. I'm not so sure. This has security implications, e.g. user might expect not to be affected by a bug in qemu if specifying vhost=on. > For vhost-user, it seems not easy and we could not just terminate > qemu in this case consider this kind of failure can only be detected during > start not init. > > Thanks I think we should at least deny connection.
On 2017年01月20日 06:27, Michael S. Tsirkin wrote: > On Thu, Jan 19, 2017 at 11:12:21AM +0800, Jason Wang wrote: >> >> On 2017年01月19日 01:50, Michael S. Tsirkin wrote: >>> On Wed, Jan 18, 2017 at 10:42:48AM +0800, Jason Wang wrote: >>>> On 2017年01月17日 22:44, Michael S. Tsirkin wrote: >>>>> On Tue, Jan 17, 2017 at 12:01:00PM +0800, Jason Wang wrote: >>>>>> We allow vhost to clear VIRITO_F_IOMMU_PLATFORM which is wrong since >>>>>> VIRTIO_F_IOMMU_PLATFORM is mandatory for security. Fixing this by >>>>>> enforce it after vdc->get_features(). >>>>>> >>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>>>>> --- >>>>>> hw/virtio/virtio-bus.c | 5 +++-- >>>>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c >>>>>> index d31cc00..a886011 100644 >>>>>> --- a/hw/virtio/virtio-bus.c >>>>>> +++ b/hw/virtio/virtio-bus.c >>>>>> @@ -47,6 +47,7 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) >>>>>> VirtioBusState *bus = VIRTIO_BUS(qbus); >>>>>> VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus); >>>>>> VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); >>>>>> + bool has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM); >>>>>> DPRINTF("%s: plug device.\n", qbus->name); >>>>>> @@ -63,8 +64,8 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) >>>>>> klass->device_plugged(qbus->parent, errp); >>>>>> } >>>>>> - if (klass->get_dma_as != NULL && >>>>>> - virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) { >>>>>> + if (klass->get_dma_as != NULL && has_iommu) { >>>>>> + virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM); >>>>>> vdev->dma_as = klass->get_dma_as(qbus->parent); >>>>>> } else { >>>>>> vdev->dma_as = &address_space_memory; >>>>> I suspect that's not enough, we must also fail or disable vhost >>>>> (depending on the options), otherwise things won't work. >>>> Looks like with the patch, VIRTIO_F_IOMMU_PLATFORM will be passed to >>>> vhost_set_features(). >>> Okay then. Could you please test on an old host kernel and confirm >>> what happens? Pls mention this in commit log. >> Test were done before sending this patch :) >> >> Vhost fallback to userspace since it lacks VIRTIO_F_IOMMU_PLATFORM. >> >>>> So if vhost backend does not support it, it will fall >>>> back to userspace (This may not work for vhost-user, but it's a bug existed >>>> even before this patch). >>>> >>>> Thanks >>> I guess this is true but this suddenly makes it important to >>> fix this. So I prefer a patchset where patch 2 addresses >>> the fallback bug. In which way? Do you think we should >>> just fail if vhost doesn't work? Does this apply to both >>> userspace and kernel? >> For 1.0 devices we could do this through NEEDS_RESET, but it doesn't work >> for legacy device. For kernel vhost, I'd prefer to keep the current >> fallback. > I'm not so sure. This has security implications, e.g. user might > expect not to be affected by a bug in qemu if specifying vhost=on. Ok. > >> For vhost-user, it seems not easy and we could not just terminate >> qemu in this case consider this kind of failure can only be detected during >> start not init. >> >> Thanks > I think we should at least deny connection. Ok and we need do something after denying it, how about vdev->broken or others? Thanks
On Fri, Jan 20, 2017 at 11:28:53AM +0800, Jason Wang wrote: > > > On 2017年01月20日 06:27, Michael S. Tsirkin wrote: > > On Thu, Jan 19, 2017 at 11:12:21AM +0800, Jason Wang wrote: > > > > > > On 2017年01月19日 01:50, Michael S. Tsirkin wrote: > > > > On Wed, Jan 18, 2017 at 10:42:48AM +0800, Jason Wang wrote: > > > > > On 2017年01月17日 22:44, Michael S. Tsirkin wrote: > > > > > > On Tue, Jan 17, 2017 at 12:01:00PM +0800, Jason Wang wrote: > > > > > > > We allow vhost to clear VIRITO_F_IOMMU_PLATFORM which is wrong since > > > > > > > VIRTIO_F_IOMMU_PLATFORM is mandatory for security. Fixing this by > > > > > > > enforce it after vdc->get_features(). > > > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > --- > > > > > > > hw/virtio/virtio-bus.c | 5 +++-- > > > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c > > > > > > > index d31cc00..a886011 100644 > > > > > > > --- a/hw/virtio/virtio-bus.c > > > > > > > +++ b/hw/virtio/virtio-bus.c > > > > > > > @@ -47,6 +47,7 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) > > > > > > > VirtioBusState *bus = VIRTIO_BUS(qbus); > > > > > > > VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus); > > > > > > > VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); > > > > > > > + bool has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM); > > > > > > > DPRINTF("%s: plug device.\n", qbus->name); > > > > > > > @@ -63,8 +64,8 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) > > > > > > > klass->device_plugged(qbus->parent, errp); > > > > > > > } > > > > > > > - if (klass->get_dma_as != NULL && > > > > > > > - virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) { > > > > > > > + if (klass->get_dma_as != NULL && has_iommu) { > > > > > > > + virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM); > > > > > > > vdev->dma_as = klass->get_dma_as(qbus->parent); > > > > > > > } else { > > > > > > > vdev->dma_as = &address_space_memory; > > > > > > I suspect that's not enough, we must also fail or disable vhost > > > > > > (depending on the options), otherwise things won't work. > > > > > Looks like with the patch, VIRTIO_F_IOMMU_PLATFORM will be passed to > > > > > vhost_set_features(). > > > > Okay then. Could you please test on an old host kernel and confirm > > > > what happens? Pls mention this in commit log. > > > Test were done before sending this patch :) > > > > > > Vhost fallback to userspace since it lacks VIRTIO_F_IOMMU_PLATFORM. > > > > > > > > So if vhost backend does not support it, it will fall > > > > > back to userspace (This may not work for vhost-user, but it's a bug existed > > > > > even before this patch). > > > > > > > > > > Thanks > > > > I guess this is true but this suddenly makes it important to > > > > fix this. So I prefer a patchset where patch 2 addresses > > > > the fallback bug. In which way? Do you think we should > > > > just fail if vhost doesn't work? Does this apply to both > > > > userspace and kernel? > > > For 1.0 devices we could do this through NEEDS_RESET, but it doesn't work > > > for legacy device. For kernel vhost, I'd prefer to keep the current > > > fallback. > > I'm not so sure. This has security implications, e.g. user might > > expect not to be affected by a bug in qemu if specifying vhost=on. > > Ok. > > > > > > For vhost-user, it seems not easy and we could not just terminate > > > qemu in this case consider this kind of failure can only be detected during > > > start not init. > > > > > > Thanks > > I think we should at least deny connection. > > Ok and we need do something after denying it, how about vdev->broken or > others? > > Thanks For vhost-user qemu does not make progress until there's a valid connection.
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c index d31cc00..a886011 100644 --- a/hw/virtio/virtio-bus.c +++ b/hw/virtio/virtio-bus.c @@ -47,6 +47,7 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) VirtioBusState *bus = VIRTIO_BUS(qbus); VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus); VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); + bool has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM); DPRINTF("%s: plug device.\n", qbus->name); @@ -63,8 +64,8 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) klass->device_plugged(qbus->parent, errp); } - if (klass->get_dma_as != NULL && - virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) { + if (klass->get_dma_as != NULL && has_iommu) { + virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM); vdev->dma_as = klass->get_dma_as(qbus->parent); } else { vdev->dma_as = &address_space_memory;
We allow vhost to clear VIRITO_F_IOMMU_PLATFORM which is wrong since VIRTIO_F_IOMMU_PLATFORM is mandatory for security. Fixing this by enforce it after vdc->get_features(). Signed-off-by: Jason Wang <jasowang@redhat.com> --- hw/virtio/virtio-bus.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)