[v3,1/1] s390: virtio: let arch accept devices without IOMMU feature

Commit Message

Pierre Morel June 17, 2020, 10:43 a.m. UTC

An architecture protecting the guest memory against unauthorized host
access may want to enforce VIRTIO I/O device protection through the
use of VIRTIO_F_IOMMU_PLATFORM.

Let's give a chance to the architecture to accept or not devices
without VIRTIO_F_IOMMU_PLATFORM.

Signed-off-by: Pierre Morel <[e-mail]>
Acked-by: Jason Wang <[e-mail]>
Acked-by: Christian Borntraeger <[e-mail]>
---
 arch/s390/mm/init.c     |  6 ++++++
 drivers/virtio/virtio.c | 22 ++++++++++++++++++++++
 include/linux/virtio.h  |  2 ++
 3 files changed, 30 insertions(+)

Comments

Heiko Carstens June 17, 2020, 11:22 a.m. UTC | #1

On Wed, Jun 17, 2020 at 12:43:57PM +0200, Pierre Morel wrote:
> An architecture protecting the guest memory against unauthorized host
> access may want to enforce VIRTIO I/O device protection through the
> use of VIRTIO_F_IOMMU_PLATFORM.
> 
> Let's give a chance to the architecture to accept or not devices
> without VIRTIO_F_IOMMU_PLATFORM.
> 
> Signed-off-by: Pierre Morel <[e-mail]>
> Acked-by: Jason Wang <[e-mail]>
> Acked-by: Christian Borntraeger <[e-mail]>
> ---
>  arch/s390/mm/init.c     |  6 ++++++
>  drivers/virtio/virtio.c | 22 ++++++++++++++++++++++
>  include/linux/virtio.h  |  2 ++
>  3 files changed, 30 insertions(+)
> 
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index 6dc7c3b60ef6..215070c03226 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -45,6 +45,7 @@
>  #include <asm/kasan.h>
>  #include <asm/dma-mapping.h>
>  #include <asm/uv.h>
> +#include <linux/virtio.h>
> 
>  pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
> 
> @@ -161,6 +162,11 @@ bool force_dma_unencrypted(struct device *dev)
>  	return is_prot_virt_guest();
>  }
> 
> +int arch_needs_virtio_iommu_platform(struct virtio_device *dev)
> +{
> +	return is_prot_virt_guest();
> +}
> +
>  /* protected virtualization */
>  static void pv_init(void)

Can we please stop dumping random code to arch/s390/mm/init.c?

All the protected virtualization functions should go into a separate
file (e.g. mem_encrypt.c like on x86), some of which could also be in
header files.

Please consider this a comment for the future.. just go ahead with
this patch as-is.

Pierre Morel June 17, 2020, 11:59 a.m. UTC | #2

On 2020-06-17 13:22, Heiko Carstens wrote:
> On Wed, Jun 17, 2020 at 12:43:57PM +0200, Pierre Morel wrote:
>> An architecture protecting the guest memory against unauthorized host
>> access may want to enforce VIRTIO I/O device protection through the
>> use of VIRTIO_F_IOMMU_PLATFORM.
>>
>> Let's give a chance to the architecture to accept or not devices
>> without VIRTIO_F_IOMMU_PLATFORM.
>>
>> Signed-off-by: Pierre Morel <[e-mail]>
>> Acked-by: Jason Wang <[e-mail]>
>> Acked-by: Christian Borntraeger <[e-mail]>
>> ---
>>   arch/s390/mm/init.c     |  6 ++++++
>>   drivers/virtio/virtio.c | 22 ++++++++++++++++++++++
>>   include/linux/virtio.h  |  2 ++
>>   3 files changed, 30 insertions(+)
>>
>> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>> index 6dc7c3b60ef6..215070c03226 100644
>> --- a/arch/s390/mm/init.c
>> +++ b/arch/s390/mm/init.c
>> @@ -45,6 +45,7 @@
>>   #include <asm/kasan.h>
>>   #include <asm/dma-mapping.h>
>>   #include <asm/uv.h>
>> +#include <linux/virtio.h>
>>
>>   pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>>
>> @@ -161,6 +162,11 @@ bool force_dma_unencrypted(struct device *dev)
>>   	return is_prot_virt_guest();
>>   }
>>
>> +int arch_needs_virtio_iommu_platform(struct virtio_device *dev)
>> +{
>> +	return is_prot_virt_guest();
>> +}
>> +
>>   /* protected virtualization */
>>   static void pv_init(void)
> 
> Can we please stop dumping random code to arch/s390/mm/init.c?
> 
> All the protected virtualization functions should go into a separate
> file (e.g. mem_encrypt.c like on x86), some of which could also be in
> header files.
> 
> Please consider this a comment for the future.. just go ahead with
> this patch as-is.
> 

OK, thanks Heiko,
Pierre

Tom Lendacky June 17, 2020, 1:36 p.m. UTC | #3

