S390, protecting the guest memory against unauthorized host access
needs to enforce VIRTIO I/O device protection through the use of
VIRTIO_F_VERSION_1 and VIRTIO_F_IOMMU_PLATFORM.
Signed-off-by: Pierre Morel <[email protected]>
---
arch/s390/kernel/uv.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
index c296e5c8dbf9..106330f6eda1 100644
--- a/arch/s390/kernel/uv.c
+++ b/arch/s390/kernel/uv.c
@@ -14,6 +14,7 @@
#include <linux/memblock.h>
#include <linux/pagemap.h>
#include <linux/swap.h>
+#include <linux/virtio_config.h>
#include <asm/facility.h>
#include <asm/sections.h>
#include <asm/uv.h>
@@ -413,3 +414,27 @@ static int __init uv_info_init(void)
}
device_initcall(uv_info_init);
#endif
+
+/*
+ * arch_validate_virtio_iommu_platform
+ * @dev: the VIRTIO device being added
+ *
+ * Return value: returns -ENODEV if any features of the
+ * device breaks the protected virtualization
+ * 0 otherwise.
+ */
+int arch_validate_virtio_features(struct virtio_device *dev)
+{
+ if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
+ dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
+ return is_prot_virt_guest() ? -ENODEV : 0;
+ }
+
+ if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
+ dev_warn(&dev->dev,
+ "device must provide VIRTIO_F_IOMMU_PLATFORM\n");
+ return is_prot_virt_guest() ? -ENODEV : 0;
+ }
+
+ return 0;
+}
--
2.25.1
On Tue, 7 Jul 2020 10:44:37 +0200
Pierre Morel <[email protected]> wrote:
> S390, protecting the guest memory against unauthorized host access
> needs to enforce VIRTIO I/O device protection through the use of
> VIRTIO_F_VERSION_1 and VIRTIO_F_IOMMU_PLATFORM.
Hm... what about:
"If protected virtualization is active on s390, the virtio queues are
not accessible to the host, unless VIRTIO_F_IOMMU_PLATFORM has been
negotiated. Use the new arch_validate_virtio_features() interface to
enforce this."
>
> Signed-off-by: Pierre Morel <[email protected]>
> ---
> arch/s390/kernel/uv.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> index c296e5c8dbf9..106330f6eda1 100644
> --- a/arch/s390/kernel/uv.c
> +++ b/arch/s390/kernel/uv.c
> @@ -14,6 +14,7 @@
> #include <linux/memblock.h>
> #include <linux/pagemap.h>
> #include <linux/swap.h>
> +#include <linux/virtio_config.h>
> #include <asm/facility.h>
> #include <asm/sections.h>
> #include <asm/uv.h>
> @@ -413,3 +414,27 @@ static int __init uv_info_init(void)
> }
> device_initcall(uv_info_init);
> #endif
> +
> +/*
> + * arch_validate_virtio_iommu_platform
s/arch_validate_virtio_iommu_platform/arch_validate_virtio_features/
> + * @dev: the VIRTIO device being added
> + *
> + * Return value: returns -ENODEV if any features of the
> + * device breaks the protected virtualization
> + * 0 otherwise.
I don't think you need to specify the contract here: that belongs to
the definition in the virtio core. What about simply adding a sentence
"Return an error if required features are missing on a guest running
with protected virtualization." ?
> + */
> +int arch_validate_virtio_features(struct virtio_device *dev)
> +{
Maybe jump out immediately if the guest is not protected?
> + if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> + dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
> + return is_prot_virt_guest() ? -ENODEV : 0;
> + }
> +
> + if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> + dev_warn(&dev->dev,
> + "device must provide VIRTIO_F_IOMMU_PLATFORM\n");
> + return is_prot_virt_guest() ? -ENODEV : 0;
> + }
if (!is_prot_virt_guest())
return 0;
if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
dev_warn(&dev->dev,
"legacy virtio is incompatible with protected guests");
return -ENODEV;
}
if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
dev_warn(&dev->dev,
"device does not work with limited memory access in protected guests");
return -ENODEV;
}
> +
> + return 0;
> +}
On 2020-07-07 11:46, Cornelia Huck wrote:
> On Tue, 7 Jul 2020 10:44:37 +0200
> Pierre Morel <[email protected]> wrote:
>
>> S390, protecting the guest memory against unauthorized host access
>> needs to enforce VIRTIO I/O device protection through the use of
>> VIRTIO_F_VERSION_1 and VIRTIO_F_IOMMU_PLATFORM.
>
> Hm... what about:
>
> "If protected virtualization is active on s390, the virtio queues are
> not accessible to the host, unless VIRTIO_F_IOMMU_PLATFORM has been
> negotiated. Use the new arch_validate_virtio_features() interface to
> enforce this."
Yes, thanks.
>
>>
>> Signed-off-by: Pierre Morel <[email protected]>
>> ---
>> arch/s390/kernel/uv.c | 25 +++++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
>> index c296e5c8dbf9..106330f6eda1 100644
>> --- a/arch/s390/kernel/uv.c
>> +++ b/arch/s390/kernel/uv.c
>> @@ -14,6 +14,7 @@
>> #include <linux/memblock.h>
>> #include <linux/pagemap.h>
>> #include <linux/swap.h>
>> +#include <linux/virtio_config.h>
>> #include <asm/facility.h>
>> #include <asm/sections.h>
>> #include <asm/uv.h>
>> @@ -413,3 +414,27 @@ static int __init uv_info_init(void)
>> }
>> device_initcall(uv_info_init);
>> #endif
>> +
>> +/*
>> + * arch_validate_virtio_iommu_platform
>
> s/arch_validate_virtio_iommu_platform/arch_validate_virtio_features/
>
>> + * @dev: the VIRTIO device being added
>> + *
>> + * Return value: returns -ENODEV if any features of the
>> + * device breaks the protected virtualization
>> + * 0 otherwise.
>
> I don't think you need to specify the contract here: that belongs to
> the definition in the virtio core. What about simply adding a sentence
> "Return an error if required features are missing on a guest running
> with protected virtualization." ?
OK, right.
>
>> + */
>> +int arch_validate_virtio_features(struct virtio_device *dev)
>> +{
>
> Maybe jump out immediately if the guest is not protected?
>
>> + if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
>> + dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
>> + return is_prot_virt_guest() ? -ENODEV : 0;
>> + }
>> +
>> + if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
>> + dev_warn(&dev->dev,
>> + "device must provide VIRTIO_F_IOMMU_PLATFORM\n");
>> + return is_prot_virt_guest() ? -ENODEV : 0;
>> + }
>
> if (!is_prot_virt_guest())
> return 0;
>
> if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> dev_warn(&dev->dev,
> "legacy virtio is incompatible with protected guests");
> return -ENODEV;
> }
>
> if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> dev_warn(&dev->dev,
> "device does not work with limited memory access in protected guests");
> return -ENODEV;
> }
Yes, easier to read.
Thanks,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
On 07.07.20 10:44, Pierre Morel wrote:
> S390, protecting the guest memory against unauthorized host access
> needs to enforce VIRTIO I/O device protection through the use of
> VIRTIO_F_VERSION_1 and VIRTIO_F_IOMMU_PLATFORM.
>
> Signed-off-by: Pierre Morel <[email protected]>
> ---
> arch/s390/kernel/uv.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> index c296e5c8dbf9..106330f6eda1 100644
> --- a/arch/s390/kernel/uv.c
> +++ b/arch/s390/kernel/uv.c
> @@ -14,6 +14,7 @@
> #include <linux/memblock.h>
> #include <linux/pagemap.h>
> #include <linux/swap.h>
> +#include <linux/virtio_config.h>
> #include <asm/facility.h>
> #include <asm/sections.h>
> #include <asm/uv.h>
> @@ -413,3 +414,27 @@ static int __init uv_info_init(void)
> }
> device_initcall(uv_info_init);
> #endif
> +
> +/*
> + * arch_validate_virtio_iommu_platform
> + * @dev: the VIRTIO device being added
> + *
> + * Return value: returns -ENODEV if any features of the
> + * device breaks the protected virtualization
> + * 0 otherwise.
> + */
> +int arch_validate_virtio_features(struct virtio_device *dev)
> +{
> + if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> + dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
I think you only want to warn if is_prot_virt_guest is true? We certainly
want to be able to run as a guest of older hypervisors with virtio 0.95, no?
> + return is_prot_virt_guest() ? -ENODEV : 0;
> + }
> +
> + if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> + dev_warn(&dev->dev,
> + "device must provide VIRTIO_F_IOMMU_PLATFORM\n");
same here.
> + return is_prot_virt_guest() ? -ENODEV : 0;
> + }
> +
> + return 0;
> +}
>
On 2020-07-07 13:12, Christian Borntraeger wrote:
>
>
> On 07.07.20 10:44, Pierre Morel wrote:
>> S390, protecting the guest memory against unauthorized host access
>> needs to enforce VIRTIO I/O device protection through the use of
>> VIRTIO_F_VERSION_1 and VIRTIO_F_IOMMU_PLATFORM.
>>
>> Signed-off-by: Pierre Morel <[email protected]>
>> ---
>> arch/s390/kernel/uv.c | 25 +++++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
>> index c296e5c8dbf9..106330f6eda1 100644
>> --- a/arch/s390/kernel/uv.c
>> +++ b/arch/s390/kernel/uv.c
>> @@ -14,6 +14,7 @@
>> #include <linux/memblock.h>
>> #include <linux/pagemap.h>
>> #include <linux/swap.h>
>> +#include <linux/virtio_config.h>
>> #include <asm/facility.h>
>> #include <asm/sections.h>
>> #include <asm/uv.h>
>> @@ -413,3 +414,27 @@ static int __init uv_info_init(void)
>> }
>> device_initcall(uv_info_init);
>> #endif
>> +
>> +/*
>> + * arch_validate_virtio_iommu_platform
>> + * @dev: the VIRTIO device being added
>> + *
>> + * Return value: returns -ENODEV if any features of the
>> + * device breaks the protected virtualization
>> + * 0 otherwise.
>> + */
>> +int arch_validate_virtio_features(struct virtio_device *dev)
>> +{
>> + if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
>> + dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
>
> I think you only want to warn if is_prot_virt_guest is true? We certainly
> want to be able to run as a guest of older hypervisors with virtio 0.95, no?
clear, yes.
I will first check for PV as Connie sugested.
>
>
>> + return is_prot_virt_guest() ? -ENODEV : 0;
>> + }
>> +
>> + if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
>> + dev_warn(&dev->dev,
>> + "device must provide VIRTIO_F_IOMMU_PLATFORM\n");
>
> same here.
Yes,
Thanks,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
On Tue, Jul 07, 2020 at 11:46:33AM +0200, Cornelia Huck wrote:
> On Tue, 7 Jul 2020 10:44:37 +0200
> Pierre Morel <[email protected]> wrote:
>
> > S390, protecting the guest memory against unauthorized host access
> > needs to enforce VIRTIO I/O device protection through the use of
> > VIRTIO_F_VERSION_1 and VIRTIO_F_IOMMU_PLATFORM.
>
> Hm... what about:
>
> "If protected virtualization is active on s390, the virtio queues are
> not accessible to the host, unless VIRTIO_F_IOMMU_PLATFORM has been
> negotiated. Use the new arch_validate_virtio_features() interface to
> enforce this."
s/enforce this/fail probe if that's not the case, preventing a host error on access attempt/
> >
> > Signed-off-by: Pierre Morel <[email protected]>
> > ---
> > arch/s390/kernel/uv.c | 25 +++++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> >
> > diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> > index c296e5c8dbf9..106330f6eda1 100644
> > --- a/arch/s390/kernel/uv.c
> > +++ b/arch/s390/kernel/uv.c
> > @@ -14,6 +14,7 @@
> > #include <linux/memblock.h>
> > #include <linux/pagemap.h>
> > #include <linux/swap.h>
> > +#include <linux/virtio_config.h>
> > #include <asm/facility.h>
> > #include <asm/sections.h>
> > #include <asm/uv.h>
> > @@ -413,3 +414,27 @@ static int __init uv_info_init(void)
> > }
> > device_initcall(uv_info_init);
> > #endif
> > +
> > +/*
> > + * arch_validate_virtio_iommu_platform
>
> s/arch_validate_virtio_iommu_platform/arch_validate_virtio_features/
>
> > + * @dev: the VIRTIO device being added
> > + *
> > + * Return value: returns -ENODEV if any features of the
> > + * device breaks the protected virtualization
> > + * 0 otherwise.
>
> I don't think you need to specify the contract here: that belongs to
> the definition in the virtio core. What about simply adding a sentence
> "Return an error if required features are missing on a guest running
> with protected virtualization." ?
>
> > + */
> > +int arch_validate_virtio_features(struct virtio_device *dev)
> > +{
>
> Maybe jump out immediately if the guest is not protected?
>
> > + if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> > + dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
> > + return is_prot_virt_guest() ? -ENODEV : 0;
> > + }
> > +
> > + if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> > + dev_warn(&dev->dev,
> > + "device must provide VIRTIO_F_IOMMU_PLATFORM\n");
> > + return is_prot_virt_guest() ? -ENODEV : 0;
> > + }
>
> if (!is_prot_virt_guest())
> return 0;
>
> if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> dev_warn(&dev->dev,
> "legacy virtio is incompatible with protected guests");
> return -ENODEV;
> }
>
> if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> dev_warn(&dev->dev,
> "device does not work with limited memory access in protected guests");
> return -ENODEV;
> }
>
> > +
> > + return 0;
> > +}
On 2020-07-07 13:14, Michael S. Tsirkin wrote:
> On Tue, Jul 07, 2020 at 11:46:33AM +0200, Cornelia Huck wrote:
>> On Tue, 7 Jul 2020 10:44:37 +0200
>> Pierre Morel <[email protected]> wrote:
>>
>>> S390, protecting the guest memory against unauthorized host access
>>> needs to enforce VIRTIO I/O device protection through the use of
>>> VIRTIO_F_VERSION_1 and VIRTIO_F_IOMMU_PLATFORM.
>>
>> Hm... what about:
>>
>> "If protected virtualization is active on s390, the virtio queues are
>> not accessible to the host, unless VIRTIO_F_IOMMU_PLATFORM has been
>> negotiated. Use the new arch_validate_virtio_features() interface to
>> enforce this."
>
> s/enforce this/fail probe if that's not the case, preventing a host error on access attempt/
>
yes, more complete, thanks.
regards,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
On Tue, 7 Jul 2020 12:38:17 +0200
Pierre Morel <[email protected]> wrote:
>
>
> On 2020-07-07 11:46, Cornelia Huck wrote:
> > On Tue, 7 Jul 2020 10:44:37 +0200
> > Pierre Morel <[email protected]> wrote:
> >
> >> S390, protecting the guest memory against unauthorized host access
> >> needs to enforce VIRTIO I/O device protection through the use of
> >> VIRTIO_F_VERSION_1 and VIRTIO_F_IOMMU_PLATFORM.
> >
> > Hm... what about:
> >
> > "If protected virtualization is active on s390, the virtio queues are
> > not accessible to the host, unless VIRTIO_F_IOMMU_PLATFORM has been
> > negotiated. Use the new arch_validate_virtio_features() interface to
> > enforce this."
>
> Yes, thanks.
>
>
> >
> >>
> >> Signed-off-by: Pierre Morel <[email protected]>
> >> ---
> >> arch/s390/kernel/uv.c | 25 +++++++++++++++++++++++++
Is this the right place to put this stuff? This file seems to be about
implementing the interface for interacting with the ultravisor. I would
rather expect something like arch/s390/kernel/virtio.c
Should we ever get arch hooks for balloon those could go in
arch/s390/kernel/virtio.c as well.
> >> 1 file changed, 25 insertions(+)
> >>
> >> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> >> index c296e5c8dbf9..106330f6eda1 100644
> >> --- a/arch/s390/kernel/uv.c
> >> +++ b/arch/s390/kernel/uv.c
> >> @@ -14,6 +14,7 @@
> >> #include <linux/memblock.h>
> >> #include <linux/pagemap.h>
> >> #include <linux/swap.h>
> >> +#include <linux/virtio_config.h>
> >> #include <asm/facility.h>
> >> #include <asm/sections.h>
> >> #include <asm/uv.h>
> >> @@ -413,3 +414,27 @@ static int __init uv_info_init(void)
> >> }
> >> device_initcall(uv_info_init);
> >> #endif
> >> +
> >> +/*
> >> + * arch_validate_virtio_iommu_platform
> >
> > s/arch_validate_virtio_iommu_platform/arch_validate_virtio_features/
> >
> >> + * @dev: the VIRTIO device being added
> >> + *
> >> + * Return value: returns -ENODEV if any features of the
> >> + * device breaks the protected virtualization
> >> + * 0 otherwise.
> >
> > I don't think you need to specify the contract here: that belongs to
> > the definition in the virtio core. What about simply adding a sentence
> > "Return an error if required features are missing on a guest running
> > with protected virtualization." ?
>
> OK, right.
>
> >
> >> + */
> >> +int arch_validate_virtio_features(struct virtio_device *dev)
> >> +{
> >
> > Maybe jump out immediately if the guest is not protected?
> >
> >> + if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> >> + dev_warn(&dev->dev, "device must provide VIRTIO_F_VERSION_1\n");
> >> + return is_prot_virt_guest() ? -ENODEV : 0;
> >> + }
> >> +
> >> + if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> >> + dev_warn(&dev->dev,
> >> + "device must provide VIRTIO_F_IOMMU_PLATFORM\n");
> >> + return is_prot_virt_guest() ? -ENODEV : 0;
> >> + }
> >
> > if (!is_prot_virt_guest())
> > return 0;
> >
> > if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
> > dev_warn(&dev->dev,
> > "legacy virtio is incompatible with protected guests");
> > return -ENODEV;
> > }
> >
> > if (!virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) {
> > dev_warn(&dev->dev,
> > "device does not work with limited memory access in protected guests");
> > return -ENODEV;
> > }
>
> Yes, easier to read.
>
Not only easier to read but does not produce warnings
if !is_prot_virt_guest(). I strongly prefer the variant proposed by
Connie.
Otherwise LGTM.
Regards,
Halil