Linux Virtualization - Aug 2020 - [PATCH v1 0/1] s390: virtio-ccw: PV needs VIRTIO I/O device protection

Hi all,

In another series I proposed to add an architecture specific
callback to fail feature negociation on architecture need.

In VIRTIO, we already have an entry to reject the features on the
transport basis.

Transport is not architecture so I send a separate series in which
we fail the feature negociation inside virtio_ccw_finalize_features,
the virtio_config_ops.finalize_features for S390 CCW transport,
when the device do not propose the VIRTIO_F_IOMMU_PLATFORM.

This solves the problem of crashing QEMU when this one is not using
a CCW device with iommu_platform=on in S390.

Regards,
Pierre

Regards,
Pierre

Pierre Morel (1):
  s390: virtio-ccw: PV needs VIRTIO I/O device protection

 drivers/s390/virtio/virtio_ccw.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

-- 
2.25.1

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 ccw_transport_features() to fail feature negociation
and consequently probe if that's not the case, preventing a host
error on access attempt.

Signed-off-by: Pierre Morel <pmorel at linux.ibm.com>
---
 drivers/s390/virtio/virtio_ccw.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index 5730572b52cd..cc8d8064c6c4 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -803,11 +803,23 @@ static u64 virtio_ccw_get_features(struct virtio_device
*vdev)
 	return rc;
 }
 
-static void ccw_transport_features(struct virtio_device *vdev)
+static int ccw_transport_features(struct virtio_device *vdev)
 {
-	/*
-	 * Currently nothing to do here.
-	 */
+	if (!is_prot_virt_guest())
+		return 0;
+
+	if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+		dev_warn(&vdev->dev,
+			 "device must provide VIRTIO_F_VERSION_1\n");
+		return -ENODEV;
+	}
+
+	if (!virtio_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
+		dev_warn(&vdev->dev,
+			 "device must provide VIRTIO_F_IOMMU_PLATFORM\n");
+		return -ENODEV;
+	}
+	return 0;
 }
 
 static int virtio_ccw_finalize_features(struct virtio_device *vdev)
@@ -837,7 +849,9 @@ static int virtio_ccw_finalize_features(struct virtio_device
*vdev)
 	vring_transport_features(vdev);
 
 	/* Give virtio_ccw a chance to accept features. */
-	ccw_transport_features(vdev);
+	ret = ccw_transport_features(vdev);
+	if (ret)
+		goto out_free;
 
 	features->index = 0;
 	features->features = cpu_to_le32((u32)vdev->features);
-- 
2.25.1

On Thu,  6 Aug 2020 16:23:01 +0200
Pierre Morel <pmorel at linux.ibm.com> wrote:
> Hi all,
> 
> In another series I proposed to add an architecture specific
> callback to fail feature negociation on architecture need.
> 
> In VIRTIO, we already have an entry to reject the features on the
> transport basis.
> 
> Transport is not architecture so I send a separate series in which
> we fail the feature negociation inside virtio_ccw_finalize_features,
> the virtio_config_ops.finalize_features for S390 CCW transport,
> when the device do not propose the VIRTIO_F_IOMMU_PLATFORM.
> 
> This solves the problem of crashing QEMU when this one is not using
> a CCW device with iommu_platform=on in S390.
This does work, and I'm tempted to queue this patch, but I'm wondering
whether we need to give up on a cross-architecture solution already
(especially keeping in mind that ccw is the only transport that is
really architecture-specific).

I know that we've gone through a few rounds already, and I'm not sure
whether we've been there already, but:

Could virtio_finalize_features() call an optional
arch_has_restricted_memory_access() function and do the enforcing of
IOMMU_PLATFORM? That would catch all transports, and things should work
once an architecture opts in. That direction also shouldn't be a
problem if virtio is a module.
> 
> Regards,
> Pierre
> 
> Regards,
> Pierre
> 
> Pierre Morel (1):
>   s390: virtio-ccw: PV needs VIRTIO I/O device protection
> 
>  drivers/s390/virtio/virtio_ccw.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
>

On 2020-08-06 17:47, Cornelia Huck wrote:
> On Thu,  6 Aug 2020 16:23:01 +0200
...
> This does work, and I'm tempted to queue this patch, but I'm
wondering
> whether we need to give up on a cross-architecture solution already
> (especially keeping in mind that ccw is the only transport that is
> really architecture-specific).
> 
> I know that we've gone through a few rounds already, and I'm not
sure
> whether we've been there already, but:
> 
> Could virtio_finalize_features() call an optional
> arch_has_restricted_memory_access() function and do the enforcing of
> IOMMU_PLATFORM? That would catch all transports, and things should work
> once an architecture opts in. That direction also shouldn't be a
> problem if virtio is a module.
Yes thanks, I rework it in this direction.


-- 
Pierre Morel
IBM Lab Boeblingen