On 6/17/20 5:43 AM, Pierre Morel wrote:
> An architecture protecting the guest memory against unauthorized host
> access may want to enforce VIRTIO I/O device protection through the
> use of VIRTIO_F_IOMMU_PLATFORM.
> 
> Let's give a chance to the architecture to accept or not devices
> without VIRTIO_F_IOMMU_PLATFORM.
> 
> Signed-off-by: Pierre Morel <[e-mail]>
> Acked-by: Jason Wang <[e-mail]>
> Acked-by: Christian Borntraeger <[e-mail]>
> ---
>  arch/s390/mm/init.c     |  6 ++++++
>  drivers/virtio/virtio.c | 22 ++++++++++++++++++++++
>  include/linux/virtio.h  |  2 ++
>  3 files changed, 30 insertions(+)
> 
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index 6dc7c3b60ef6..215070c03226 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -45,6 +45,7 @@
>  #include <asm/kasan.h>
>  #include <asm/dma-mapping.h>
>  #include <asm/uv.h>
> +#include <linux/virtio.h>
>  
>  pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>  
> @@ -161,6 +162,11 @@ bool force_dma_unencrypted(struct device *dev)
>  	return is_prot_virt_guest();
>  }
>  
> +int arch_needs_virtio_iommu_platform(struct virtio_device *dev)
> +{
> +	return is_prot_virt_guest();
> +}
> +
>  /* protected virtualization */
>  static void pv_init(void)
>  {
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index a977e32a88f2..aa8e01104f86 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -167,6 +167,21 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
>  }
>  EXPORT_SYMBOL_GPL(virtio_add_status);
>  
> +/*
> + * arch_needs_virtio_iommu_platform - provide arch specific hook when finalizing
> + *				      features for VIRTIO device dev
> + * @dev: the VIRTIO device being added
> + *
> + * Permits the platform to provide architecture specific functionality when
> + * devices features are finalized. This is the default implementation.
> + * Architecture implementations can override this.
> + */
> +
> +int __weak arch_needs_virtio_iommu_platform(struct virtio_device *dev)
> +{
> +	return 0;
> +}
> +
>  int virtio_finalize_features(struct virtio_device *dev)
>  {
>  	int ret = dev->config->finalize_features(dev);
> @@ -179,6 +194,13 @@ int virtio_finalize_features(struct virtio_device *dev)
>  	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>  		return 0;
>  
> +	if (arch_needs_virtio_iommu_platform(dev) &&
> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {

Just a style nit, but the "!virtio..." should be directly under the
"arch_...", not tabbed out.

Thanks,
Tom

> +		dev_warn(&dev->dev,
> +			 "virtio: device must provide VIRTIO_F_IOMMU_PLATFORM\n");
> +		return -ENODEV;
> +	}
> +
>  	virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
>  	status = dev->config->get_status(dev);
>  	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index a493eac08393..e8526ae3463e 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -195,4 +195,6 @@ void unregister_virtio_driver(struct virtio_driver *drv);
>  #define module_virtio_driver(__virtio_driver) \
>  	module_driver(__virtio_driver, register_virtio_driver, \
>  			unregister_virtio_driver)
> +
> +int arch_needs_virtio_iommu_platform(struct virtio_device *dev);
>  #endif /* _LINUX_VIRTIO_H */
>

Pierre Morel June 17, 2020, 2:12 p.m. UTC | #4

On 2020-06-17 15:36, Tom Lendacky wrote:
> On 6/17/20 5:43 AM, Pierre Morel wrote:
>> An architecture protecting the guest memory against unauthorized host
>> access may want to enforce VIRTIO I/O device protection through the
>> use of VIRTIO_F_IOMMU_PLATFORM.
>>
>> Let's give a chance to the architecture to accept or not devices
>> without VIRTIO_F_IOMMU_PLATFORM.
>>
>> Signed-off-by: Pierre Morel <[e-mail]>
>> Acked-by: Jason Wang <[e-mail]>
>> Acked-by: Christian Borntraeger <[e-mail]>
>> ---
>>   arch/s390/mm/init.c     |  6 ++++++
>>   drivers/virtio/virtio.c | 22 ++++++++++++++++++++++
>>   include/linux/virtio.h  |  2 ++
>>   3 files changed, 30 insertions(+)
>>
>> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>> index 6dc7c3b60ef6..215070c03226 100644
>> --- a/arch/s390/mm/init.c
>> +++ b/arch/s390/mm/init.c
>> @@ -45,6 +45,7 @@
>>   #include <asm/kasan.h>
>>   #include <asm/dma-mapping.h>
>>   #include <asm/uv.h>
>> +#include <linux/virtio.h>
>>   
>>   pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>>   
>> @@ -161,6 +162,11 @@ bool force_dma_unencrypted(struct device *dev)
>>   	return is_prot_virt_guest();
>>   }
>>   
>> +int arch_needs_virtio_iommu_platform(struct virtio_device *dev)
>> +{
>> +	return is_prot_virt_guest();
>> +}
>> +
>>   /* protected virtualization */
>>   static void pv_init(void)
>>   {
>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> index a977e32a88f2..aa8e01104f86 100644
>> --- a/drivers/virtio/virtio.c
>> +++ b/drivers/virtio/virtio.c
>> @@ -167,6 +167,21 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
>>   }
>>   EXPORT_SYMBOL_GPL(virtio_add_status);
>>   
>> +/*
>> + * arch_needs_virtio_iommu_platform - provide arch specific hook when finalizing
>> + *				      features for VIRTIO device dev
>> + * @dev: the VIRTIO device being added
>> + *
>> + * Permits the platform to provide architecture specific functionality when
>> + * devices features are finalized. This is the default implementation.
>> + * Architecture implementations can override this.
>> + */
>> +
>> +int __weak arch_needs_virtio_iommu_platform(struct virtio_device *dev)
>> +{
>> +	return 0;
>> +}
>> +
>>   int virtio_finalize_features(struct virtio_device *dev)
>>   {
>>   	int ret = dev->config->finalize_features(dev);
>> @@ -179,6 +194,13 @@ int virtio_finalize_features(struct virtio_device *dev)
>>   	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>>   		return 0;
>>   
>> +	if (arch_needs_virtio_iommu_platform(dev) &&
>> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> 
> Just a style nit, but the "!virtio..." should be directly under the
> "arch_...", not tabbed out.

Oh right, thanks!

Pierre

> 
> Thanks,
> Tom
> 

...snip...

Halil Pasic June 17, 2020, 10:29 p.m. UTC | #5

On Wed, 17 Jun 2020 12:43:57 +0200
Pierre Morel <[e-mail]> wrote:

> An architecture protecting the guest memory against unauthorized host
> access may want to enforce VIRTIO I/O device protection through the
> use of VIRTIO_F_IOMMU_PLATFORM.
> 
> Let's give a chance to the architecture to accept or not devices
> without VIRTIO_F_IOMMU_PLATFORM.
> 
[..]


I'm still not really satisfied with your commit message, furthermore
I did some thinking about the abstraction you introduce here. I will
give a short analysis of that, but first things first. Your patch does
the job of preventing calamity, and the details can be changed any time,
thus: 

Acked-by: Halil Pasic <[e-mail]>

Regarding the interaction of architecture specific code with virtio core,
I believe we could have made the interface more generic.

One option is to introduce virtio_arch_finalize_features(), a hook that
could reject any feature that is inappropriate.

Another option would be to find a common name for is_prot_virt_guest()
(arch/s390) sev_active() (arch/x86) and is_secure_guest() (arch/powerpc)
and use that instead of arch_needs_virtio_iommu_platform() and where-ever
appropriate. Currently we seem to want this info in driver code only for
virtio, but if the virtio driver has a legitimate need to know, other
drivers may as well have a legitimate need to know. For example if we
wanted to protect ourselves in ccw device drivers from somebody
setting up a vfio-ccw device and attach it to the prot-virt guest (AFAICT
we only lack guest enablement for this) such a function could be useful.

But since this can be rewritten any time, let's go with the option
people already agree with, instead of more discussion.

Just another question. Do we want this backported? Do we need cc stable?
[..]


>  int virtio_finalize_features(struct virtio_device *dev)
>  {
>  	int ret = dev->config->finalize_features(dev);
> @@ -179,6 +194,13 @@ int virtio_finalize_features(struct virtio_device *dev)
>  	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>  		return 0;
>  
> +	if (arch_needs_virtio_iommu_platform(dev) &&
> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> +		dev_warn(&dev->dev,
> +			 "virtio: device must provide VIRTIO_F_IOMMU_PLATFORM\n");

I'm not sure, divulging the current Linux name of this feature bit is a
good idea, but if everybody else is fine with this, I don't care that
much. An alternative would be:
"virtio: device falsely claims to have full access to the memory,
aborting the device"


Regards,
Halil

Cornelia Huck June 19, 2020, 9:20 a.m. UTC | #6

On Thu, 18 Jun 2020 00:29:56 +0200
Halil Pasic <[e-mail]> wrote:

> On Wed, 17 Jun 2020 12:43:57 +0200
> Pierre Morel <[e-mail]> wrote:
> 
> > An architecture protecting the guest memory against unauthorized host
> > access may want to enforce VIRTIO I/O device protection through the
> > use of VIRTIO_F_IOMMU_PLATFORM.
> > 
> > Let's give a chance to the architecture to accept or not devices
> > without VIRTIO_F_IOMMU_PLATFORM.
> >   
> [..]
> 
> 
> I'm still not really satisfied with your commit message, furthermore
> I did some thinking about the abstraction you introduce here. I will
> give a short analysis of that, but first things first. Your patch does
> the job of preventing calamity, and the details can be changed any time,
> thus: 
> 
> Acked-by: Halil Pasic <[e-mail]>
> 
> Regarding the interaction of architecture specific code with virtio core,
> I believe we could have made the interface more generic.
> 
> One option is to introduce virtio_arch_finalize_features(), a hook that
> could reject any feature that is inappropriate.

s/any feature/any combination of features/

This sounds like a good idea (for a later update).

> 
> Another option would be to find a common name for is_prot_virt_guest()
> (arch/s390) sev_active() (arch/x86) and is_secure_guest() (arch/powerpc)
> and use that instead of arch_needs_virtio_iommu_platform() and where-ever
> appropriate. Currently we seem to want this info in driver code only for
> virtio, but if the virtio driver has a legitimate need to know, other
> drivers may as well have a legitimate need to know. For example if we
> wanted to protect ourselves in ccw device drivers from somebody
> setting up a vfio-ccw device and attach it to the prot-virt guest (AFAICT
> we only lack guest enablement for this) such a function could be useful.

I'm not really sure if we can find enough commonality between
architectures, unless you propose to have a function for checking
things like device memory only.

> 
> But since this can be rewritten any time, let's go with the option
> people already agree with, instead of more discussion.

Yes, there's nothing wrong with the patch as-is.

Acked-by: Cornelia Huck <[e-mail]>

Which tree should this go through? Virtio? s390?

> 
> Just another question. Do we want this backported? Do we need cc stable?

It does change behaviour of virtio-ccw devices; but then, it only
fences off configurations that would not have worked anyway.
Distributions should probably pick this; but I do not consider it
strictly a "fix" (more a mitigation for broken configurations), so I'm
not sure whether stable applies.

> [..]
> 
> 
> >  int virtio_finalize_features(struct virtio_device *dev)
> >  {
> >  	int ret = dev->config->finalize_features(dev);
> > @@ -179,6 +194,13 @@ int virtio_finalize_features(struct virtio_device *dev)
> >  	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
> >  		return 0;
> >  
> > +	if (arch_needs_virtio_iommu_platform(dev) &&
> > +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> > +		dev_warn(&dev->dev,
> > +			 "virtio: device must provide VIRTIO_F_IOMMU_PLATFORM\n");  
> 
> I'm not sure, divulging the current Linux name of this feature bit is a
> good idea, but if everybody else is fine with this, I don't care that

Not sure if that feature name will ever change, as it is exported in
headers. At most, we might want to add the new ACCESS_PLATFORM define
and keep the old one, but that would still mean some churn.

> much. An alternative would be:
> "virtio: device falsely claims to have full access to the memory,
> aborting the device"

"virtio: device does not work with limited memory access" ?

But no issue with keeping the current message.

Halil Pasic June 19, 2020, 12:02 p.m. UTC | #7

On Fri, 19 Jun 2020 11:20:51 +0200
Cornelia Huck <[e-mail]> wrote:

> > > +	if (arch_needs_virtio_iommu_platform(dev) &&
> > > +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> > > +		dev_warn(&dev->dev,
> > > +			 "virtio: device must provide VIRTIO_F_IOMMU_PLATFORM\n");    
> > 
> > I'm not sure, divulging the current Linux name of this feature bit is a
> > good idea, but if everybody else is fine with this, I don't care that  
> 
> Not sure if that feature name will ever change, as it is exported in
> headers. At most, we might want to add the new ACCESS_PLATFORM define
> and keep the old one, but that would still mean some churn.
> 
> > much. An alternative would be:
> > "virtio: device falsely claims to have full access to the memory,
> > aborting the device"  
> 
> "virtio: device does not work with limited memory access" ?
> 
> But no issue with keeping the current message.

I think I prefer Conny's version, but no strong feelings here.

Halil

Pierre Morel June 29, 2020, 1:14 p.m. UTC | #8

On 2020-06-19 11:20, Cornelia Huck wrote:
> On Thu, 18 Jun 2020 00:29:56 +0200
> Halil Pasic <[e-mail]> wrote:
> 
>> On Wed, 17 Jun 2020 12:43:57 +0200
>> Pierre Morel <[e-mail]> wrote:
...
>>
>> But since this can be rewritten any time, let's go with the option
>> people already agree with, instead of more discussion.
> 
> Yes, there's nothing wrong with the patch as-is.
> 
> Acked-by: Cornelia Huck <[e-mail]>

Thanks,


> 
> Which tree should this go through? Virtio? s390? >
>>
>> Just another question. Do we want this backported? Do we need cc stable?
> 
> It does change behaviour of virtio-ccw devices; but then, it only
> fences off configurations that would not have worked anyway.
> Distributions should probably pick this; but I do not consider it
> strictly a "fix" (more a mitigation for broken configurations), so I'm
> not sure whether stable applies.
> 
>> [..]
>>
>>
>>>   int virtio_finalize_features(struct virtio_device *dev)
>>>   {
>>>   	int ret = dev->config->finalize_features(dev);
>>> @@ -179,6 +194,13 @@ int virtio_finalize_features(struct virtio_device *dev)
>>>   	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>>>   		return 0;
>>>   
>>> +	if (arch_needs_virtio_iommu_platform(dev) &&
>>> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
>>> +		dev_warn(&dev->dev,
>>> +			 "virtio: device must provide VIRTIO_F_IOMMU_PLATFORM\n");
>>
>> I'm not sure, divulging the current Linux name of this feature bit is a
>> good idea, but if everybody else is fine with this, I don't care that
> 
> Not sure if that feature name will ever change, as it is exported in
> headers. At most, we might want to add the new ACCESS_PLATFORM define
> and keep the old one, but that would still mean some churn.
> 
>> much. An alternative would be:
>> "virtio: device falsely claims to have full access to the memory,
>> aborting the device"
> 
> "virtio: device does not work with limited memory access" ?
> 
> But no issue with keeping the current message.
> 

If it is OK, I would like to specify that the arch is responsible to 
accept or not the device.
The reason why the device is not accepted without IOMMU_PLATFORM is arch 
specific.

Regards,
Pierre

Pierre Morel June 29, 2020, 1:15 p.m. UTC | #9

On 2020-06-19 14:02, Halil Pasic wrote:
> On Fri, 19 Jun 2020 11:20:51 +0200
> Cornelia Huck <[e-mail]> wrote:
> 
>>>> +	if (arch_needs_virtio_iommu_platform(dev) &&
>>>> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
>>>> +		dev_warn(&dev->dev,
>>>> +			 "virtio: device must provide VIRTIO_F_IOMMU_PLATFORM\n");
>>>
>>> I'm not sure, divulging the current Linux name of this feature bit is a
>>> good idea, but if everybody else is fine with this, I don't care that
>>
>> Not sure if that feature name will ever change, as it is exported in
>> headers. At most, we might want to add the new ACCESS_PLATFORM define
>> and keep the old one, but that would still mean some churn.
>>
>>> much. An alternative would be:
>>> "virtio: device falsely claims to have full access to the memory,
>>> aborting the device"
>>
>> "virtio: device does not work with limited memory access" ?
>>
>> But no issue with keeping the current message.
> 
> I think I prefer Conny's version, but no strong feelings here.
> 


The reason why the device is not accepted without IOMMU_PLATFORM is arch 
specific, I think it should be clearly stated.
If no strong oposition...

Thanks,
Pierre

Pierre Morel June 29, 2020, 1:21 p.m. UTC | #10

On 2020-06-18 00:29, Halil Pasic wrote:
> On Wed, 17 Jun 2020 12:43:57 +0200
> Pierre Morel <[e-mail]> wrote:
> 
>> An architecture protecting the guest memory against unauthorized host
>> access may want to enforce VIRTIO I/O device protection through the
>> use of VIRTIO_F_IOMMU_PLATFORM.
>>
>> Let's give a chance to the architecture to accept or not devices
>> without VIRTIO_F_IOMMU_PLATFORM.
>>
> [..]
> 
> 
> I'm still not really satisfied with your commit message, furthermore
> I did some thinking about the abstraction you introduce here. I will
> give a short analysis of that, but first things first. Your patch does
> the job of preventing calamity, and the details can be changed any time,
> thus:
> 
> Acked-by: Halil Pasic <[e-mail]>

Thanks,

Connie already answered the other points you raised and I have nothing 
to add on it.

Regards,
Pierre

Cornelia Huck June 29, 2020, 1:44 p.m. UTC | #11

On Mon, 29 Jun 2020 15:14:04 +0200
Pierre Morel <[e-mail]> wrote:

> On 2020-06-19 11:20, Cornelia Huck wrote:
> > On Thu, 18 Jun 2020 00:29:56 +0200
> > Halil Pasic <[e-mail]> wrote:
> >   
> >> On Wed, 17 Jun 2020 12:43:57 +0200
> >> Pierre Morel <[e-mail]> wrote:  

> >>> @@ -179,6 +194,13 @@ int virtio_finalize_features(struct virtio_device *dev)
> >>>   	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
> >>>   		return 0;
> >>>   
> >>> +	if (arch_needs_virtio_iommu_platform(dev) &&
> >>> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> >>> +		dev_warn(&dev->dev,
> >>> +			 "virtio: device must provide VIRTIO_F_IOMMU_PLATFORM\n");  

[Side note: wasn't there a patch renaming this bit on the list? I think
this name is only kept for userspace compat.]

> >>
> >> I'm not sure, divulging the current Linux name of this feature bit is a
> >> good idea, but if everybody else is fine with this, I don't care that  
> > 
> > Not sure if that feature name will ever change, as it is exported in
> > headers. At most, we might want to add the new ACCESS_PLATFORM define
> > and keep the old one, but that would still mean some churn.
> >   
> >> much. An alternative would be:
> >> "virtio: device falsely claims to have full access to the memory,
> >> aborting the device"  
> > 
> > "virtio: device does not work with limited memory access" ?
> > 
> > But no issue with keeping the current message.
> >   
> 
> If it is OK, I would like to specify that the arch is responsible to 
> accept or not the device.
> The reason why the device is not accepted without IOMMU_PLATFORM is arch 
> specific.

Hm, I'd think the reason is always the same (the device cannot access
the memory directly), just the way to figure out whether that is the
case or not is arch-specific, as with so many other things. No real
need to go into detail here, I think.

Michael S. Tsirkin June 29, 2020, 3:57 p.m. UTC | #12

On Wed, Jun 17, 2020 at 12:43:57PM +0200, Pierre Morel wrote:
> An architecture protecting the guest memory against unauthorized host
> access may want to enforce VIRTIO I/O device protection through the
> use of VIRTIO_F_IOMMU_PLATFORM.
> 
> Let's give a chance to the architecture to accept or not devices
> without VIRTIO_F_IOMMU_PLATFORM.
> 
> Signed-off-by: Pierre Morel <[e-mail]>
> Acked-by: Jason Wang <[e-mail]>
> Acked-by: Christian Borntraeger <[e-mail]>
> ---
>  arch/s390/mm/init.c     |  6 ++++++
>  drivers/virtio/virtio.c | 22 ++++++++++++++++++++++
>  include/linux/virtio.h  |  2 ++
>  3 files changed, 30 insertions(+)
> 
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index 6dc7c3b60ef6..215070c03226 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -45,6 +45,7 @@
>  #include <asm/kasan.h>
>  #include <asm/dma-mapping.h>
>  #include <asm/uv.h>
> +#include <linux/virtio.h>
>  
>  pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>  
> @@ -161,6 +162,11 @@ bool force_dma_unencrypted(struct device *dev)
>  	return is_prot_virt_guest();
>  }
>  
> +int arch_needs_virtio_iommu_platform(struct virtio_device *dev)
> +{
> +	return is_prot_virt_guest();
> +}
> +
>  /* protected virtualization */
>  static void pv_init(void)
>  {
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index a977e32a88f2..aa8e01104f86 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -167,6 +167,21 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
>  }
>  EXPORT_SYMBOL_GPL(virtio_add_status);
>  
> +/*
> + * arch_needs_virtio_iommu_platform - provide arch specific hook when finalizing
> + *				      features for VIRTIO device dev
> + * @dev: the VIRTIO device being added
> + *
> + * Permits the platform to provide architecture specific functionality when
> + * devices features are finalized. This is the default implementation.
> + * Architecture implementations can override this.
> + */
> +
> +int __weak arch_needs_virtio_iommu_platform(struct virtio_device *dev)
> +{
> +	return 0;
> +}
> +
>  int virtio_finalize_features(struct virtio_device *dev)
>  {
>  	int ret = dev->config->finalize_features(dev);
> @@ -179,6 +194,13 @@ int virtio_finalize_features(struct virtio_device *dev)
>  	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>  		return 0;
>  
> +	if (arch_needs_virtio_iommu_platform(dev) &&
> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> +		dev_warn(&dev->dev,
> +			 "virtio: device must provide VIRTIO_F_IOMMU_PLATFORM\n");
> +		return -ENODEV;
> +	}
> +
>  	virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
>  	status = dev->config->get_status(dev);
>  	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {

Well don't you need to check it *before* VIRTIO_F_VERSION_1, not after?



> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index a493eac08393..e8526ae3463e 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -195,4 +195,6 @@ void unregister_virtio_driver(struct virtio_driver *drv);
>  #define module_virtio_driver(__virtio_driver) \
>  	module_driver(__virtio_driver, register_virtio_driver, \
>  			unregister_virtio_driver)
> +
> +int arch_needs_virtio_iommu_platform(struct virtio_device *dev);
>  #endif /* _LINUX_VIRTIO_H */
> -- 
> 2.25.1

Cornelia Huck June 29, 2020, 4:05 p.m. UTC | #13

On Mon, 29 Jun 2020 11:57:14 -0400
"Michael S. Tsirkin" <[e-mail]> wrote:

> On Wed, Jun 17, 2020 at 12:43:57PM +0200, Pierre Morel wrote:
> > An architecture protecting the guest memory against unauthorized host
> > access may want to enforce VIRTIO I/O device protection through the
> > use of VIRTIO_F_IOMMU_PLATFORM.
> > 
> > Let's give a chance to the architecture to accept or not devices
> > without VIRTIO_F_IOMMU_PLATFORM.
> > 
> > Signed-off-by: Pierre Morel <[e-mail]>
> > Acked-by: Jason Wang <[e-mail]>
> > Acked-by: Christian Borntraeger <[e-mail]>
> > ---
> >  arch/s390/mm/init.c     |  6 ++++++
> >  drivers/virtio/virtio.c | 22 ++++++++++++++++++++++
> >  include/linux/virtio.h  |  2 ++
> >  3 files changed, 30 insertions(+)

> > @@ -179,6 +194,13 @@ int virtio_finalize_features(struct virtio_device *dev)
> >  	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
> >  		return 0;
> >  
> > +	if (arch_needs_virtio_iommu_platform(dev) &&
> > +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> > +		dev_warn(&dev->dev,
> > +			 "virtio: device must provide VIRTIO_F_IOMMU_PLATFORM\n");
> > +		return -ENODEV;
> > +	}
> > +
> >  	virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
> >  	status = dev->config->get_status(dev);
> >  	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {  
> 
> Well don't you need to check it *before* VIRTIO_F_VERSION_1, not after?

But it's only available with VERSION_1 anyway, isn't it? So it probably
also needs to fail when this feature is needed if VERSION_1 has not been
negotiated, I think.

Michael S. Tsirkin June 29, 2020, 4:09 p.m. UTC | #14

On Wed, Jun 17, 2020 at 12:43:57PM +0200, Pierre Morel wrote:
> An architecture protecting the guest memory against unauthorized host
> access may want to enforce VIRTIO I/O device protection through the
> use of VIRTIO_F_IOMMU_PLATFORM.
> Let's give a chance to the architecture to accept or not devices
> without VIRTIO_F_IOMMU_PLATFORM.

I agree it's a bit misleading. Protection is enforced by memory
encryption, you can't trust the hypervisor to report the bit correctly
so using that as a securoty measure would be pointless.
The real gain here is that broken configs are easier to
debug.

Here's an attempt at a better description:

	On some architectures, guest knows that VIRTIO_F_IOMMU_PLATFORM is
	required for virtio to function: e.g. this is the case on s390 protected
	virt guests, since otherwise guest passes encrypted guest memory to devices,
	which the device can't read. Without VIRTIO_F_IOMMU_PLATFORM the
	result is that affected memory (or even a whole page containing
	it is corrupted). Detect and fail probe instead - that is easier
	to debug.

however, now that we have described what it is (hypervisor
misconfiguration) I ask a question: can we be sure this will never
ever work? E.g. what if some future hypervisor gains ability to
access the protected guest memory in some abstractly secure manner?
We are blocking this here, and it's hard to predict the future,
and a broken hypervisor can always find ways to crash the guest ...

IMHO it would be safer to just print a warning.
What do you think?



> 
> Signed-off-by: Pierre Morel <[e-mail]>
> Acked-by: Jason Wang <[e-mail]>
> Acked-by: Christian Borntraeger <[e-mail]>
> ---
>  arch/s390/mm/init.c     |  6 ++++++
>  drivers/virtio/virtio.c | 22 ++++++++++++++++++++++
>  include/linux/virtio.h  |  2 ++
>  3 files changed, 30 insertions(+)
> 
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index 6dc7c3b60ef6..215070c03226 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -45,6 +45,7 @@
>  #include <asm/kasan.h>
>  #include <asm/dma-mapping.h>
>  #include <asm/uv.h>
> +#include <linux/virtio.h>
>  
>  pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>  
> @@ -161,6 +162,11 @@ bool force_dma_unencrypted(struct device *dev)
>  	return is_prot_virt_guest();
>  }
>  
> +int arch_needs_virtio_iommu_platform(struct virtio_device *dev)
> +{
> +	return is_prot_virt_guest();
> +}
> +
>  /* protected virtualization */
>  static void pv_init(void)
>  {
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index a977e32a88f2..aa8e01104f86 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -167,6 +167,21 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
>  }
>  EXPORT_SYMBOL_GPL(virtio_add_status);
>  
> +/*
> + * arch_needs_virtio_iommu_platform - provide arch specific hook when finalizing
> + *				      features for VIRTIO device dev
> + * @dev: the VIRTIO device being added
> + *
> + * Permits the platform to provide architecture specific functionality when
> + * devices features are finalized. This is the default implementation.
> + * Architecture implementations can override this.
> + */
> +
> +int __weak arch_needs_virtio_iommu_platform(struct virtio_device *dev)
> +{
> +	return 0;
> +}
> +
>  int virtio_finalize_features(struct virtio_device *dev)
>  {
>  	int ret = dev->config->finalize_features(dev);
> @@ -179,6 +194,13 @@ int virtio_finalize_features(struct virtio_device *dev)
>  	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>  		return 0;
>  
> +	if (arch_needs_virtio_iommu_platform(dev) &&
> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> +		dev_warn(&dev->dev,
> +			 "virtio: device must provide VIRTIO_F_IOMMU_PLATFORM\n");
> +		return -ENODEV;
> +	}
> +
>  	virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
>  	status = dev->config->get_status(dev);
>  	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index a493eac08393..e8526ae3463e 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -195,4 +195,6 @@ void unregister_virtio_driver(struct virtio_driver *drv);
>  #define module_virtio_driver(__virtio_driver) \
>  	module_driver(__virtio_driver, register_virtio_driver, \
>  			unregister_virtio_driver)
> +
> +int arch_needs_virtio_iommu_platform(struct virtio_device *dev);
>  #endif /* _LINUX_VIRTIO_H */
> -- 
> 2.25.1

Pierre Morel June 29, 2020, 4:09 p.m. UTC | #15

On 2020-06-29 17:57, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2020 at 12:43:57PM +0200, Pierre Morel wrote:
>> An architecture protecting the guest memory against unauthorized host
>> access may want to enforce VIRTIO I/O device protection through the
>> use of VIRTIO_F_IOMMU_PLATFORM.
>>
>> Let's give a chance to the architecture to accept or not devices
>> without VIRTIO_F_IOMMU_PLATFORM.
>>
>> Signed-off-by: Pierre Morel <[e-mail]>
>> Acked-by: Jason Wang <[e-mail]>
>> Acked-by: Christian Borntraeger <[e-mail]>
>> ---
>>   arch/s390/mm/init.c     |  6 ++++++
>>   drivers/virtio/virtio.c | 22 ++++++++++++++++++++++
>>   include/linux/virtio.h  |  2 ++
>>   3 files changed, 30 insertions(+)
>>
>> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>> index 6dc7c3b60ef6..215070c03226 100644
>> --- a/arch/s390/mm/init.c
>> +++ b/arch/s390/mm/init.c
>> @@ -45,6 +45,7 @@
>>   #include <asm/kasan.h>
>>   #include <asm/dma-mapping.h>
>>   #include <asm/uv.h>
>> +#include <linux/virtio.h>
>>   
>>   pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
>>   
>> @@ -161,6 +162,11 @@ bool force_dma_unencrypted(struct device *dev)
>>   	return is_prot_virt_guest();
>>   }
>>   
>> +int arch_needs_virtio_iommu_platform(struct virtio_device *dev)
>> +{
>> +	return is_prot_virt_guest();
>> +}
>> +
>>   /* protected virtualization */
>>   static void pv_init(void)
>>   {
>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> index a977e32a88f2..aa8e01104f86 100644
>> --- a/drivers/virtio/virtio.c
>> +++ b/drivers/virtio/virtio.c
>> @@ -167,6 +167,21 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
>>   }
>>   EXPORT_SYMBOL_GPL(virtio_add_status);
>>   
>> +/*
>> + * arch_needs_virtio_iommu_platform - provide arch specific hook when finalizing
>> + *				      features for VIRTIO device dev
>> + * @dev: the VIRTIO device being added
>> + *
>> + * Permits the platform to provide architecture specific functionality when
>> + * devices features are finalized. This is the default implementation.
>> + * Architecture implementations can override this.
>> + */
>> +
>> +int __weak arch_needs_virtio_iommu_platform(struct virtio_device *dev)
>> +{
>> +	return 0;
>> +}
>> +
>>   int virtio_finalize_features(struct virtio_device *dev)
>>   {
>>   	int ret = dev->config->finalize_features(dev);
>> @@ -179,6 +194,13 @@ int virtio_finalize_features(struct virtio_device *dev)
>>   	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>>   		return 0;
>>   
>> +	if (arch_needs_virtio_iommu_platform(dev) &&
>> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
>> +		dev_warn(&dev->dev,
>> +			 "virtio: device must provide VIRTIO_F_IOMMU_PLATFORM\n");
>> +		return -ENODEV;
>> +	}
>> +
>>   	virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
>>   	status = dev->config->get_status(dev);
>>   	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> 
> Well don't you need to check it *before* VIRTIO_F_VERSION_1, not after?

Yes, you are right,

Thanks,

Pierre

Pierre Morel June 29, 2020, 4:10 p.m. UTC | #16

On 2020-06-29 15:44, Cornelia Huck wrote:
> On Mon, 29 Jun 2020 15:14:04 +0200
> Pierre Morel <[e-mail]> wrote:
> 
>> On 2020-06-19 11:20, Cornelia Huck wrote:
>>> On Thu, 18 Jun 2020 00:29:56 +0200
>>> Halil Pasic <[e-mail]> wrote:
>>>    
>>>> On Wed, 17 Jun 2020 12:43:57 +0200
>>>> Pierre Morel <[e-mail]> wrote:
> 
>>>>> @@ -179,6 +194,13 @@ int virtio_finalize_features(struct virtio_device *dev)
>>>>>    	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>>>>>    		return 0;
>>>>>    
>>>>> +	if (arch_needs_virtio_iommu_platform(dev) &&
>>>>> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
>>>>> +		dev_warn(&dev->dev,
>>>>> +			 "virtio: device must provide VIRTIO_F_IOMMU_PLATFORM\n");
> 
> [Side note: wasn't there a patch renaming this bit on the list? I think
> this name is only kept for userspace compat.]

Sorry, I do not understand what you expect from me.
On which mailing list you think there is a patch on?

> 
>>>>
>>>> I'm not sure, divulging the current Linux name of this feature bit is a
>>>> good idea, but if everybody else is fine with this, I don't care that
>>>
>>> Not sure if that feature name will ever change, as it is exported in
>>> headers. At most, we might want to add the new ACCESS_PLATFORM define
>>> and keep the old one, but that would still mean some churn.
>>>    
>>>> much. An alternative would be:
>>>> "virtio: device falsely claims to have full access to the memory,
>>>> aborting the device"
>>>
>>> "virtio: device does not work with limited memory access" ?
>>>
>>> But no issue with keeping the current message.
>>>    
>>
>> If it is OK, I would like to specify that the arch is responsible to
>> accept or not the device.
>> The reason why the device is not accepted without IOMMU_PLATFORM is arch
>> specific.
> 
> Hm, I'd think the reason is always the same (the device cannot access
> the memory directly), just the way to figure out whether that is the
> case or not is arch-specific, as with so many other things. No real
> need to go into detail here, I think.
> 

As you like, so I rename the subject to:
"virtio: device does not work with limited memory access"

Regards,

Pierre

Pierre Morel June 29, 2020, 4:48 p.m. UTC | #17

On 2020-06-29 18:09, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2020 at 12:43:57PM +0200, Pierre Morel wrote:
>> An architecture protecting the guest memory against unauthorized host
>> access may want to enforce VIRTIO I/O device protection through the
>> use of VIRTIO_F_IOMMU_PLATFORM.
>> Let's give a chance to the architecture to accept or not devices
>> without VIRTIO_F_IOMMU_PLATFORM.
> 
> I agree it's a bit misleading. Protection is enforced by memory
> encryption, you can't trust the hypervisor to report the bit correctly
> so using that as a securoty measure would be pointless.
> The real gain here is that broken configs are easier to
> debug.
> 
> Here's an attempt at a better description:
> 
> 	On some architectures, guest knows that VIRTIO_F_IOMMU_PLATFORM is
> 	required for virtio to function: e.g. this is the case on s390 protected
> 	virt guests, since otherwise guest passes encrypted guest memory to devices,
> 	which the device can't read. Without VIRTIO_F_IOMMU_PLATFORM the
> 	result is that affected memory (or even a whole page containing
> 	it is corrupted). Detect and fail probe instead - that is easier
> 	to debug.

Thanks indeed better aside from the "encrypted guest memory": the 
mechanism used to avoid the access to the guest memory from the host by 
s390 is not encryption but a hardware feature denying the general host 
access and allowing pieces of memory to be shared between guest and host.
As a consequence the data read from memory is not corrupted but not read 
at all and the read error kills the hypervizor with a SIGSEGV.


> 
> however, now that we have described what it is (hypervisor
> misconfiguration) I ask a question: can we be sure this will never
> ever work? E.g. what if some future hypervisor gains ability to
> access the protected guest memory in some abstractly secure manner?

The goal of the s390 PV feature is to avoid this possibility so I don't 
think so; however, there is a possibility that some hardware VIRTIO 
device gain access to the guest's protected memory, even such device 
does not exist yet.

At the moment such device exists we will need a driver for it, at least 
to enable the feature and apply policies, it is also one of the reasons 
why a hook to the architecture is interesting.

> We are blocking this here, and it's hard to predict the future,
> and a broken hypervisor can always find ways to crash the guest ...

yes, this is also something to fix on the hypervizor side, Halil is 
working on it.

> 
> IMHO it would be safer to just print a warning.
> What do you think?

Sadly, putting a warning may not help as qemu is killed if it accesses 
the protected memory.
Also note that the crash occurs not only on start but also on hotplug.

Thanks,
Pierre

Michael S. Tsirkin June 29, 2020, 9:18 p.m. UTC | #18

On Mon, Jun 29, 2020 at 06:48:28PM +0200, Pierre Morel wrote:
> 
> 
> On 2020-06-29 18:09, Michael S. Tsirkin wrote:
> > On Wed, Jun 17, 2020 at 12:43:57PM +0200, Pierre Morel wrote:
> > > An architecture protecting the guest memory against unauthorized host
> > > access may want to enforce VIRTIO I/O device protection through the
> > > use of VIRTIO_F_IOMMU_PLATFORM.
> > > Let's give a chance to the architecture to accept or not devices
> > > without VIRTIO_F_IOMMU_PLATFORM.
> > 
> > I agree it's a bit misleading. Protection is enforced by memory
> > encryption, you can't trust the hypervisor to report the bit correctly
> > so using that as a securoty measure would be pointless.
> > The real gain here is that broken configs are easier to
> > debug.
> > 
> > Here's an attempt at a better description:
> > 
> > 	On some architectures, guest knows that VIRTIO_F_IOMMU_PLATFORM is
> > 	required for virtio to function: e.g. this is the case on s390 protected
> > 	virt guests, since otherwise guest passes encrypted guest memory to devices,
> > 	which the device can't read. Without VIRTIO_F_IOMMU_PLATFORM the
> > 	result is that affected memory (or even a whole page containing
> > 	it is corrupted). Detect and fail probe instead - that is easier
> > 	to debug.
> 
> Thanks indeed better aside from the "encrypted guest memory": the mechanism
> used to avoid the access to the guest memory from the host by s390 is not
> encryption but a hardware feature denying the general host access and
> allowing pieces of memory to be shared between guest and host.

s/encrypted/protected/

> As a consequence the data read from memory is not corrupted but not read at
> all and the read error kills the hypervizor with a SIGSEGV.

s/(or even a whole page containing it is corrupted)/can not be
	read and the read error kills the hypervizor with a SIGSEGV/


As an aside, we could maybe handle that more gracefully
on the hypervisor side.

> 
> > 
> > however, now that we have described what it is (hypervisor
> > misconfiguration) I ask a question: can we be sure this will never
> > ever work? E.g. what if some future hypervisor gains ability to
> > access the protected guest memory in some abstractly secure manner?
> 
> The goal of the s390 PV feature is to avoid this possibility so I don't
> think so; however, there is a possibility that some hardware VIRTIO device
> gain access to the guest's protected memory, even such device does not exist
> yet.
> 
> At the moment such device exists we will need a driver for it, at least to
> enable the feature and apply policies, it is also one of the reasons why a
> hook to the architecture is interesting.


Not neessarily, it could also be fully transparent. See e.g.
recent AMD andvances allowing unmodified guests with SEV.


> > We are blocking this here, and it's hard to predict the future,
> > and a broken hypervisor can always find ways to crash the guest ...
> 
> yes, this is also something to fix on the hypervizor side, Halil is working
> on it.
> 
> > 
> > IMHO it would be safer to just print a warning.
> > What do you think?
> 
> Sadly, putting a warning may not help as qemu is killed if it accesses the
> protected memory.
> Also note that the crash occurs not only on start but also on hotplug.
> 
> Thanks,
> Pierre

Well that depends on where does the warning go. If it's on a serial port
it might be reported host side before the crash triggers.  But
interesting point generally. How about a feature to send a warning code
or string to host then?

> -- 
> Pierre Morel
> IBM Lab Boeblingen

Cornelia Huck June 30, 2020, 7:08 a.m. UTC | #19

On Mon, 29 Jun 2020 17:18:09 -0400
"Michael S. Tsirkin" <[e-mail]> wrote:

> On Mon, Jun 29, 2020 at 06:48:28PM +0200, Pierre Morel wrote:
> > 
> > 
> > On 2020-06-29 18:09, Michael S. Tsirkin wrote:  
> > > On Wed, Jun 17, 2020 at 12:43:57PM +0200, Pierre Morel wrote:  
> > > > An architecture protecting the guest memory against unauthorized host
> > > > access may want to enforce VIRTIO I/O device protection through the
> > > > use of VIRTIO_F_IOMMU_PLATFORM.
> > > > Let's give a chance to the architecture to accept or not devices
> > > > without VIRTIO_F_IOMMU_PLATFORM.  
> > > 
> > > I agree it's a bit misleading. Protection is enforced by memory
> > > encryption, you can't trust the hypervisor to report the bit correctly
> > > so using that as a securoty measure would be pointless.
> > > The real gain here is that broken configs are easier to
> > > debug.
> > > 
> > > Here's an attempt at a better description:
> > > 
> > > 	On some architectures, guest knows that VIRTIO_F_IOMMU_PLATFORM is
> > > 	required for virtio to function: e.g. this is the case on s390 protected
> > > 	virt guests, since otherwise guest passes encrypted guest memory to devices,
> > > 	which the device can't read. Without VIRTIO_F_IOMMU_PLATFORM the
> > > 	result is that affected memory (or even a whole page containing
> > > 	it is corrupted). Detect and fail probe instead - that is easier
> > > 	to debug.  

s/guest/the guest/ (x2)

> > 
> > Thanks indeed better aside from the "encrypted guest memory": the mechanism
> > used to avoid the access to the guest memory from the host by s390 is not
> > encryption but a hardware feature denying the general host access and
> > allowing pieces of memory to be shared between guest and host.  
> 
> s/encrypted/protected/
> 
> > As a consequence the data read from memory is not corrupted but not read at
> > all and the read error kills the hypervizor with a SIGSEGV.  
> 
> s/(or even a whole page containing it is corrupted)/can not be
> 	read and the read error kills the hypervizor with a SIGSEGV/

s/hypervizor/hypervisor/

> 
> 
> As an aside, we could maybe handle that more gracefully
> on the hypervisor side.
> 
> >   
> > > 
> > > however, now that we have described what it is (hypervisor
> > > misconfiguration) I ask a question: can we be sure this will never
> > > ever work? E.g. what if some future hypervisor gains ability to
> > > access the protected guest memory in some abstractly secure manner?  
> > 
> > The goal of the s390 PV feature is to avoid this possibility so I don't
> > think so; however, there is a possibility that some hardware VIRTIO device
> > gain access to the guest's protected memory, even such device does not exist
> > yet.
> > 
> > At the moment such device exists we will need a driver for it, at least to
> > enable the feature and apply policies, it is also one of the reasons why a
> > hook to the architecture is interesting.  
> 
> 
> Not neessarily, it could also be fully transparent. See e.g.
> recent AMD andvances allowing unmodified guests with SEV.

I guess it depends on the architecture's protection mechanism and
threat model whether this makes sense.

> 
> 
> > > We are blocking this here, and it's hard to predict the future,
> > > and a broken hypervisor can always find ways to crash the guest ...  
> > 
> > yes, this is also something to fix on the hypervizor side, Halil is working
> > on it.
> >   
> > > 
> > > IMHO it would be safer to just print a warning.
> > > What do you think?  
> > 
> > Sadly, putting a warning may not help as qemu is killed if it accesses the
> > protected memory.
> > Also note that the crash occurs not only on start but also on hotplug.

Failing to start a guest is not that bad IMHO, but crashing a guest
that is running perfectly fine is. I vote for just failing the probe if
preconditions are not met.

> > 
> > Thanks,
> > Pierre  
> 
> Well that depends on where does the warning go. If it's on a serial port
> it might be reported host side before the crash triggers.  But
> interesting point generally. How about a feature to send a warning code
> or string to host then?

I would generally expect a guest warning to stay on the guest side --
especially as the host admin and the guest admin may be different
persons. So having a general way to send an alert to from a guest to
the host is not uninteresting, although we need to be careful to avoid
the guest being able to DOS the host.

Pierre Morel July 2, 2020, 1:03 p.m. UTC | #20

On 2020-06-29 18:05, Cornelia Huck wrote:
> On Mon, 29 Jun 2020 11:57:14 -0400
> "Michael S. Tsirkin" <[e-mail]> wrote:
> 
>> On Wed, Jun 17, 2020 at 12:43:57PM +0200, Pierre Morel wrote:
>>> An architecture protecting the guest memory against unauthorized host
>>> access may want to enforce VIRTIO I/O device protection through the
>>> use of VIRTIO_F_IOMMU_PLATFORM.
>>>
>>> Let's give a chance to the architecture to accept or not devices
>>> without VIRTIO_F_IOMMU_PLATFORM.
>>>
>>> Signed-off-by: Pierre Morel <[e-mail]>
>>> Acked-by: Jason Wang <[e-mail]>
>>> Acked-by: Christian Borntraeger <[e-mail]>
>>> ---
>>>   arch/s390/mm/init.c     |  6 ++++++
>>>   drivers/virtio/virtio.c | 22 ++++++++++++++++++++++
>>>   include/linux/virtio.h  |  2 ++
>>>   3 files changed, 30 insertions(+)
> 
>>> @@ -179,6 +194,13 @@ int virtio_finalize_features(struct virtio_device *dev)
>>>   	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>>>   		return 0;
>>>   
>>> +	if (arch_needs_virtio_iommu_platform(dev) &&
>>> +		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
>>> +		dev_warn(&dev->dev,
>>> +			 "virtio: device must provide VIRTIO_F_IOMMU_PLATFORM\n");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>>   	virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
>>>   	status = dev->config->get_status(dev);
>>>   	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
>>
>> Well don't you need to check it *before* VIRTIO_F_VERSION_1, not after?
> 
> But it's only available with VERSION_1 anyway, isn't it? So it probably
> also needs to fail when this feature is needed if VERSION_1 has not been
> negotiated, I think.
> 

Yes, clearly, I will add this.

Thanks,
Pierre

Pierre Morel July 6, 2020, 1:37 p.m. UTC | #21

On 2020-07-02 15:03, Pierre Morel wrote:
> 
> 
> On 2020-06-29 18:05, Cornelia Huck wrote:
>> On Mon, 29 Jun 2020 11:57:14 -0400
>> "Michael S. Tsirkin" <[e-mail]> wrote:
>>
>>> On Wed, Jun 17, 2020 at 12:43:57PM +0200, Pierre Morel wrote:
>>>> An architecture protecting the guest memory against unauthorized host
>>>> access may want to enforce VIRTIO I/O device protection through the
>>>> use of VIRTIO_F_IOMMU_PLATFORM.
>>>>
>>>> Let's give a chance to the architecture to accept or not devices
>>>> without VIRTIO_F_IOMMU_PLATFORM.
>>>>
>>>> Signed-off-by: Pierre Morel <[e-mail]>
>>>> Acked-by: Jason Wang <[e-mail]>
>>>> Acked-by: Christian Borntraeger <[e-mail]>
>>>> ---
>>>>   arch/s390/mm/init.c     |  6 ++++++
>>>>   drivers/virtio/virtio.c | 22 ++++++++++++++++++++++
>>>>   include/linux/virtio.h  |  2 ++
>>>>   3 files changed, 30 insertions(+)
>>
>>>> @@ -179,6 +194,13 @@ int virtio_finalize_features(struct 
>>>> virtio_device *dev)
>>>>       if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>>>>           return 0;
>>>> +    if (arch_needs_virtio_iommu_platform(dev) &&
>>>> +        !virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
>>>> +        dev_warn(&dev->dev,
>>>> +             "virtio: device must provide VIRTIO_F_IOMMU_PLATFORM\n");
>>>> +        return -ENODEV;
>>>> +    }
>>>> +
>>>>       virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
>>>>       status = dev->config->get_status(dev);
>>>>       if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
>>>
>>> Well don't you need to check it *before* VIRTIO_F_VERSION_1, not after?
>>
>> But it's only available with VERSION_1 anyway, isn't it? So it probably
>> also needs to fail when this feature is needed if VERSION_1 has not been
>> negotiated, I think.


would be something like:

-       if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
-               return 0;
+       if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
+               ret = arch_accept_virtio_features(dev);
+               if (ret)
+                       dev_warn(&dev->dev,
+                                "virtio: device must provide 
VIRTIO_F_VERSION_1\n");
+               return ret;
+       }


just a thought on the function name:
It becomes more general than just IOMMU_PLATFORM related.

What do you think of:

arch_accept_virtio_features()

?

Regards,
Pierre

Cornelia Huck July 6, 2020, 2:33 p.m. UTC | #22

On Mon, 6 Jul 2020 15:37:37 +0200
Pierre Morel <[e-mail]> wrote:

> On 2020-07-02 15:03, Pierre Morel wrote:
> > 
> > 
> > On 2020-06-29 18:05, Cornelia Huck wrote:  
> >> On Mon, 29 Jun 2020 11:57:14 -0400
> >> "Michael S. Tsirkin" <[e-mail]> wrote:
> >>  
> >>> On Wed, Jun 17, 2020 at 12:43:57PM +0200, Pierre Morel wrote:  
> >>>> An architecture protecting the guest memory against unauthorized host
> >>>> access may want to enforce VIRTIO I/O device protection through the
> >>>> use of VIRTIO_F_IOMMU_PLATFORM.
> >>>>
> >>>> Let's give a chance to the architecture to accept or not devices
> >>>> without VIRTIO_F_IOMMU_PLATFORM.
> >>>>
> >>>> Signed-off-by: Pierre Morel <[e-mail]>
> >>>> Acked-by: Jason Wang <[e-mail]>
> >>>> Acked-by: Christian Borntraeger <[e-mail]>
> >>>> ---
> >>>>   arch/s390/mm/init.c     |  6 ++++++
> >>>>   drivers/virtio/virtio.c | 22 ++++++++++++++++++++++
> >>>>   include/linux/virtio.h  |  2 ++
> >>>>   3 files changed, 30 insertions(+)  
> >>  
> >>>> @@ -179,6 +194,13 @@ int virtio_finalize_features(struct 
> >>>> virtio_device *dev)
> >>>>       if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
> >>>>           return 0;
> >>>> +    if (arch_needs_virtio_iommu_platform(dev) &&
> >>>> +        !virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> >>>> +        dev_warn(&dev->dev,
> >>>> +             "virtio: device must provide VIRTIO_F_IOMMU_PLATFORM\n");
> >>>> +        return -ENODEV;
> >>>> +    }
> >>>> +
> >>>>       virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
> >>>>       status = dev->config->get_status(dev);
> >>>>       if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {  
> >>>
> >>> Well don't you need to check it *before* VIRTIO_F_VERSION_1, not after?  
> >>
> >> But it's only available with VERSION_1 anyway, isn't it? So it probably
> >> also needs to fail when this feature is needed if VERSION_1 has not been
> >> negotiated, I think.  
> 
> 
> would be something like:
> 
> -       if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
> -               return 0;
> +       if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> +               ret = arch_accept_virtio_features(dev);
> +               if (ret)
> +                       dev_warn(&dev->dev,
> +                                "virtio: device must provide 
> VIRTIO_F_VERSION_1\n");
> +               return ret;
> +       }

That looks wrong; I think we want to validate in all cases. What about:

ret = arch_accept_virtio_features(dev); // this can include checking for
                                        // older or newer features
if (ret)
	// assume that the arch callback moaned already
	return ret;

if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
	return 0;

// do the virtio-1 only FEATURES_OK dance

> 
> 
> just a thought on the function name:
> It becomes more general than just IOMMU_PLATFORM related.
> 
> What do you think of:
> 
> arch_accept_virtio_features()

Or maybe arch_validate_virtio_features()?

> 
> ?
> 
> Regards,
> Pierre
> 
>

Pierre Morel July 6, 2020, 3:01 p.m. UTC | #23

On 2020-07-06 16:33, Cornelia Huck wrote:
> On Mon, 6 Jul 2020 15:37:37 +0200
> Pierre Morel <[e-mail]> wrote:
> 
>> On 2020-07-02 15:03, Pierre Morel wrote:
>>>
>>>
>>> On 2020-06-29 18:05, Cornelia Huck wrote:
>>>> On Mon, 29 Jun 2020 11:57:14 -0400
>>>> "Michael S. Tsirkin" <[e-mail]> wrote:
>>>>   
>>>>> On Wed, Jun 17, 2020 at 12:43:57PM +0200, Pierre Morel wrote:
>>>>>> An architecture protecting the guest memory against unauthorized host
>>>>>> access may want to enforce VIRTIO I/O device protection through the
>>>>>> use of VIRTIO_F_IOMMU_PLATFORM.
>>>>>>
>>>>>> Let's give a chance to the architecture to accept or not devices
>>>>>> without VIRTIO_F_IOMMU_PLATFORM.
>>>>>>
>>>>>> Signed-off-by: Pierre Morel <[e-mail]>
>>>>>> Acked-by: Jason Wang <[e-mail]>
>>>>>> Acked-by: Christian Borntraeger <[e-mail]>
>>>>>> ---
>>>>>>    arch/s390/mm/init.c     |  6 ++++++
>>>>>>    drivers/virtio/virtio.c | 22 ++++++++++++++++++++++
>>>>>>    include/linux/virtio.h  |  2 ++
>>>>>>    3 files changed, 30 insertions(+)
>>>>   
>>>>>> @@ -179,6 +194,13 @@ int virtio_finalize_features(struct
>>>>>> virtio_device *dev)
>>>>>>        if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>>>>>>            return 0;
>>>>>> +    if (arch_needs_virtio_iommu_platform(dev) &&
>>>>>> +        !virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
>>>>>> +        dev_warn(&dev->dev,
>>>>>> +             "virtio: device must provide VIRTIO_F_IOMMU_PLATFORM\n");
>>>>>> +        return -ENODEV;
>>>>>> +    }
>>>>>> +
>>>>>>        virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
>>>>>>        status = dev->config->get_status(dev);
>>>>>>        if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
>>>>>
>>>>> Well don't you need to check it *before* VIRTIO_F_VERSION_1, not after?
>>>>
>>>> But it's only available with VERSION_1 anyway, isn't it? So it probably
>>>> also needs to fail when this feature is needed if VERSION_1 has not been
>>>> negotiated, I think.
>>
>>
>> would be something like:
>>
>> -       if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
>> -               return 0;
>> +       if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
>> +               ret = arch_accept_virtio_features(dev);
>> +               if (ret)
>> +                       dev_warn(&dev->dev,
>> +                                "virtio: device must provide
>> VIRTIO_F_VERSION_1\n");
>> +               return ret;
>> +       }
> 
> That looks wrong; I think we want to validate in all cases. What about:
> 
> ret = arch_accept_virtio_features(dev); // this can include checking for
>                                          // older or newer features
> if (ret)
> 	// assume that the arch callback moaned already
> 	return ret;
> 
> if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
> 	return 0;
> 
> // do the virtio-1 only FEATURES_OK dance

hum, you are right, I was too focused on keeping my simple 
arch_accept_virtio_features() function unchanged.
It must be more general.

> 
>>
>>
>> just a thought on the function name:
>> It becomes more general than just IOMMU_PLATFORM related.
>>
>> What do you think of:
>>
>> arch_accept_virtio_features()
> 
> Or maybe arch_validate_virtio_features()?

OK validated.

Thanks,
Pierre

Patch

diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
index 6dc7c3b60ef6..215070c03226 100644
--- a/arch/s390/mm/init.c
+++ b/arch/s390/mm/init.c
@@ -45,6 +45,7 @@ 
 #include <asm/kasan.h>
 #include <asm/dma-mapping.h>
 #include <asm/uv.h>
+#include <linux/virtio.h>
 
 pgd_t swapper_pg_dir[PTRS_PER_PGD] __section(.bss..swapper_pg_dir);
 
@@ -161,6 +162,11 @@  bool force_dma_unencrypted(struct device *dev)
 	return is_prot_virt_guest();
 }
 
+int arch_needs_virtio_iommu_platform(struct virtio_device *dev)
+{
+	return is_prot_virt_guest();
+}
+
 /* protected virtualization */
 static void pv_init(void)
 {
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index a977e32a88f2..aa8e01104f86 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -167,6 +167,21 @@  void virtio_add_status(struct virtio_device *dev, unsigned int status)
 }
 EXPORT_SYMBOL_GPL(virtio_add_status);
 
+/*
+ * arch_needs_virtio_iommu_platform - provide arch specific hook when finalizing
+ *				      features for VIRTIO device dev
+ * @dev: the VIRTIO device being added
+ *
+ * Permits the platform to provide architecture specific functionality when
+ * devices features are finalized. This is the default implementation.
+ * Architecture implementations can override this.
+ */
+
+int __weak arch_needs_virtio_iommu_platform(struct virtio_device *dev)
+{
+	return 0;
+}
+
 int virtio_finalize_features(struct virtio_device *dev)
 {
 	int ret = dev->config->finalize_features(dev);
@@ -179,6 +194,13 @@  int virtio_finalize_features(struct virtio_device *dev)
 	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
 		return 0;
 
+	if (arch_needs_virtio_iommu_platform(dev) &&
+		!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
+		dev_warn(&dev->dev,
+			 "virtio: device must provide VIRTIO_F_IOMMU_PLATFORM\n");
+		return -ENODEV;
+	}
+
 	virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
 	status = dev->config->get_status(dev);
 	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index a493eac08393..e8526ae3463e 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -195,4 +195,6 @@  void unregister_virtio_driver(struct virtio_driver *drv);
 #define module_virtio_driver(__virtio_driver) \
 	module_driver(__virtio_driver, register_virtio_driver, \
 			unregister_virtio_driver)
+
+int arch_needs_virtio_iommu_platform(struct virtio_device *dev);
 #endif /* _LINUX_VIRTIO_H */