Virtualization - Jun 2021 - [PATCH v1 1/8] virtio: Force only split mode with protected guest

When running under TDX the virtio host is untrusted. The bulk
of the kernel memory is encrypted and protected, but the virtio
ring is in special shared memory that is shared with the
untrusted host.

This means virtio needs to be hardened against any attacks from
the host through the ring. Of course it's impossible to prevent DOS
(the host can chose at any time to stop doing IO), but there
should be no buffer overruns or similar that might give access to
any private memory in the guest.

virtio has a lot of modes, most are difficult to harden.

The best for hardening seems to be split mode without indirect
descriptors. This also simplifies the hardening job because
it's only a single code path.

Only allow split mode when in a protected guest. Followon
patches harden the split mode code paths, and we don't want
an malicious host to force anything else. Also disallow
indirect mode for similar reasons.

Signed-off-by: Andi Kleen <ak at linux.intel.com>
---
 drivers/virtio/virtio_ring.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 71e16b53e9c1..f35629fa47b1 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/hrtimer.h>
 #include <linux/dma-mapping.h>
+#include <linux/protected_guest.h>
 #include <xen/xen.h>
 
 #ifdef DEBUG
@@ -2221,8 +2222,16 @@ void vring_transport_features(struct virtio_device *vdev)
 	unsigned int i;
 
 	for (i = VIRTIO_TRANSPORT_F_START; i < VIRTIO_TRANSPORT_F_END; i++) {
+
+		/*
+		 * In protected guest mode disallow packed or indirect
+		 * because they ain't hardened.
+		 */
+
 		switch (i) {
 		case VIRTIO_RING_F_INDIRECT_DESC:
+			if (protected_guest_has(VM_MEM_ENCRYPT))
+				goto clear;
 			break;
 		case VIRTIO_RING_F_EVENT_IDX:
 			break;
@@ -2231,9 +2240,12 @@ void vring_transport_features(struct virtio_device *vdev)
 		case VIRTIO_F_ACCESS_PLATFORM:
 			break;
 		case VIRTIO_F_RING_PACKED:
+			if (protected_guest_has(VM_MEM_ENCRYPT))
+				goto clear;
 			break;
 		case VIRTIO_F_ORDER_PLATFORM:
 			break;
+		clear:
 		default:
 			/* We don't understand this bit. */
 			__virtio_clear_bit(vdev, i);
-- 
2.25.4

? 2021/6/3 ??8:41, Andi Kleen ??:
> When running under TDX the virtio host is untrusted. The bulk
> of the kernel memory is encrypted and protected, but the virtio
> ring is in special shared memory that is shared with the
> untrusted host.
>
> This means virtio needs to be hardened against any attacks from
> the host through the ring. Of course it's impossible to prevent DOS
> (the host can chose at any time to stop doing IO), but there
> should be no buffer overruns or similar that might give access to
> any private memory in the guest.
>
> virtio has a lot of modes, most are difficult to harden.
>
> The best for hardening seems to be split mode without indirect
> descriptors. This also simplifies the hardening job because
> it's only a single code path.
>
> Only allow split mode when in a protected guest. Followon
> patches harden the split mode code paths, and we don't want
> an malicious host to force anything else. Also disallow
> indirect mode for similar reasons.
>
> Signed-off-by: Andi Kleen <ak at linux.intel.com>
> ---
>   drivers/virtio/virtio_ring.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 71e16b53e9c1..f35629fa47b1 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -11,6 +11,7 @@
>   #include <linux/module.h>
>   #include <linux/hrtimer.h>
>   #include <linux/dma-mapping.h>
> +#include <linux/protected_guest.h>
>   #include <xen/xen.h>
>   
>   #ifdef DEBUG
> @@ -2221,8 +2222,16 @@ void vring_transport_features(struct virtio_device
*vdev)
>   	unsigned int i;
>   
>   	for (i = VIRTIO_TRANSPORT_F_START; i < VIRTIO_TRANSPORT_F_END; i++) {
> +
> +		/*
> +		 * In protected guest mode disallow packed or indirect
> +		 * because they ain't hardened.
> +		 */
> +
>   		switch (i) {
>   		case VIRTIO_RING_F_INDIRECT_DESC:
> +			if (protected_guest_has(VM_MEM_ENCRYPT))
> +				goto clear;

So we will see huge performance regression without indirect descriptor. 
We need to consider to address this.

Thanks

>   			break;
>   		case VIRTIO_RING_F_EVENT_IDX:
>   			break;
> @@ -2231,9 +2240,12 @@ void vring_transport_features(struct virtio_device
*vdev)
>   		case VIRTIO_F_ACCESS_PLATFORM:
>   			break;
>   		case VIRTIO_F_RING_PACKED:
> +			if (protected_guest_has(VM_MEM_ENCRYPT))
> +				goto clear;
>   			break;
>   		case VIRTIO_F_ORDER_PLATFORM:
>   			break;
> +		clear:
>   		default:
>   			/* We don't understand this bit. */
>   			__virtio_clear_bit(vdev, i);

On 6/2/21 5:41 PM, Andi Kleen wrote:
> Only allow split mode when in a protected guest. Followon
> patches harden the split mode code paths, and we don't want
> an malicious host to force anything else. Also disallow
> indirect mode for similar reasons.
I read this as "the virtio driver is buggy.  Let's disable most of the
buggy code in one special case in which we need a driver without bugs.
In all the other cases (e.g. hardware virtio device connected over
USB-C), driver bugs are still allowed."

Can we just fix the driver without special cases?

--Andy