Linux Virtualization - Jun 2020 - [PATCH v3 1/1] s390: virtio: let arch accept devices without IOMMU feature

On Wed, 17 Jun 2020 12:43:57 +0200
Pierre Morel <pmorel at linux.ibm.com> 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 <pasic at linux.ibm.com>

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

On Thu, 18 Jun 2020 00:29:56 +0200
Halil Pasic <pasic at linux.ibm.com> wrote:
> On Wed, 17 Jun 2020 12:43:57 +0200
> Pierre Morel <pmorel at linux.ibm.com> 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 <pasic at linux.ibm.com>
> 
> 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 <cohuck at redhat.com>

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.

On Fri, 19 Jun 2020 11:20:51 +0200
Cornelia Huck <cohuck at redhat.com> 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

On 2020-06-19 11:20, Cornelia Huck wrote:
> On Thu, 18 Jun 2020 00:29:56 +0200
> Halil Pasic <pasic at linux.ibm.com> wrote:
> 
>> On Wed, 17 Jun 2020 12:43:57 +0200
>> Pierre Morel <pmorel at linux.ibm.com> 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 <cohuck at redhat.com>
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
IBM Lab Boeblingen

On 2020-06-18 00:29, Halil Pasic wrote:
> On Wed, 17 Jun 2020 12:43:57 +0200
> Pierre Morel <pmorel at linux.ibm.com> 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 <pasic at linux.ibm.com>
Thanks,

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

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen