[v1: Initial post]
With confidential computing like TDX the guest doesn't trust the host
anymore. The host is allowed to DOS of course, but it is not allowed
to read or write any guest memory not explicitely shared with it.
This has implication for virtio. Traditionally virtio didn't assume
the other side of the communication channel is malicious, and therefore
didn't do any boundary checks in virtio ring data structures.
This patchkit does hardening for virtio. In a TDX like model
the only host memory accesses allowed are in the virtio ring,
as well as the (forced) swiotlb buffer.
This patch kit does various changes to ensure there can be no
access outside these two areas. It is possible for the host
to break the communication, but this should result in a IO
error on the guest, but no memory safety violations.
virtio is quite complicated with many modes. To simplify
the task we enforce that virtio is only in split mode without
indirect descriptors, when running as a TDX guest. We also
enforce use of the DMA API.
Then these code paths are hardened against any corruptions
on the ring.
This patchkit has components in three subsystems:
- Hardening changes to virtio, all in the generic virtio-ring
- Hardening changes to kernel/dma swiotlb to harden swiotlb against
malicious pointers. It requires an API change which needed a tree sweep.
- A single x86 patch to enable the arch_has_restricted_memory_access
for TDX
It depends on Sathya's earlier patchkit that adds the basic infrastructure
for TDX. This is only needed for the "am I running in TDX" part.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
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 <redacted>
---
drivers/virtio/virtio_ring.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
In protected guest mode we don't trust the host.
This means we need to make sure the host cannot subvert us through
virtio communication. In general it can corrupt our virtio data
and cause a DOS, but it should not be able to access any data
that is not explicitely under IO.
Also boundary checking so that the free list (which is accessible
to the host) cannot point outside the virtio ring. Note it could
still contain loops or similar, but these should only cause an DOS,
not a memory corruption or leak.
When we detect any out of bounds descriptor trigger an IO error.
We also use a WARN() (in case it was a software bug instead of
an attack). This implies that a malicious host can flood
the guest kernel log, but that's only a DOS and acceptable
in the threat model.
This patch only hardens the initial consumption of the free list,
the freeing comes later.
Any of these errors can cause DMA memory leaks, but there is nothing
we can do about that and that would be just a DOS.
Signed-off-by: Andi Kleen <redacted>
---
drivers/virtio/virtio_ring.c | 46 ++++++++++++++++++++++++++++++++----
1 file changed, 42 insertions(+), 4 deletions(-)
@@ -413,6 +413,15 @@ static struct vring_desc *alloc_indirect_split(struct virtqueue *_vq,returndesc;}+/* assumes no indirect mode */+staticinlineboolinside_split_ring(structvring_virtqueue*vq,+unsignedindex)+{+return!WARN(index>=vq->split.vring.num,+"desc index %u out of bounds (%u)\n",+index,vq->split.vring.num);+}+staticinlineintvirtqueue_add_split(structvirtqueue*_vq,structscatterlist*sgs[],unsignedinttotal_sg,
@@ -428,6 +437,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,unsignedinti,n,avail,descs_used,prev,err_idx;inthead;boolindirect;+intio_err;START_USE(vq);
@@ -481,7 +491,13 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,for(n=0;n<out_sgs;n++){for(sg=sgs[n];sg;sg=sg_next(sg)){-dma_addr_taddr=vring_map_one_sg(vq,sg,DMA_TO_DEVICE);+dma_addr_taddr;++io_err=-EIO;+if(!inside_split_ring(vq,i))+gotounmap_release;+io_err=-ENOMEM;+addr=vring_map_one_sg(vq,sg,DMA_TO_DEVICE);if(vring_mapping_error(vq,addr))gotounmap_release;
@@ -494,7 +510,13 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,}for(;n<(out_sgs+in_sgs);n++){for(sg=sgs[n];sg;sg=sg_next(sg)){-dma_addr_taddr=vring_map_one_sg(vq,sg,DMA_FROM_DEVICE);+dma_addr_taddr;++io_err=-EIO;+if(!inside_split_ring(vq,i))+gotounmap_release;+io_err=-ENOMEM;+addr=vring_map_one_sg(vq,sg,DMA_FROM_DEVICE);if(vring_mapping_error(vq,addr))gotounmap_release;
@@ -513,6 +535,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,dma_addr_taddr=vring_map_single(vq,desc,total_sg*sizeof(structvring_desc),DMA_TO_DEVICE);+io_err=-ENOMEM;if(vring_mapping_error(vq,addr))gotounmap_release;
@@ -528,6 +551,10 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,/* We're using some buffers from the free list. */vq->vq.num_free-=descs_used;+io_err=-EIO;+if(!inside_split_ring(vq,head))+gotounmap_release;+/* Update free pointer */if(indirect)vq->free_head=virtio16_to_cpu(_vq->vdev,
@@ -545,6 +572,10 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,/* Put entry in available array (but don't update avail->idx until they*dosync).*/avail=vq->split.avail_idx_shadow&(vq->split.vring.num-1);++if(avail>=vq->split.vring.num)+gotounmap_release;+vq->split.vring.avail->ring[avail]=cpu_to_virtio16(_vq->vdev,head);/* Descriptors and available array need to be set before we expose the
@@ -576,6 +607,8 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,for(n=0;n<total_sg;n++){if(i==err_idx)break;+if(!inside_split_ring(vq,i))+break;vring_unmap_one_split(vq,&desc[i]);i=virtio16_to_cpu(_vq->vdev,desc[i].next);}
@@ -584,7 +617,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,kfree(desc);END_USE(vq);-return-ENOMEM;+returnio_err;}staticboolvirtqueue_kick_prepare_split(structvirtqueue*_vq)
@@ -1146,7 +1179,12 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,c=0;for(n=0;n<out_sgs+in_sgs;n++){for(sg=sgs[n];sg;sg=sg_next(sg)){-dma_addr_taddr=vring_map_one_sg(vq,sg,n<out_sgs?+dma_addr_taddr;++if(curr>=vq->packed.vring.num)+gotounmap_release;++addr=vring_map_one_sg(vq,sg,n<out_sgs?DMA_TO_DEVICE:DMA_FROM_DEVICE);if(vring_mapping_error(vq,addr))gotounmap_release;
--
2.25.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Harden the split buffer detachment path by adding boundary checking. Note
that when this fails we may fail to unmap some swiotlb mapping, which could
result in a leak and a DOS. But that's acceptable because an malicious host
can DOS us anyways.
Signed-off-by: Andi Kleen <redacted>
---
drivers/virtio/virtio_ring.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
@@ -684,7 +693,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,/* Free the indirect table, if any, now that it's unmapped. */if(!indir_desc)-return;+return0;len=virtio32_to_cpu(vq->vq.vdev,vq->split.vring.desc[head].len);
@@ -751,7 +762,12 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,/* detach_buf_split clears data, so grab it now. */ret=vq->split.desc_state[i].data;-detach_buf_split(vq,i,ctx);+err=detach_buf_split(vq,i,ctx);+if(err){+END_USE(vq);+returnNULL;+}+vq->last_used_idx++;/* If we expect an interrupt for the next entry, tell host*bywritingeventindexandflushoutthewritebefore
@@ -863,6 +879,7 @@ static void *virtqueue_detach_unused_buf_split(struct virtqueue *_vq)/* detach_buf_split clears data, so grab it now. */buf=vq->split.desc_state[i].data;detach_buf_split(vq,i,NULL);+/* Don't need to check for error because nothing is returned */vq->split.avail_idx_shadow--;vq->split.vring.avail->idx=cpu_to_virtio16(_vq->vdev,vq->split.avail_idx_shadow);
--
2.25.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
swiotlb currently only uses the start address of a DMA to check if something
is in the swiotlb or not. But with virtio and untrusted hosts the host
could give some DMA mapping that crosses the swiotlb boundaries,
potentially leaking or corrupting data. Add size checks to all the swiotlb
checks and reject any DMAs that cross the swiotlb buffer boundaries.
Signed-off-by: Andi Kleen <redacted>
---
drivers/iommu/dma-iommu.c | 13 ++++++-------
drivers/xen/swiotlb-xen.c | 11 ++++++-----
include/linux/dma-mapping.h | 4 ++--
include/linux/swiotlb.h | 8 +++++---
kernel/dma/direct.c | 8 ++++----
kernel/dma/direct.h | 8 ++++----
kernel/dma/mapping.c | 4 ++--
net/xdp/xsk_buff_pool.c | 2 +-
8 files changed, 30 insertions(+), 28 deletions(-)
In virtio the host decides whether the guest uses the DMA
API or not using the strangely named VIRTIO_F_ACCESS_PLATFORM
bit (which really indicates whether the DMA API is used or not)
For hardened virtio on TDX we want to enforce that that swiotlb is
always used, which requires using the DMA API. While IO wouldn't
really work without the swiotlb, it might be possible that an
attacker forces swiotlbless IO to manipulate memory in the guest.
So we want to force the DMA API (which then forces swiotlb),
but without relying on the host.
There is already an arch_has_restricted_memory_acces hook for
this, which is currently used only by s390. Enable
the config option for the hook for x86 and enable it for TDX.
Signed-off-by: Andi Kleen <redacted>
---
arch/x86/Kconfig | 1 +
arch/x86/mm/mem_encrypt_common.c | 7 +++++++
2 files changed, 8 insertions(+)
In some situations when we know swiotlb is forced and we have
to deal with untrusted hosts, it's useful to know if a mapping
was in the swiotlb or not. This allows us to abort any IO
operation that would access memory outside the swiotlb.
Otherwise it might be possible for a malicious host to inject
any guest page in a read operation. While it couldn't directly
access the results of the read() inside the guest, there
might scenarios where data is echoed back with a write(),
and that would then leak guest memory.
Add a return value to dma_unmap_single/page. Most users
of course will ignore it. The return value is set to EIO
if we're in forced swiotlb mode and the buffer is not inside
the swiotlb buffer. Otherwise it's always 0.
A new callback is used to avoid changing all the IOMMU drivers.
Signed-off-by: Andi Kleen <redacted>
---
drivers/iommu/dma-iommu.c | 17 +++++++++++------
include/linux/dma-map-ops.h | 3 +++
include/linux/dma-mapping.h | 7 ++++---
kernel/dma/mapping.c | 6 +++++-
4 files changed, 23 insertions(+), 10 deletions(-)
Error out with a warning when the free list loops longer
than the maximum size while freeing descriptors. While technically
we don't care about DOS it is still better to abort it early.
We ran into this problem while fuzzing the virtio interactions
where the fuzzed code would get stuck for a long time.
Signed-off-by: Andi Kleen <redacted>
---
drivers/virtio/virtio_ring.c | 5 +++++
1 file changed, 5 insertions(+)
Now that we have a return value for unmapping DMA mappings that
are outside the forced swiotlb, use that to abort the IO operation.
This prevents the host from subverting a read to access some
data in the guest address space, which it might then get access somehow in
another IO operation. It can subvert reads to point to other
reads or other writes, but since it controls IO it can do
that anyways.
This is only done for the split code path, which is the only
one supported with confidential guests.
Signed-off-by: Andi Kleen <redacted>
---
drivers/virtio/virtio_ring.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
@@ -365,29 +365,31 @@ static int vring_mapping_error(const struct vring_virtqueue *vq,*Splitringspecificfunctions-*_split().*/-staticvoidvring_unmap_one_split(conststructvring_virtqueue*vq,+staticintvring_unmap_one_split(conststructvring_virtqueue*vq,structvring_desc*desc){u16flags;+intret;if(!vq->use_dma_api)-return;+return0;flags=virtio16_to_cpu(vq->vq.vdev,desc->flags);if(flags&VRING_DESC_F_INDIRECT){-dma_unmap_single(vring_dma_dev(vq),+ret=dma_unmap_single(vring_dma_dev(vq),virtio64_to_cpu(vq->vq.vdev,desc->addr),virtio32_to_cpu(vq->vq.vdev,desc->len),(flags&VRING_DESC_F_WRITE)?DMA_FROM_DEVICE:DMA_TO_DEVICE);}else{-dma_unmap_page(vring_dma_dev(vq),+ret=dma_unmap_page(vring_dma_dev(vq),virtio64_to_cpu(vq->vq.vdev,desc->addr),virtio32_to_cpu(vq->vq.vdev,desc->len),(flags&VRING_DESC_F_WRITE)?DMA_FROM_DEVICE:DMA_TO_DEVICE);}+returnret;}staticstructvring_desc*alloc_indirect_split(structvirtqueue*_vq,
@@ -609,6 +611,10 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,break;if(!inside_split_ring(vq,i))break;+/*+*Ignoreunmappingerrorssince+*we'reabortinganyways.+*/vring_unmap_one_split(vq,&desc[i]);i=virtio16_to_cpu(_vq->vdev,desc[i].next);}
@@ -671,7 +677,10 @@ static int detach_buf_split(struct vring_virtqueue *vq, unsigned int head,i=head;while(vq->split.vring.desc[i].flags&nextflag){-vring_unmap_one_split(vq,&vq->split.vring.desc[i]);+intret;+ret=vring_unmap_one_split(vq,&vq->split.vring.desc[i]);+if(ret)+returnret;i=virtio16_to_cpu(vq->vq.vdev,vq->split.vring.desc[i].next);if(!inside_split_ring(vq,i))return-EIO;
@@ -878,6 +887,7 @@ static void *virtqueue_detach_unused_buf_split(struct virtqueue *_vq)continue;/* detach_buf_split clears data, so grab it now. */buf=vq->split.desc_state[i].data;+/* Ignore unmap errors because there is nothing to abort */detach_buf_split(vq,i,NULL);/* Don't need to check for error because nothing is returned */vq->split.avail_idx_shadow--;
--
2.25.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
From: Jason Wang <hidden> Date: 2021-06-03 01:34:23
在 2021/6/3 上午8:41, Andi Kleen 写道:
[v1: Initial post]
With confidential computing like TDX the guest doesn't trust the host
anymore. The host is allowed to DOS of course, but it is not allowed
to read or write any guest memory not explicitely shared with it.
This has implication for virtio. Traditionally virtio didn't assume
the other side of the communication channel is malicious, and therefore
didn't do any boundary checks in virtio ring data structures.
This patchkit does hardening for virtio. In a TDX like model
the only host memory accesses allowed are in the virtio ring,
as well as the (forced) swiotlb buffer.
This patch kit does various changes to ensure there can be no
access outside these two areas. It is possible for the host
to break the communication, but this should result in a IO
error on the guest, but no memory safety violations.
virtio is quite complicated with many modes. To simplify
the task we enforce that virtio is only in split mode without
indirect descriptors, when running as a TDX guest. We also
enforce use of the DMA API.
Then these code paths are hardened against any corruptions
on the ring.
This patchkit has components in three subsystems:
- Hardening changes to virtio, all in the generic virtio-ring
- Hardening changes to kernel/dma swiotlb to harden swiotlb against
malicious pointers. It requires an API change which needed a tree sweep.
- A single x86 patch to enable the arch_has_restricted_memory_access
for TDX
It depends on Sathya's earlier patchkit that adds the basic infrastructure
for TDX. This is only needed for the "am I running in TDX" part.
Note that it's probably needed by other cases as well:
1) Other encrypted VM technology
2) VDUSE[1]
3) Smart NICs
We have already had discussions and some patches have been posted[2][3][4].
I think the basic idea is similar, basically, we don't trust any
metadata provided by the device.
[2] is the series that use the metadata stored in the private memory
which can't be accessed by swiotlb, this series aims to eliminate all
the possible attacks via virtqueue metadata
[3] is one example for the the used length validation
[4] is the fix for the malicious config space
Thanks
[1] https://www.spinics.net/lists/netdev/msg743264.html
[2] https://www.spinics.net/lists/kvm/msg241825.html
[3] https://patches.linaro.org/patch/450733/
[4] https://lkml.org/lkml/2021/5/17/376
From: Jason Wang <hidden> Date: 2021-06-03 01:36:15
在 2021/6/3 上午8:41, Andi Kleen 写道:
quoted hunk
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 <redacted>
---
drivers/virtio/virtio_ring.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
So we will see huge performance regression without indirect
descriptor. We need to consider to address this.
A regression would be when some existing case would be slower.
That's not the case because the behavior for the existing cases does not
change.
Anyways when there are performance problems they can be addressed, but
first is to make it secure.
-Andi
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);
From: Konrad Rzeszutek Wilk <hidden> Date: 2021-06-03 01:50:00
On Wed, Jun 02, 2021 at 05:41:30PM -0700, Andi Kleen wrote:
swiotlb currently only uses the start address of a DMA to check if something
is in the swiotlb or not. But with virtio and untrusted hosts the host
could give some DMA mapping that crosses the swiotlb boundaries,
potentially leaking or corrupting data. Add size checks to all the swiotlb
checks and reject any DMAs that cross the swiotlb buffer boundaries.
I seem to be only CC-ed on this and #7, so please bear with me.
But could you explain to me why please:
commit daf9514fd5eb098d7d6f3a1247cb8cc48fc94155 (swiotlb/stable/for-linus-5.12)
Author: Martin Radev [off-list ref]
Date: Tue Jan 12 16:07:29 2021 +0100
swiotlb: Validate bounce size in the sync/unmap path
does not solve the problem as well?
Note that it's probably needed by other cases as well:
1) Other encrypted VM technology
2) VDUSE[1]
3) Smart NICs
Right. I don't see any reason why these shouldn't work. You may just
need to add the enable for the lockdown, but you can reuse the basic
infrastructure.
We have already had discussions and some patches have been
posted[2][3][4].
Thanks.
Yes [2] is indeed an alternative. We considered this at some point, but
since we don't care about DOS in our case it seemed simpler to just
harden the existing code. But yes if it's there it's useful for TDX too.
FWIW I would argue that the descriptor boundary checking should be added
in any case, security case or separated metadata or not, because it can
catch bugs and is very cheap. Checking boundaries is good practice.
[4] would be an independent issue, that's something we didn't catch.
Also the swiotlb hardening implemented in this patchkit doesn't seem to
be in any of the other patches.
So I would say my patches are mostly orthogonal to these patches below
and not conflicting, even though they address a similar problem space.
-Andi
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On Wed, Jun 02, 2021 at 05:41:30PM -0700, Andi Kleen wrote:
quoted
swiotlb currently only uses the start address of a DMA to check if something
is in the swiotlb or not. But with virtio and untrusted hosts the host
could give some DMA mapping that crosses the swiotlb boundaries,
potentially leaking or corrupting data. Add size checks to all the swiotlb
checks and reject any DMAs that cross the swiotlb buffer boundaries.
I seem to be only CC-ed on this and #7, so please bear with me.
You weren't cc'ed originally so if you get partial emails it must be
through some list.
But could you explain to me why please:
commit daf9514fd5eb098d7d6f3a1247cb8cc48fc94155 (swiotlb/stable/for-linus-5.12)
Author: Martin Radev [off-list ref]
Date: Tue Jan 12 16:07:29 2021 +0100
swiotlb: Validate bounce size in the sync/unmap path
does not solve the problem as well?
Thanks. I missed that patch, race condition.
One major difference of my patch is that it supports an error return,
which allows virtio to error out. This is important in virtio because
otherwise you'll end up with uninitialized memory on the target without
any indication. This uninitialized memory could be an potential attack
vector on the guest memory, e.g. if the attacker finds some way to echo
it out again.
But the error return could be added to your infrastructure too and what
would make this patch much shorter. I'll take a look at that.
-Andi
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
From: Jason Wang <hidden> Date: 2021-06-03 02:14:50
在 2021/6/3 上午8:41, Andi Kleen 写道:
quoted hunk
In protected guest mode we don't trust the host.
This means we need to make sure the host cannot subvert us through
virtio communication. In general it can corrupt our virtio data
and cause a DOS, but it should not be able to access any data
that is not explicitely under IO.
Also boundary checking so that the free list (which is accessible
to the host) cannot point outside the virtio ring. Note it could
still contain loops or similar, but these should only cause an DOS,
not a memory corruption or leak.
When we detect any out of bounds descriptor trigger an IO error.
We also use a WARN() (in case it was a software bug instead of
an attack). This implies that a malicious host can flood
the guest kernel log, but that's only a DOS and acceptable
in the threat model.
This patch only hardens the initial consumption of the free list,
the freeing comes later.
Any of these errors can cause DMA memory leaks, but there is nothing
we can do about that and that would be just a DOS.
Signed-off-by: Andi Kleen <redacted>
---
drivers/virtio/virtio_ring.c | 46 ++++++++++++++++++++++++++++++++----
1 file changed, 42 insertions(+), 4 deletions(-)
@@ -413,6 +413,15 @@ static struct vring_desc *alloc_indirect_split(struct virtqueue *_vq,returndesc;}+/* assumes no indirect mode */+staticinlineboolinside_split_ring(structvring_virtqueue*vq,+unsignedindex)+{+return!WARN(index>=vq->split.vring.num,+"desc index %u out of bounds (%u)\n",+index,vq->split.vring.num);
It's better to use BAD_RING to stop virtqueue in this case.
quoted hunk
+}
+
static inline int virtqueue_add_split(struct virtqueue *_vq,
struct scatterlist *sgs[],
unsigned int total_sg,
@@ -428,6 +437,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, unsigned int i, n, avail, descs_used, prev, err_idx; int head; bool indirect;+ int io_err; START_USE(vq);
@@ -481,7 +491,13 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, for (n = 0; n < out_sgs; n++) { for (sg = sgs[n]; sg; sg = sg_next(sg)) {- dma_addr_t addr = vring_map_one_sg(vq, sg, DMA_TO_DEVICE);+ dma_addr_t addr;++ io_err = -EIO;+ if (!inside_split_ring(vq, i))+ goto unmap_release;+ io_err = -ENOMEM;+ addr = vring_map_one_sg(vq, sg, DMA_TO_DEVICE); if (vring_mapping_error(vq, addr)) goto unmap_release;
@@ -494,7 +510,13 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, } for (; n < (out_sgs + in_sgs); n++) { for (sg = sgs[n]; sg; sg = sg_next(sg)) {- dma_addr_t addr = vring_map_one_sg(vq, sg, DMA_FROM_DEVICE);+ dma_addr_t addr;++ io_err = -EIO;+ if (!inside_split_ring(vq, i))+ goto unmap_release;+ io_err = -ENOMEM;+ addr = vring_map_one_sg(vq, sg, DMA_FROM_DEVICE); if (vring_mapping_error(vq, addr)) goto unmap_release;
It looks to me all the evils came from the fact that we depends on the
descriptor ring.
So the checks in this patch could is unnecessary if we don't even read
from the descriptor ring which could be manipulated by the device.
This is what my series tries to achieve:
https://www.spinics.net/lists/kvm/msg241825.html
Thanks
@@ -528,6 +551,10 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, /* We're using some buffers from the free list. */ vq->vq.num_free -= descs_used;+ io_err = -EIO;+ if (!inside_split_ring(vq, head))+ goto unmap_release;+ /* Update free pointer */ if (indirect) vq->free_head = virtio16_to_cpu(_vq->vdev,
@@ -545,6 +572,10 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, /* Put entry in available array (but don't update avail->idx until they * do sync). */ avail = vq->split.avail_idx_shadow & (vq->split.vring.num - 1);++ if (avail >= vq->split.vring.num)+ goto unmap_release;+ vq->split.vring.avail->ring[avail] = cpu_to_virtio16(_vq->vdev, head); /* Descriptors and available array need to be set before we expose the
@@ -576,6 +607,8 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, for (n = 0; n < total_sg; n++) { if (i == err_idx) break;+ if (!inside_split_ring(vq, i))+ break; vring_unmap_one_split(vq, &desc[i]); i = virtio16_to_cpu(_vq->vdev, desc[i].next); }
It looks to me all the evils came from the fact that we depends on the
descriptor ring.
So the checks in this patch could is unnecessary if we don't even read
from the descriptor ring which could be manipulated by the device.
This is what my series tries to achieve:
https://www.spinics.net/lists/kvm/msg241825.html
I would argue that you should boundary check in any case. It was always
a bug to not have boundary checks in such a data structure with multiple
users, trust or not.
But yes your patch series is interesting and definitely makes sense for
TDX too.
Best would be to have both I guess, and always check the boundaries
everywhere.
So what's the merge status of your series?
-Andi
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
From: Jason Wang <hidden> Date: 2021-06-03 02:29:22
在 2021/6/3 上午8:41, Andi Kleen 写道:
quoted hunk
Harden the split buffer detachment path by adding boundary checking. Note
that when this fails we may fail to unmap some swiotlb mapping, which could
result in a leak and a DOS. But that's acceptable because an malicious host
can DOS us anyways.
Signed-off-by: Andi Kleen <redacted>
---
drivers/virtio/virtio_ring.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
@@ -651,12 +651,19 @@ static bool virtqueue_kick_prepare_split(struct virtqueue *_vq)returnneeds_kick;}-staticvoiddetach_buf_split(structvring_virtqueue*vq,unsignedinthead,-void**ctx)+staticintdetach_buf_split(structvring_virtqueue*vq,unsignedinthead,+void**ctx){unsignedinti,j;__virtio16nextflag=cpu_to_virtio16(vq->vq.vdev,VRING_DESC_F_NEXT);+/* We'll leak DMA mappings when this happens, but nothing+*canbedoneaboutthat.Intheworstcasethehost+*couldDOSus,butitcanofcoursedothatanyways.+*/+if(!inside_split_ring(vq,head))+return-EIO;
I think the caller have already did this for us with even more check on
the token (virtqueue_get_buf_ctx_split()):
if (unlikely(i >= vq->split.vring.num)) {
BAD_RING(vq, "id %u out of range\n", i);
return NULL;
}
if (unlikely(!vq->split.desc_state[i].data)) {
BAD_RING(vq, "id %u is not a head!\n", i);
return NULL;
}
quoted hunk
+
/* Clear data ptr. */
vq->split.desc_state[head].data = NULL;
@@ -666,6 +673,8 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head, while (vq->split.vring.desc[i].flags & nextflag) { vring_unmap_one_split(vq, &vq->split.vring.desc[i]); i = virtio16_to_cpu(vq->vq.vdev, vq->split.vring.desc[i].next);+ if (!inside_split_ring(vq, i))+ return -EIO;
Similarly, if we don't depend on the metadata stored in the descriptor,
we don't need this check.
quoted hunk
vq->vq.num_free++;
}
@@ -684,7 +693,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head, /* Free the indirect table, if any, now that it's unmapped. */ if (!indir_desc)- return;+ return 0; len = virtio32_to_cpu(vq->vq.vdev, vq->split.vring.desc[head].len);
@@ -717,6 +727,7 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, void *ret; unsigned int i; u16 last_used;+ int err; START_USE(vq);
@@ -751,7 +762,12 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, /* detach_buf_split clears data, so grab it now. */ ret = vq->split.desc_state[i].data;- detach_buf_split(vq, i, ctx);+ err = detach_buf_split(vq, i, ctx);+ if (err) {+ END_USE(vq);
This reminds me that we don't use END_USE() after BAD_RING() which
should be fixed.
Thanks
quoted hunk
+ return NULL;
+ }
+
vq->last_used_idx++;
/* If we expect an interrupt for the next entry, tell host
* by writing event index and flush out the write before
@@ -863,6 +879,7 @@ static void *virtqueue_detach_unused_buf_split(struct virtqueue *_vq) /* detach_buf_split clears data, so grab it now. */ buf = vq->split.desc_state[i].data; detach_buf_split(vq, i, NULL);+ /* Don't need to check for error because nothing is returned */ vq->split.avail_idx_shadow--; vq->split.vring.avail->idx = cpu_to_virtio16(_vq->vdev, vq->split.avail_idx_shadow);
From: Jason Wang <hidden> Date: 2021-06-03 02:32:50
在 2021/6/3 上午9:48, Andi Kleen 写道:
quoted
So we will see huge performance regression without indirect
descriptor. We need to consider to address this.
A regression would be when some existing case would be slower.
That's not the case because the behavior for the existing cases does
not change.
Anyways when there are performance problems they can be addressed, but
first is to make it secure.
I agree, but I want to know why indirect descriptor needs to be
disabled. The table can't be wrote by the device since it's not coherent
swiotlb mapping.
Thanks
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);
From: Jason Wang <hidden> Date: 2021-06-03 02:37:20
在 2021/6/3 上午10:18, Andi Kleen 写道:
quoted
It looks to me all the evils came from the fact that we depends on
the descriptor ring.
So the checks in this patch could is unnecessary if we don't even
read from the descriptor ring which could be manipulated by the device.
This is what my series tries to achieve:
https://www.spinics.net/lists/kvm/msg241825.html
I would argue that you should boundary check in any case. It was
always a bug to not have boundary checks in such a data structure with
multiple users, trust or not.
But yes your patch series is interesting and definitely makes sense
for TDX too.
Best would be to have both I guess, and always check the boundaries
everywhere.
I agree but some of the checks are unnecessary in we do this series on
top of my series.
So what's the merge status of your series?
If I understand correctly from Michael, I will send a formal series and
he will try to merge it for the 5.14.
Thanks
I agree, but I want to know why indirect descriptor needs to be
disabled. The table can't be wrote by the device since it's not
coherent swiotlb mapping.
I had all kinds of problems with uninitialized entries in the indirect
table. So I gave up on it and concluded it would be too difficult to secure.
-Andi
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
From: Jason Wang <hidden> Date: 2021-06-03 03:05:07
在 2021/6/3 上午10:56, Andi Kleen 写道:
quoted
I agree, but I want to know why indirect descriptor needs to be
disabled. The table can't be wrote by the device since it's not
coherent swiotlb mapping.
I had all kinds of problems with uninitialized entries in the indirect
table. So I gave up on it and concluded it would be too difficult to
secure.
-Andi
Ok, but what I meant is this, if we don't read from the descriptor ring,
and validate all the other metadata supplied by the device (used id and
len). Then there should be no way for the device to suppress the dma
flags to write to the indirect descriptor table.
Or do you have an example how it can do that?
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
From: Robin Murphy <robin.murphy@arm.com> Date: 2021-06-03 09:08:39
Hi Andi,
On 2021-06-03 01:41, Andi Kleen wrote:
In some situations when we know swiotlb is forced and we have
to deal with untrusted hosts, it's useful to know if a mapping
was in the swiotlb or not. This allows us to abort any IO
operation that would access memory outside the swiotlb.
Otherwise it might be possible for a malicious host to inject
any guest page in a read operation. While it couldn't directly
access the results of the read() inside the guest, there
might scenarios where data is echoed back with a write(),
and that would then leak guest memory.
Add a return value to dma_unmap_single/page. Most users
of course will ignore it. The return value is set to EIO
if we're in forced swiotlb mode and the buffer is not inside
the swiotlb buffer. Otherwise it's always 0.
I have to say my first impression of this isn't too good :(
What it looks like to me is abusing SWIOTLB's internal housekeeping to
keep track of virtio-specific state. The DMA API does not attempt to
validate calls in general since in many cases the additional overhead
would be prohibitive. It has always been callers' responsibility to keep
track of what they mapped and make sure sync/unmap calls match, and
there are many, many, subtle and not-so-subtle ways for things to go
wrong if they don't. If virtio is not doing a good enough job of that,
what's the justification for making it the DMA API's problem?
A new callback is used to avoid changing all the IOMMU drivers.
Nit: presumably by "IOMMU drivers" you actually mean arch DMA API backends?
As an aside, we'll take a look at the rest of the series for the
perspective of our prototyping for Arm's Confidential Compute
Architecture, but I'm not sure we'll need it, since accesses beyond the
bounds of the shared SWIOTLB buffer shouldn't be an issue for us.
Furthermore, AFAICS it's still not going to help against exfiltrating
guest memory by over-unmapping the original SWIOTLB slot *without* going
past the end of the whole buffer, but I think Martin's patch *has*
addressed that already.
Robin.
From: Robin Murphy <robin.murphy@arm.com> Date: 2021-06-03 09:09:39
On 2021-06-03 01:41, Andi Kleen wrote:
quoted hunk
swiotlb currently only uses the start address of a DMA to check if something
is in the swiotlb or not. But with virtio and untrusted hosts the host
could give some DMA mapping that crosses the swiotlb boundaries,
potentially leaking or corrupting data. Add size checks to all the swiotlb
checks and reject any DMAs that cross the swiotlb buffer boundaries.
Signed-off-by: Andi Kleen <redacted>
---
drivers/iommu/dma-iommu.c | 13 ++++++-------
drivers/xen/swiotlb-xen.c | 11 ++++++-----
include/linux/dma-mapping.h | 4 ++--
include/linux/swiotlb.h | 8 +++++---
kernel/dma/direct.c | 8 ++++----
kernel/dma/direct.h | 8 ++++----
kernel/dma/mapping.c | 4 ++--
net/xdp/xsk_buff_pool.c | 2 +-
8 files changed, 30 insertions(+), 28 deletions(-)
What it looks like to me is abusing SWIOTLB's internal housekeeping to
keep track of virtio-specific state. The DMA API does not attempt to
validate calls in general since in many cases the additional overhead
would be prohibitive. It has always been callers' responsibility to
keep track of what they mapped and make sure sync/unmap calls match,
and there are many, many, subtle and not-so-subtle ways for things to
go wrong if they don't. If virtio is not doing a good enough job of
that, what's the justification for making it the DMA API's problem?
In this case it's not prohibitive at all. Just adding a few error
returns, and checking the overlap (which seems to have been already
solved anyways) I would argue the error returns are good practice
anyways, so that API users can check that something bad happening and
abort. The DMA API was never very good at proper error handling, but
there's no reason at all to continue being bad it forever.
AFAIK the rest just works anyways, so it's not really a new problem to
be solved.
quoted
A new callback is used to avoid changing all the IOMMU drivers.
Nit: presumably by "IOMMU drivers" you actually mean arch DMA API
backends?
Yes
Furthermore, AFAICS it's still not going to help against exfiltrating
guest memory by over-unmapping the original SWIOTLB slot *without*
going past the end of the whole buffer,
That would be just exfiltrating data that is already shared, unless I'm
misunderstanding you.
-Andi
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Ok, but what I meant is this, if we don't read from the descriptor
ring, and validate all the other metadata supplied by the device (used
id and len). Then there should be no way for the device to suppress
the dma flags to write to the indirect descriptor table.
Or do you have an example how it can do that?
I don't. If you can validate everything it's probably ok
The only drawback is even more code to audit and test.
-Andi
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
From: Andy Lutomirski <luto@kernel.org> Date: 2021-06-03 17:33:44
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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
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."
My understanding is most of the other modes (except for split with separate descriptors) are obsolete and just there for compatibility. As long as they're deprecated they won't harm anyone.
-Andi
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On Thu, Jun 3, 2021, at 11:00 AM, Andi Kleen wrote:
On 6/3/2021 10:33 AM, Andy Lutomirski wrote:
quoted
On 6/2/21 5:41 PM, Andi Kleen wrote:
quoted
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."
My understanding is most of the other modes (except for split with
separate descriptors) are obsolete and just there for compatibility. As
long as they're deprecated they won't harm anyone.
Tell that to every crypto downgrade attack ever.
I see two credible solutions:
1. Actually harden the virtio driver.
2. Have a new virtio-modern driver and use it for modern use cases. Maybe rename the old driver virtio-legacy or virtio-insecure. They can share code.
Another snag you may hit: virtio’s heuristic for whether to use proper DMA ops or to bypass them is a giant kludge. I’m very slightly optimistic that getting the heuristic wrong will make the driver fail to operate but won’t allow the host to take over the guest, but I’m not really convinced. And I wrote that code! A virtio-modern mode probably should not have a heuristic, and the various iommu-bypassing modes should be fixed to work at the bus level, not the device level.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
I see two credible solutions:
1. Actually harden the virtio driver.
That's exactly what this patchkit, and the alternative approaches, like
Jason's, are doing.
2. Have a new virtio-modern driver and use it for modern use cases. Maybe rename the old driver virtio-legacy or virtio-insecure. They can share code.
In most use cases the legacy driver is not insecure because there is no
memory protection anyways.
Yes maybe such a split would be a good idea for maintenance and maybe
performance reasons, but at least from the security perspective I don't
see any need for it.
Another snag you may hit: virtio’s heuristic for whether to use proper DMA ops or to bypass them is a giant kludge. I’m very slightly optimistic that getting the heuristic wrong will make the driver fail to operate but won’t allow the host to take over the guest, but I’m not really convinced. And I wrote that code! A virtio-modern mode probably should not have a heuristic, and the various iommu-bypassing modes should be fixed to work at the bus level, not the device level
TDX and SEV use the arch hook to enforce DMA API, so that part is also
solved.
-Andi
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On Thu, Jun 3, 2021, at 12:53 PM, Andi Kleen wrote:
quoted
Tell that to every crypto downgrade attack ever.
That's exactly what this patch addresses.
quoted
I see two credible solutions:
1. Actually harden the virtio driver.
That's exactly what this patchkit, and the alternative approaches, like
Jason's, are doing.
quoted
2. Have a new virtio-modern driver and use it for modern use cases. Maybe rename the old driver virtio-legacy or virtio-insecure. They can share code.
In most use cases the legacy driver is not insecure because there is no
memory protection anyways.
Yes maybe such a split would be a good idea for maintenance and maybe
performance reasons, but at least from the security perspective I don't
see any need for it.
Please reread my email.
We do not need an increasing pile of kludges to make TDX and SEV “secure”. We need the actual loaded driver to be secure. The virtio architecture is full of legacy nonsense, and there is no good reason for SEV and TDX to be a giant special case.
As I said before, real PCIe (Thunderbolt/USB-C or anything else) has the exact same problem. The fact that TDX has encrypted memory is, at best, a poor proxy for the actual condition. The actual condition is that the host does not trust the device to implement the virtio protocol correctly.
quoted
Another snag you may hit: virtio’s heuristic for whether to use proper DMA ops or to bypass them is a giant kludge. I’m very slightly optimistic that getting the heuristic wrong will make the driver fail to operate but won’t allow the host to take over the guest, but I’m not really convinced. And I wrote that code! A virtio-modern mode probably should not have a heuristic, and the various iommu-bypassing modes should be fixed to work at the bus level, not the device level
TDX and SEV use the arch hook to enforce DMA API, so that part is also
solved.
Do you mean disabling features is a kludge?
If yes I disagree with that characterization.
to make TDX and SEV “secure”. We need the actual loaded driver to be secure. The virtio architecture is full of legacy nonsense,
and there is no good reason for SEV and TDX to be a giant special case.
I don't know where you see a "giant special case". Except for the
limited feature negotiation all the changes are common, and the
disabling of features (which is not new BTW, but already done e.g. with
forcing DMA API in some cases) can be of course used by all these other
technologies too. But it just cannot be done by default for everything
because it would break compatibility. So every technology with such
requirements has to explicitly opt-in.
As I said before, real PCIe (Thunderbolt/USB-C or anything else) has the exact same problem. The fact that TDX has encrypted memory is, at best, a poor proxy for the actual condition. The actual condition is that the host does not trust the device to implement the virtio protocol correctly.
Right they can do similar limitations of feature sets. But again it
cannot be default.
quoted
TDX and SEV use the arch hook to enforce DMA API, so that part is also
solved.
Can you point me to the code you’re referring to?
See 4/8 in this patch kit. It uses an existing hook which is already
used in tree by s390.
-Andi
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
From: Jason Wang <hidden> Date: 2021-06-04 01:22:56
在 2021/6/4 上午3:31, Andy Lutomirski 写道:
On Thu, Jun 3, 2021, at 11:00 AM, Andi Kleen wrote:
quoted
On 6/3/2021 10:33 AM, Andy Lutomirski wrote:
quoted
On 6/2/21 5:41 PM, Andi Kleen wrote:
quoted
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."
My understanding is most of the other modes (except for split with
separate descriptors) are obsolete and just there for compatibility. As
long as they're deprecated they won't harm anyone.
Tell that to every crypto downgrade attack ever.
I see two credible solutions:
1. Actually harden the virtio driver.
2. Have a new virtio-modern driver and use it for modern use cases. Maybe rename the old driver virtio-legacy or virtio-insecure. They can share code.
Note that we had already split legacy driver out which can be turned off
via Kconfig.
Another snag you may hit: virtio’s heuristic for whether to use proper DMA ops or to bypass them is a giant kludge. I’m very slightly optimistic that getting the heuristic wrong will make the driver fail to operate but won’t allow the host to take over the guest, but I’m not really convinced. And I wrote that code! A virtio-modern mode probably should not have a heuristic, and the various iommu-bypassing modes should be fixed to work at the bus level, not the device level.
I remember there's a very long discussion about this and probably
without any conclusion. Fortunately, the management layer has been
taught to enforce VIRTIO_F_ACCESS_PLATFORM for encrypted guests.
A possible way to fix this is without any conflicts is to mandate the
VIRTIO_F_ACCESS_PLATFORM in version 1.2.
Thanks
From: Jason Wang <hidden> Date: 2021-06-04 01:29:21
在 2021/6/4 上午2:00, Andi Kleen 写道:
On 6/3/2021 10:33 AM, Andy Lutomirski wrote:
quoted
On 6/2/21 5:41 PM, Andi Kleen wrote:
quoted
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."
My understanding is most of the other modes (except for split with
separate descriptors) are obsolete and just there for compatibility.
As long as they're deprecated they won't harm anyone.
-Andi
For "mode" do you packed vs split? If yes, it's not just for
compatibility. Though packed virtqueue is designed to be more hardware
friendly, most hardware vendors choose to start from split.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
From: Andy Lutomirski <luto@kernel.org> Date: 2021-06-04 01:46:10
On 6/3/21 4:32 PM, Andi Kleen wrote:
quoted
We do not need an increasing pile of kludges
Do you mean disabling features is a kludge?
If yes I disagree with that characterization.
quoted
to make TDX and SEV “secure”. We need the actual loaded driver to be
secure. The virtio architecture is full of legacy nonsense,
and there is no good reason for SEV and TDX to be a giant special case.
I don't know where you see a "giant special case". Except for the
limited feature negotiation all the changes are common, and the
disabling of features (which is not new BTW, but already done e.g. with
forcing DMA API in some cases) can be of course used by all these other
technologies too. But it just cannot be done by default for everything
because it would break compatibility. So every technology with such
requirements has to explicitly opt-in.
quoted
As I said before, real PCIe (Thunderbolt/USB-C or anything else) has
the exact same problem. The fact that TDX has encrypted memory is, at
best, a poor proxy for the actual condition. The actual condition is
that the host does not trust the device to implement the virtio
protocol correctly.
Right they can do similar limitations of feature sets. But again it
cannot be default.
Let me try again.
For most Linux drivers, a report that a misbehaving device can corrupt
host memory is a bug, not a feature. If a USB device can corrupt kernel
memory, that's a serious bug. If a USB-C device can corrupt kernel
memory, that's also a serious bug, although, sadly, we probably have
lots of these bugs. If a Firewire device can corrupt kernel memory,
news at 11. If a Bluetooth or WiFi peer can corrupt kernel memory,
people write sonnets about it and give it clever names. Why is virtio
special?
If, for some reason, the virtio driver cannot be fixed so that it is
secure and compatible [1], then I think that the limited cases that are
secure should be accessible to anyone, with or without TDX. Have a
virtio.secure_mode module option or a udev-controllable parameter or an
alternative driver name or *something*. An alternative driver name
would allow userspace to prevent the insecure mode from auto-binding to
devices. And make whatever system configures encrypted guests for
security use this mode. (Linux is not going to be magically secure just
by booting it in TDX. There's a whole process of unsealing or remote
attestation, something needs to prevent the hypervisor from connecting a
virtual keyboard and typing init=/bin/bash, something needs to provision
an SSH key, etc.)
In my opinion, it is not so great to identify bugs in the driver and
then say that they're only being fixed for TDX and SEV.
Keep in mind that, as I understand it, there is nothing virt specific
about virtio. There are real physical devices that speak virtio.
[1] The DMA quirk is nasty. Fortunately, it's the only case I'm aware
of in which the virtio driver genuinely cannot be made secure and
compatible at the smae time. Also, fortunately, most real deployments
except on powerpc work just fine with the DMA quirk unquirked.
quoted
quoted
TDX and SEV use the arch hook to enforce DMA API, so that part is also
solved.
Can you point me to the code you’re referring to?
See 4/8 in this patch kit. It uses an existing hook which is already
used in tree by s390.
This one:
int arch_has_restricted_virtio_memory_access(void)
+{
+ return is_tdx_guest();
+}
I'm looking at a fairly recent kernel, and I don't see anything for s390
wired up in vring_use_dma_api.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
For most Linux drivers, a report that a misbehaving device can corrupt
host memory is a bug, not a feature. If a USB device can corrupt kernel
memory, that's a serious bug. If a USB-C device can corrupt kernel
memory, that's also a serious bug, although, sadly, we probably have
lots of these bugs. If a Firewire device can corrupt kernel memory,
news at 11. If a Bluetooth or WiFi peer can corrupt kernel memory,
people write sonnets about it and give it clever names. Why is virtio
special?
Well for most cases it's pointless because they don't have any memory
protection anyways.
Why break compatibility if it does not buy you anything?
Anyways if you want to enable the restricted mode for something else,
it's easy to do. The cases where it matters seem to already work on it,
like the user space virtio ring.
My changes for boundary checking are enabled unconditionally anyways, as
well as the other patchkits.
This one:
int arch_has_restricted_virtio_memory_access(void)
+{
+ return is_tdx_guest();
+}
I'm looking at a fairly recent kernel, and I don't see anything for s390
wired up in vring_use_dma_api.
It's not using vring_use_dma_api, but enforces the DMA API at virtio
ring setup time, same as SEV/TDX.
-Andi
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
From: Jason Wang <hidden> Date: 2021-06-04 02:21:01
在 2021/6/4 上午1:33, Andy Lutomirski 写道:
On 6/2/21 5:41 PM, Andi Kleen wrote:
quoted
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?
From: Jason Wang <hidden> Date: 2021-06-04 02:29:59
在 2021/6/3 下午9:55, Andi Kleen 写道:
quoted
Ok, but what I meant is this, if we don't read from the descriptor
ring, and validate all the other metadata supplied by the device
(used id and len). Then there should be no way for the device to
suppress the dma flags to write to the indirect descriptor table.
Or do you have an example how it can do that?
I don't. If you can validate everything it's probably ok
The only drawback is even more code to audit and test.
-Andi
Ok, then I'm going to post a formal series, please have a look and we
can start from there.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization