* [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM @ 2017-01-17 4:01 Jason Wang 2017-01-17 14:44 ` Michael S. Tsirkin 0 siblings, 1 reply; 8+ messages in thread From: Jason Wang @ 2017-01-17 4:01 UTC (permalink / raw) To: mst, qemu-devel; +Cc: Jason Wang 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; -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM 2017-01-17 4:01 [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM Jason Wang @ 2017-01-17 14:44 ` Michael S. Tsirkin 2017-01-18 2:42 ` Jason Wang 0 siblings, 1 reply; 8+ messages in thread From: Michael S. Tsirkin @ 2017-01-17 14:44 UTC (permalink / raw) To: Jason Wang; +Cc: qemu-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM 2017-01-17 14:44 ` Michael S. Tsirkin @ 2017-01-18 2:42 ` Jason Wang 2017-01-18 17:50 ` Michael S. Tsirkin 0 siblings, 1 reply; 8+ messages in thread From: Jason Wang @ 2017-01-18 2:42 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM 2017-01-18 2:42 ` Jason Wang @ 2017-01-18 17:50 ` Michael S. Tsirkin 2017-01-19 3:12 ` Jason Wang 0 siblings, 1 reply; 8+ messages in thread From: Michael S. Tsirkin @ 2017-01-18 17:50 UTC (permalink / raw) To: Jason Wang; +Cc: qemu-devel 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? -- MST ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM 2017-01-18 17:50 ` Michael S. Tsirkin @ 2017-01-19 3:12 ` Jason Wang 2017-01-19 22:27 ` Michael S. Tsirkin 0 siblings, 1 reply; 8+ messages in thread From: Jason Wang @ 2017-01-19 3:12 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM 2017-01-19 3:12 ` Jason Wang @ 2017-01-19 22:27 ` Michael S. Tsirkin 2017-01-20 3:28 ` Jason Wang 0 siblings, 1 reply; 8+ messages in thread From: Michael S. Tsirkin @ 2017-01-19 22:27 UTC (permalink / raw) To: Jason Wang; +Cc: qemu-devel 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. -- MST ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM 2017-01-19 22:27 ` Michael S. Tsirkin @ 2017-01-20 3:28 ` Jason Wang 2017-01-20 16:31 ` Michael S. Tsirkin 0 siblings, 1 reply; 8+ messages in thread From: Jason Wang @ 2017-01-20 3:28 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM 2017-01-20 3:28 ` Jason Wang @ 2017-01-20 16:31 ` Michael S. Tsirkin 0 siblings, 0 replies; 8+ messages in thread From: Michael S. Tsirkin @ 2017-01-20 16:31 UTC (permalink / raw) To: Jason Wang; +Cc: qemu-devel 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-01-20 16:31 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-01-17 4:01 [Qemu-devel] [PATCH] virtio: force VIRTIO_F_IOMMU_PLATFORM Jason Wang 2017-01-17 14:44 ` Michael S. Tsirkin 2017-01-18 2:42 ` Jason Wang 2017-01-18 17:50 ` Michael S. Tsirkin 2017-01-19 3:12 ` Jason Wang 2017-01-19 22:27 ` Michael S. Tsirkin 2017-01-20 3:28 ` Jason Wang 2017-01-20 16:31 ` Michael S. Tsirkin
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).