linux-kernel - Re: [PATCH 0/7] Do not read from descriptor ring

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20210711120824-mutt-send-email-mst@kernel.org>
Date:   Sun, 11 Jul 2021 12:08:36 -0400
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org, xieyongji@...edance.com,
        stefanha@...hat.com, file@...t.tu-berlin.de, ashish.kalra@....com,
        konrad.wilk@...cle.com, kvm@...r.kernel.org, hch@...radead.org,
        ak@...ux.intel.com, luto@...nel.org
Subject: Re: [PATCH 0/7] Do not read from descriptor ring

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