| Message ID | 1591794711-5915-1-git-send-email-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390: protvirt: virtio: Refuse device without IOMMU | expand |
On Wed, 10 Jun 2020 15:11:51 +0200 Pierre Morel <pmorel@linux.ibm.com> wrote: > Protected Virtualisation protects the memory of the guest and > do not allow a the host to access all of its memory. > > Let's refuse a VIRTIO device which does not use IOMMU > protected access. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > drivers/s390/virtio/virtio_ccw.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > index 5730572b52cd..06ffbc96587a 100644 > --- a/drivers/s390/virtio/virtio_ccw.c > +++ b/drivers/s390/virtio/virtio_ccw.c > @@ -986,6 +986,11 @@ static void virtio_ccw_set_status(struct virtio_device *vdev, u8 status) > if (!ccw) > return; > > + /* Protected Virtualisation guest needs IOMMU */ > + if (is_prot_virt_guest() && > + !__virtio_test_bit(vdev, VIRTIO_F_IOMMU_PLATFORM)) > + status &= ~VIRTIO_CONFIG_S_FEATURES_OK; > + set_status seems like an odd place to look at features; shouldn't that rather be done in finalize_features? > /* Write the status to the host. */ > vcdev->dma_area->status = status; > ccw->cmd_code = CCW_CMD_WRITE_STATUS;
On 2020-06-10 15:24, Cornelia Huck wrote: > On Wed, 10 Jun 2020 15:11:51 +0200 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> Protected Virtualisation protects the memory of the guest and >> do not allow a the host to access all of its memory. >> >> Let's refuse a VIRTIO device which does not use IOMMU >> protected access. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> drivers/s390/virtio/virtio_ccw.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c >> index 5730572b52cd..06ffbc96587a 100644 >> --- a/drivers/s390/virtio/virtio_ccw.c >> +++ b/drivers/s390/virtio/virtio_ccw.c >> @@ -986,6 +986,11 @@ static void virtio_ccw_set_status(struct virtio_device *vdev, u8 status) >> if (!ccw) >> return; >> >> + /* Protected Virtualisation guest needs IOMMU */ >> + if (is_prot_virt_guest() && >> + !__virtio_test_bit(vdev, VIRTIO_F_IOMMU_PLATFORM)) >> + status &= ~VIRTIO_CONFIG_S_FEATURES_OK; >> + > > set_status seems like an odd place to look at features; shouldn't that > rather be done in finalize_features? Right, looks better to me too. What about: diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c index 06ffbc96587a..227676297ea0 100644 --- a/drivers/s390/virtio/virtio_ccw.c +++ b/drivers/s390/virtio/virtio_ccw.c @@ -833,6 +833,11 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev) ret = -ENOMEM; goto out_free; } + + if (is_prot_virt_guest() && + !__virtio_test_bit(vdev, VIRTIO_F_IOMMU_PLATFORM)) + return -EIO; + /* Give virtio_ring a chance to accept features. */ vring_transport_features(vdev); Thanks, Regards, Pierre
On Wed, 10 Jun 2020 16:37:55 +0200 Pierre Morel <pmorel@linux.ibm.com> wrote: > On 2020-06-10 15:24, Cornelia Huck wrote: > > On Wed, 10 Jun 2020 15:11:51 +0200 > > Pierre Morel <pmorel@linux.ibm.com> wrote: > > > >> Protected Virtualisation protects the memory of the guest and > >> do not allow a the host to access all of its memory. > >> > >> Let's refuse a VIRTIO device which does not use IOMMU > >> protected access. > >> > >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > >> --- > >> drivers/s390/virtio/virtio_ccw.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > >> index 5730572b52cd..06ffbc96587a 100644 > >> --- a/drivers/s390/virtio/virtio_ccw.c > >> +++ b/drivers/s390/virtio/virtio_ccw.c > >> @@ -986,6 +986,11 @@ static void virtio_ccw_set_status(struct virtio_device *vdev, u8 status) > >> if (!ccw) > >> return; > >> > >> + /* Protected Virtualisation guest needs IOMMU */ > >> + if (is_prot_virt_guest() && > >> + !__virtio_test_bit(vdev, VIRTIO_F_IOMMU_PLATFORM)) > >> + status &= ~VIRTIO_CONFIG_S_FEATURES_OK; > >> + > > > > set_status seems like an odd place to look at features; shouldn't that > > rather be done in finalize_features? > > Right, looks better to me too. > What about: > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c > b/drivers/s390/virtio/virtio_ccw.c > index 06ffbc96587a..227676297ea0 100644 > --- a/drivers/s390/virtio/virtio_ccw.c > +++ b/drivers/s390/virtio/virtio_ccw.c > @@ -833,6 +833,11 @@ static int virtio_ccw_finalize_features(struct > virtio_device *vdev) > ret = -ENOMEM; > goto out_free; > } > + > + if (is_prot_virt_guest() && > + !__virtio_test_bit(vdev, VIRTIO_F_IOMMU_PLATFORM)) Add a comment, and (maybe) a message? Otherwise, I think this is fine, as it should fail the probe, which is what we want. > + return -EIO; > + > /* Give virtio_ring a chance to accept features. */ > vring_transport_features(vdev); > > > > Thanks, > > Regards, > Pierre >
On 2020-06-10 16:53, Cornelia Huck wrote: > On Wed, 10 Jun 2020 16:37:55 +0200 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> On 2020-06-10 15:24, Cornelia Huck wrote: >>> On Wed, 10 Jun 2020 15:11:51 +0200 >>> Pierre Morel <pmorel@linux.ibm.com> wrote: >>> >>>> Protected Virtualisation protects the memory of the guest and >>>> do not allow a the host to access all of its memory. >>>> >>>> Let's refuse a VIRTIO device which does not use IOMMU >>>> protected access. >>>> >>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>> --- >>>> drivers/s390/virtio/virtio_ccw.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c >>>> index 5730572b52cd..06ffbc96587a 100644 >>>> --- a/drivers/s390/virtio/virtio_ccw.c >>>> +++ b/drivers/s390/virtio/virtio_ccw.c >>>> @@ -986,6 +986,11 @@ static void virtio_ccw_set_status(struct virtio_device *vdev, u8 status) >>>> if (!ccw) >>>> return; >>>> >>>> + /* Protected Virtualisation guest needs IOMMU */ >>>> + if (is_prot_virt_guest() && >>>> + !__virtio_test_bit(vdev, VIRTIO_F_IOMMU_PLATFORM)) >>>> + status &= ~VIRTIO_CONFIG_S_FEATURES_OK; >>>> + >>> >>> set_status seems like an odd place to look at features; shouldn't that >>> rather be done in finalize_features? >> >> Right, looks better to me too. >> What about: >> >> >> >> diff --git a/drivers/s390/virtio/virtio_ccw.c >> b/drivers/s390/virtio/virtio_ccw.c >> index 06ffbc96587a..227676297ea0 100644 >> --- a/drivers/s390/virtio/virtio_ccw.c >> +++ b/drivers/s390/virtio/virtio_ccw.c >> @@ -833,6 +833,11 @@ static int virtio_ccw_finalize_features(struct >> virtio_device *vdev) >> ret = -ENOMEM; >> goto out_free; >> } >> + >> + if (is_prot_virt_guest() && >> + !__virtio_test_bit(vdev, VIRTIO_F_IOMMU_PLATFORM)) > > Add a comment, and (maybe) a message? > > Otherwise, I think this is fine, as it should fail the probe, which is > what we want. yes right a message is needed. and I extend a little the comment I had before. thanks Regards, Pierre
On Wed, 10 Jun 2020 15:11:51 +0200 Pierre Morel <pmorel@linux.ibm.com> wrote: > Protected Virtualisation protects the memory of the guest and > do not allow a the host to access all of its memory. > > Let's refuse a VIRTIO device which does not use IOMMU > protected access. > Should we ever get a virtio-ccw device implemented in hardware, we could in theory have a trusted device, i.e. one that is not affected by the memory protection. IMHO this restriction applies to paravitualized devices, that is devices realized by the hypervisor. In that sense this is not about ccw, but could in the future affect PCI as well. Thus the transport code may not be the best place to fence this, but frankly looking at how the QEMU discussion is going (the one where I try to prevent this condition) I would be glad to have something like this as a safety net. I would however like the commit message to reflect what is written above. Do we need backports (and cc-stable) for this? Connie what do you think? > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > drivers/s390/virtio/virtio_ccw.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > index 5730572b52cd..06ffbc96587a 100644 > --- a/drivers/s390/virtio/virtio_ccw.c > +++ b/drivers/s390/virtio/virtio_ccw.c > @@ -986,6 +986,11 @@ static void virtio_ccw_set_status(struct virtio_device *vdev, u8 status) > if (!ccw) > return; > > + /* Protected Virtualisation guest needs IOMMU */ > + if (is_prot_virt_guest() && > + !__virtio_test_bit(vdev, VIRTIO_F_IOMMU_PLATFORM)) If you were to add && !__virtio_test_bit(vdev, VIRTIO_F_ORDER_PLATFORM) we could confine this check and the bail out to paravirtualized devices, because a device realized in HW is expected to give us both F_ACCESS_PLATFORM and F_ORDER_PLATFORM. I would not bet it will ever matter for virtio-ccw though. Connie, do you have an opinion on this? Regards, Halil > + status &= ~VIRTIO_CONFIG_S_FEATURES_OK; > + > /* Write the status to the host. */ > vcdev->dma_area->status = status; > ccw->cmd_code = CCW_CMD_WRITE_STATUS;
On 2020/6/10 下午9:11, Pierre Morel wrote: > Protected Virtualisation protects the memory of the guest and > do not allow a the host to access all of its memory. > > Let's refuse a VIRTIO device which does not use IOMMU > protected access. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > drivers/s390/virtio/virtio_ccw.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > index 5730572b52cd..06ffbc96587a 100644 > --- a/drivers/s390/virtio/virtio_ccw.c > +++ b/drivers/s390/virtio/virtio_ccw.c > @@ -986,6 +986,11 @@ static void virtio_ccw_set_status(struct virtio_device *vdev, u8 status) > if (!ccw) > return; > > + /* Protected Virtualisation guest needs IOMMU */ > + if (is_prot_virt_guest() && > + !__virtio_test_bit(vdev, VIRTIO_F_IOMMU_PLATFORM)) > + status &= ~VIRTIO_CONFIG_S_FEATURES_OK; > + > /* Write the status to the host. */ > vcdev->dma_area->status = status; > ccw->cmd_code = CCW_CMD_WRITE_STATUS; I wonder whether we need move it to virtio core instead of ccw. I think the other memory protection technologies may suffer from this as well. Thanks
On 2020-06-11 05:10, Jason Wang wrote: > > On 2020/6/10 下午9:11, Pierre Morel wrote: >> Protected Virtualisation protects the memory of the guest and >> do not allow a the host to access all of its memory. >> >> Let's refuse a VIRTIO device which does not use IOMMU >> protected access. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> drivers/s390/virtio/virtio_ccw.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/s390/virtio/virtio_ccw.c >> b/drivers/s390/virtio/virtio_ccw.c >> index 5730572b52cd..06ffbc96587a 100644 >> --- a/drivers/s390/virtio/virtio_ccw.c >> +++ b/drivers/s390/virtio/virtio_ccw.c >> @@ -986,6 +986,11 @@ static void virtio_ccw_set_status(struct >> virtio_device *vdev, u8 status) >> if (!ccw) >> return; >> + /* Protected Virtualisation guest needs IOMMU */ >> + if (is_prot_virt_guest() && >> + !__virtio_test_bit(vdev, VIRTIO_F_IOMMU_PLATFORM)) >> + status &= ~VIRTIO_CONFIG_S_FEATURES_OK; >> + >> /* Write the status to the host. */ >> vcdev->dma_area->status = status; >> ccw->cmd_code = CCW_CMD_WRITE_STATUS; > > > I wonder whether we need move it to virtio core instead of ccw. > > I think the other memory protection technologies may suffer from this as > well. > > Thanks > What would you think of the following, also taking into account Connie's comment on where the test should be done: - declare a weak function in virtio.c code, returning that memory protection is not in use. - overwrite the function in the arch code - call this function inside core virtio_finalize_features() and if required fail if the device don't have VIRTIO_F_IOMMU_PLATFORM. Alternative could be to test a global variable that the architecture would overwrite if needed but I find the weak function solution more flexible. With a function, we also have the possibility to provide the device as argument and take actions depending it, this may answer Halil's concern. Regards, Pierre
On 2020-06-12 11:21, Pierre Morel wrote: > > > On 2020-06-11 05:10, Jason Wang wrote: >> >> On 2020/6/10 下午9:11, Pierre Morel wrote: >>> Protected Virtualisation protects the memory of the guest and >>> do not allow a the host to access all of its memory. >>> >>> Let's refuse a VIRTIO device which does not use IOMMU >>> protected access. >>> >>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>> --- >>> drivers/s390/virtio/virtio_ccw.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/drivers/s390/virtio/virtio_ccw.c >>> b/drivers/s390/virtio/virtio_ccw.c >>> index 5730572b52cd..06ffbc96587a 100644 >>> --- a/drivers/s390/virtio/virtio_ccw.c >>> +++ b/drivers/s390/virtio/virtio_ccw.c >>> @@ -986,6 +986,11 @@ static void virtio_ccw_set_status(struct >>> virtio_device *vdev, u8 status) >>> if (!ccw) >>> return; >>> + /* Protected Virtualisation guest needs IOMMU */ >>> + if (is_prot_virt_guest() && >>> + !__virtio_test_bit(vdev, VIRTIO_F_IOMMU_PLATFORM)) >>> + status &= ~VIRTIO_CONFIG_S_FEATURES_OK; >>> + >>> /* Write the status to the host. */ >>> vcdev->dma_area->status = status; >>> ccw->cmd_code = CCW_CMD_WRITE_STATUS; >> >> >> I wonder whether we need move it to virtio core instead of ccw. >> >> I think the other memory protection technologies may suffer from this >> as well. >> >> Thanks >> > > > What would you think of the following, also taking into account Connie's > comment on where the test should be done: > > - declare a weak function in virtio.c code, returning that memory > protection is not in use. > > - overwrite the function in the arch code > > - call this function inside core virtio_finalize_features() and if > required fail if the device don't have VIRTIO_F_IOMMU_PLATFORM. > > Alternative could be to test a global variable that the architecture > would overwrite if needed but I find the weak function solution more > flexible. > > With a function, we also have the possibility to provide the device as > argument and take actions depending it, this may answer Halil's concern. > > Regards, > Pierre > hum, in between I found another way which seems to me much better: We already have the force_dma_unencrypted() function available which AFAIU is what we want for encrypted memory protection and is already used by power and x86 SEV/SME in a way that seems AFAIU compatible with our problem. Even DMA and IOMMU are different things, I think they should be used together in our case. What do you think? The patch would then be something like: diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index a977e32a88f2..53476d5bbe35 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -4,6 +4,7 @@ #include <linux/virtio_config.h> #include <linux/module.h> #include <linux/idr.h> +#include <linux/dma-direct.h> #include <uapi/linux/virtio_ids.h> /* Unique numbering for virtio devices. */ @@ -179,6 +180,10 @@ int virtio_finalize_features(struct virtio_device *dev) if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) return 0; + if (force_dma_unencrypted(&dev->dev) && + !virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) + return -EIO; + virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK); status = dev->config->get_status(dev); if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) { Regards, Pierre
On Wed, Jun 10, 2020 at 12:32 PM Pierre Morel <pmorel@linux.ibm.com> wrote: > > Protected Virtualisation protects the memory of the guest and > do not allow a the host to access all of its memory. > > Let's refuse a VIRTIO device which does not use IOMMU > protected access. > Stupid questions: 1. Do all CPU families we care about (which are?) support IOMMU? Ex: would it recognize an ARM thingie with SMMU? [1] 2. Would it make sense to have some kind of yes-I-know-the-consequences-but-I-need-to-have-a-virtio-device-without-iommu-in-this-guest flag? > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > drivers/s390/virtio/virtio_ccw.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > index 5730572b52cd..06ffbc96587a 100644 > --- a/drivers/s390/virtio/virtio_ccw.c > +++ b/drivers/s390/virtio/virtio_ccw.c > @@ -986,6 +986,11 @@ static void virtio_ccw_set_status(struct virtio_device *vdev, u8 status) > if (!ccw) > return; > > + /* Protected Virtualisation guest needs IOMMU */ > + if (is_prot_virt_guest() && > + !__virtio_test_bit(vdev, VIRTIO_F_IOMMU_PLATFORM)) > + status &= ~VIRTIO_CONFIG_S_FEATURES_OK; > + > /* Write the status to the host. */ > vcdev->dma_area->status = status; > ccw->cmd_code = CCW_CMD_WRITE_STATUS; > -- > 2.25.1 > [1] https://developer.arm.com/architectures/system-architectures/system-components/system-mmu-support
On 2020-06-12 15:45, Mauricio Tavares wrote: > On Wed, Jun 10, 2020 at 12:32 PM Pierre Morel <pmorel@linux.ibm.com> wrote: >> >> Protected Virtualisation protects the memory of the guest and >> do not allow a the host to access all of its memory. >> >> Let's refuse a VIRTIO device which does not use IOMMU >> protected access. >> > Stupid questions: not stupid at all. :) > > 1. Do all CPU families we care about (which are?) support IOMMU? Ex: > would it recognize an ARM thingie with SMMU? [1] In Message-ID: <6356ba7f-afab-75e1-05ff-4a22b88c610e@linux.ibm.com> (as answer to Jason) I modified the patch and propose to take care of this problem by using force_dma_unencrypted() inside virtio core instead of a S390 specific test. If we use force_dma_unencrypted(dev) to check if we must refuse a device without the VIRTIO_F_IOMMU_PLATFORM feature, we are safe: only architectures defining CONFIG_ARCH_HAS_FORCE_DMA_UNENCRYPTED will have to define force_dma_unencrypted(dev), and they can choose what to do by checking the architecture functionalities and/or the device. > 2. Would it make sense to have some kind of > yes-I-know-the-consequences-but-I-need-to-have-a-virtio-device-without-iommu-in-this-guest > flag? Yes, two ways: Never refuse a device without VIRTIO_F_IOMMU_PLATFORM, by not defining CONFIG_ARCH_HAS_FORCE_DMA_UNENCRYPTED or by always return 0 in force_dma_unencrypted() have force_dma_unencrypted() selectively answer by checking the device and/or architecture state. > ...snip... >> > > [1] https://developer.arm.com/architectures/system-architectures/system-components/system-mmu-support > Regards, Pierre
On 2020/6/12 下午7:38, Pierre Morel wrote: > > > On 2020-06-12 11:21, Pierre Morel wrote: >> >> >> On 2020-06-11 05:10, Jason Wang wrote: >>> >>> On 2020/6/10 下午9:11, Pierre Morel wrote: >>>> Protected Virtualisation protects the memory of the guest and >>>> do not allow a the host to access all of its memory. >>>> >>>> Let's refuse a VIRTIO device which does not use IOMMU >>>> protected access. >>>> >>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>> --- >>>> drivers/s390/virtio/virtio_ccw.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/drivers/s390/virtio/virtio_ccw.c >>>> b/drivers/s390/virtio/virtio_ccw.c >>>> index 5730572b52cd..06ffbc96587a 100644 >>>> --- a/drivers/s390/virtio/virtio_ccw.c >>>> +++ b/drivers/s390/virtio/virtio_ccw.c >>>> @@ -986,6 +986,11 @@ static void virtio_ccw_set_status(struct >>>> virtio_device *vdev, u8 status) >>>> if (!ccw) >>>> return; >>>> + /* Protected Virtualisation guest needs IOMMU */ >>>> + if (is_prot_virt_guest() && >>>> + !__virtio_test_bit(vdev, VIRTIO_F_IOMMU_PLATFORM)) >>>> + status &= ~VIRTIO_CONFIG_S_FEATURES_OK; >>>> + >>>> /* Write the status to the host. */ >>>> vcdev->dma_area->status = status; >>>> ccw->cmd_code = CCW_CMD_WRITE_STATUS; >>> >>> >>> I wonder whether we need move it to virtio core instead of ccw. >>> >>> I think the other memory protection technologies may suffer from >>> this as well. >>> >>> Thanks >>> >> >> >> What would you think of the following, also taking into account >> Connie's comment on where the test should be done: >> >> - declare a weak function in virtio.c code, returning that memory >> protection is not in use. >> >> - overwrite the function in the arch code >> >> - call this function inside core virtio_finalize_features() and if >> required fail if the device don't have VIRTIO_F_IOMMU_PLATFORM. I think this is fine. >> >> Alternative could be to test a global variable that the architecture >> would overwrite if needed but I find the weak function solution more >> flexible. >> >> With a function, we also have the possibility to provide the device >> as argument and take actions depending it, this may answer Halil's >> concern. >> >> Regards, >> Pierre >> > > hum, in between I found another way which seems to me much better: > > We already have the force_dma_unencrypted() function available which > AFAIU is what we want for encrypted memory protection and is already > used by power and x86 SEV/SME in a way that seems AFAIU compatible > with our problem. > > Even DMA and IOMMU are different things, I think they should be used > together in our case. > > What do you think? > > The patch would then be something like: > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index a977e32a88f2..53476d5bbe35 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -4,6 +4,7 @@ > #include <linux/virtio_config.h> > #include <linux/module.h> > #include <linux/idr.h> > +#include <linux/dma-direct.h> > #include <uapi/linux/virtio_ids.h> > > /* Unique numbering for virtio devices. */ > @@ -179,6 +180,10 @@ int virtio_finalize_features(struct virtio_device > *dev) > if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) > return 0; > > + if (force_dma_unencrypted(&dev->dev) && > + !virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) > + return -EIO; > + > virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK); > status = dev->config->get_status(dev); > if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) { I think this can work but need to listen from Michael. Thanks > > > Regards, > Pierre >
On Mon, 15 Jun 2020 11:01:55 +0800 Jason Wang <jasowang@redhat.com> wrote: > > hum, in between I found another way which seems to me much better: > > > > We already have the force_dma_unencrypted() function available which > > AFAIU is what we want for encrypted memory protection and is already > > used by power and x86 SEV/SME in a way that seems AFAIU compatible > > with our problem. > > > > Even DMA and IOMMU are different things, I think they should be used > > together in our case. > > > > What do you think? > > > > The patch would then be something like: > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > > index a977e32a88f2..53476d5bbe35 100644 > > --- a/drivers/virtio/virtio.c > > +++ b/drivers/virtio/virtio.c > > @@ -4,6 +4,7 @@ > > #include <linux/virtio_config.h> > > #include <linux/module.h> > > #include <linux/idr.h> > > +#include <linux/dma-direct.h> > > #include <uapi/linux/virtio_ids.h> > > > > /* Unique numbering for virtio devices. */ > > @@ -179,6 +180,10 @@ int virtio_finalize_features(struct virtio_device > > *dev) > > if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) > > return 0; > > > > + if (force_dma_unencrypted(&dev->dev) && > > + !virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) > > + return -EIO; > > + > > virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK); > > status = dev->config->get_status(dev); > > if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) { > > > I think this can work but need to listen from Michael I don't think Christoph Hellwig will like force_dma_unencrypted() in virtio code: https://lkml.org/lkml/2020/2/20/630 Regards, Halil
On 2020-06-15 12:37, Halil Pasic wrote: > On Mon, 15 Jun 2020 11:01:55 +0800 > Jason Wang <jasowang@redhat.com> wrote: > >>> hum, in between I found another way which seems to me much better: >>> >>> We already have the force_dma_unencrypted() function available which >>> AFAIU is what we want for encrypted memory protection and is already >>> used by power and x86 SEV/SME in a way that seems AFAIU compatible >>> with our problem. >>> >>> Even DMA and IOMMU are different things, I think they should be used >>> together in our case. >>> >>> What do you think? >>> >>> The patch would then be something like: >>> >>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c >>> index a977e32a88f2..53476d5bbe35 100644 >>> --- a/drivers/virtio/virtio.c >>> +++ b/drivers/virtio/virtio.c >>> @@ -4,6 +4,7 @@ >>> #include <linux/virtio_config.h> >>> #include <linux/module.h> >>> #include <linux/idr.h> >>> +#include <linux/dma-direct.h> >>> #include <uapi/linux/virtio_ids.h> >>> >>> /* Unique numbering for virtio devices. */ >>> @@ -179,6 +180,10 @@ int virtio_finalize_features(struct virtio_device >>> *dev) >>> if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) >>> return 0; >>> >>> + if (force_dma_unencrypted(&dev->dev) && >>> + !virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) >>> + return -EIO; >>> + >>> virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK); >>> status = dev->config->get_status(dev); >>> if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) { >> >> >> I think this can work but need to listen from Michael > > I don't think Christoph Hellwig will like force_dma_unencrypted() > in virtio code: > https://lkml.org/lkml/2020/2/20/630 > > Regards, > Halil > OK, then back to the first idea. Thanks, Pierre
On 2020-06-15 05:01, Jason Wang wrote: > > On 2020/6/12 下午7:38, Pierre Morel wrote: >> >> >> On 2020-06-12 11:21, Pierre Morel wrote: >>> >>> >>> On 2020-06-11 05:10, Jason Wang wrote: >>>> >>>> On 2020/6/10 下午9:11, Pierre Morel wrote: >>>>> Protected Virtualisation protects the memory of the guest and >>>>> do not allow a the host to access all of its memory. >>>>> >>>>> Let's refuse a VIRTIO device which does not use IOMMU >>>>> protected access. >>>>> >>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>>> --- >>>>> drivers/s390/virtio/virtio_ccw.c | 5 +++++ >>>>> 1 file changed, 5 insertions(+) >>>>> >>>>> diff --git a/drivers/s390/virtio/virtio_ccw.c >>>>> b/drivers/s390/virtio/virtio_ccw.c >>>>> index 5730572b52cd..06ffbc96587a 100644 >>>>> --- a/drivers/s390/virtio/virtio_ccw.c >>>>> +++ b/drivers/s390/virtio/virtio_ccw.c >>>>> @@ -986,6 +986,11 @@ static void virtio_ccw_set_status(struct >>>>> virtio_device *vdev, u8 status) >>>>> if (!ccw) >>>>> return; >>>>> + /* Protected Virtualisation guest needs IOMMU */ >>>>> + if (is_prot_virt_guest() && >>>>> + !__virtio_test_bit(vdev, VIRTIO_F_IOMMU_PLATFORM)) >>>>> + status &= ~VIRTIO_CONFIG_S_FEATURES_OK; >>>>> + >>>>> /* Write the status to the host. */ >>>>> vcdev->dma_area->status = status; >>>>> ccw->cmd_code = CCW_CMD_WRITE_STATUS; >>>> >>>> >>>> I wonder whether we need move it to virtio core instead of ccw. >>>> >>>> I think the other memory protection technologies may suffer from >>>> this as well. >>>> >>>> Thanks >>>> >>> >>> >>> What would you think of the following, also taking into account >>> Connie's comment on where the test should be done: >>> >>> - declare a weak function in virtio.c code, returning that memory >>> protection is not in use. >>> >>> - overwrite the function in the arch code >>> >>> - call this function inside core virtio_finalize_features() and if >>> required fail if the device don't have VIRTIO_F_IOMMU_PLATFORM. > > > I think this is fine. > Thanks, I try this. Regards, Pierre
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c index 5730572b52cd..06ffbc96587a 100644 --- a/drivers/s390/virtio/virtio_ccw.c +++ b/drivers/s390/virtio/virtio_ccw.c @@ -986,6 +986,11 @@ static void virtio_ccw_set_status(struct virtio_device *vdev, u8 status) if (!ccw) return; + /* Protected Virtualisation guest needs IOMMU */ + if (is_prot_virt_guest() && + !__virtio_test_bit(vdev, VIRTIO_F_IOMMU_PLATFORM)) + status &= ~VIRTIO_CONFIG_S_FEATURES_OK; + /* Write the status to the host. */ vcdev->dma_area->status = status; ccw->cmd_code = CCW_CMD_WRITE_STATUS;
Protected Virtualisation protects the memory of the guest and do not allow a the host to access all of its memory. Let's refuse a VIRTIO device which does not use IOMMU protected access. Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> --- drivers/s390/virtio/virtio_ccw.c | 5 +++++ 1 file changed, 5 insertions(+)