| Message ID | 20210604055350.58753-1-jasowang@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Do not read from descriptor ring | expand |
On 6/3/21 10:53 PM, Jason Wang wrote: > Hi: > > The virtio driver should not trust the device. This beame more urgent > for the case of encrtpyed VM or VDUSE[1]. In both cases, technology > like swiotlb/IOMMU is used to prevent the poking/mangling of memory > from the device. But this is not sufficient since current virtio > driver may trust what is stored in the descriptor table (coherent > mapping) for performing the DMA operations like unmap and bounce so > the device may choose to utilize the behaviour of swiotlb to perform > attacks[2]. Based on a quick skim, this looks entirely reasonable to me. (I'm not a virtio maintainer or expert. I got my hands very dirty with virtio once dealing with the DMA mess, but that's about it.) --Andy
在 2021/6/9 上午12:24, Andy Lutomirski 写道: > On 6/3/21 10:53 PM, Jason Wang wrote: >> Hi: >> >> The virtio driver should not trust the device. This beame more urgent >> for the case of encrtpyed VM or VDUSE[1]. In both cases, technology >> like swiotlb/IOMMU is used to prevent the poking/mangling of memory >> from the device. But this is not sufficient since current virtio >> driver may trust what is stored in the descriptor table (coherent >> mapping) for performing the DMA operations like unmap and bounce so >> the device may choose to utilize the behaviour of swiotlb to perform >> attacks[2]. > Based on a quick skim, this looks entirely reasonable to me. > > (I'm not a virtio maintainer or expert. I got my hands very dirty with > virtio once dealing with the DMA mess, but that's about it.) > > --Andy Good to know that :) Thanks
On Fri, Jun 04, 2021 at 01:53:43PM +0800, Jason Wang wrote: > Hi: > > The virtio driver should not trust the device. This beame more urgent > for the case of encrtpyed VM or VDUSE[1]. In both cases, technology > like swiotlb/IOMMU is used to prevent the poking/mangling of memory > from the device. But this is not sufficient since current virtio > driver may trust what is stored in the descriptor table (coherent > mapping) for performing the DMA operations like unmap and bounce so > the device may choose to utilize the behaviour of swiotlb to perform > attacks[2]. > > To protect from a malicous device, this series store and use the > descriptor metadata in an auxiliay structure which can not be accessed > via swiotlb/device instead of the ones in the descriptor table. This > means the descriptor table is write-only from the view of the driver. > > Actually, we've almost achieved that through packed virtqueue and we > just need to fix a corner case of handling mapping errors. For split > virtqueue we just follow what's done in the packed. > > Note that we don't duplicate descriptor medata for indirect > descriptors since it uses stream mapping which is read only so it's > safe if the metadata of non-indirect descriptors are correct. > > For split virtqueue, the change increase the footprint due the the > auxiliary metadata but it's almost neglectlable in simple test like > pktgen and netperf TCP stream (slightly noticed in a 40GBE environment > with more CPU usage). > > Slightly tested with packed on/off, iommu on/of, swiotlb force/off in > the guest. > > Note that this series tries to fix the attack via descriptor > ring. The other cases (used ring and config space) will be fixed by > other series or patches. > > Please review. This needs a rebase - can you do it pls? > Changes from RFC V2: > - no code change > - twaeak the commit log a little bit > > Changes from RFC V1: > - Always use auxiliary metadata for split virtqueue > - Don't read from descripto when detaching indirect descriptor > > Jason Wang (7): > virtio-ring: maintain next in extra state for packed virtqueue > virtio_ring: rename vring_desc_extra_packed > virtio-ring: factor out desc_extra allocation > virtio_ring: secure handling of mapping errors > virtio_ring: introduce virtqueue_desc_add_split() > virtio: use err label in __vring_new_virtqueue() > virtio-ring: store DMA metadata in desc_extra for split virtqueue > > drivers/virtio/virtio_ring.c | 201 +++++++++++++++++++++++++---------- > 1 file changed, 144 insertions(+), 57 deletions(-) > > -- > 2.25.1