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

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
>

On Mon, 15 Jun 2020 11:01:55 +0800
Jason Wang <jasowang at 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 at 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

-- 
Pierre Morel
IBM Lab Boeblingen

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 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.
> 
Thanks,
I try this.

Regards,
Pierre




-- 
Pierre Morel
IBM Lab Boeblingen