Linux Virtualization - Jun 2020 - [PATCH] s390: protvirt: virtio: Refuse device without IOMMU

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 at 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

-- 
Pierre Morel
IBM Lab Boeblingen

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 at 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

-- 
Pierre Morel
IBM Lab Boeblingen

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 at 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
>