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

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;
-- 
2.25.1

On Wed, 10 Jun 2020 15:11:51 +0200
Pierre Morel <pmorel at 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 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;
> +
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 at 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 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;
>> +
> 
> 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

-- 
Pierre Morel
IBM Lab Boeblingen

On Wed, 10 Jun 2020 15:11:51 +0200
Pierre Morel <pmorel at 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 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))
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 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

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