On Mon, Mar 07 2022, Halil Pasic <[email protected]> wrote: > Unlike most virtio features ACCESS_PLATFORM is considered mandatory by > QEMU, i.e. the driver must accept it if offered by the device. The > virtio specification says that the driver SHOULD accept the > ACCESS_PLATFORM feature if offered, and that the device MAY fail to > operate if ACCESS_PLATFORM was offered but not negotiated. > > While a SHOULD ain't exactly a MUST, we are certainly allowed to fail > the device when the driver fences ACCESS_PLATFORM. With commit > 2943b53f68 ("virtio: force VIRTIO_F_IOMMU_PLATFORM") we already made the > decision to do so whenever the get_dma_as() callback is implemented (by > the bus), which in practice means for the entirety of virtio-pci. > > That means, if the device needs to translate I/O addresses, then > ACCESS_PLATFORM is mandatory. The aforementioned commit tells us in the > commit message that this is for security reasons. More precisely if we > were to allow a less then trusted driver (e.g. an user-space driver, or > a nested guest) to make the device bypass the IOMMU by not negotiating > ACCESS_PLATFORM, then the guest kernel would have no ability to > control/police (by programming the IOMMU) what pieces of guest memory > the driver may manipulate using the device. Which would break security > assumptions within the guest. > > If ACCESS_PLATFORM is offered not because we want the device to utilize > an IOMMU and do address translation, but because the device does not > have access to the entire guest RAM, and needs the driver to grant > access to the bits it needs access to (e.g. confidential guest support), > we still require the guest to have the corresponding logic and to accept > ACCESS_PLATFORM. If the driver does not accept ACCESS_PLATFORM, then > things are bound to go wrong, and we may see failures much less graceful > than failing the device because the driver didn't negotiate > ACCESS_PLATFORM. > > So let us make ACCESS_PLATFORM mandatory for the driver regardless > of whether the get_dma_as() callback is implemented or not. > > Signed-off-by: Halil Pasic <[email protected]> > Fixes: 2943b53f68 ("virtio: force VIRTIO_F_IOMMU_PLATFORM") > > --- > v2 -> v3: > * Rebased onto the next branch of the > git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git > repository > v1 -> v2: > * Change comment: reflect that this is not about the verify > but also about the device features as seen by the driver (Connie) > RFC -> v1: > * Tweaked the commit message and fixed typos (Connie) > * Added two sentences discussing the security implications (Michael) > > This patch is based on: > https://www.mail-archive.com/[email protected]/msg866199.html > > During the review of "virtio: fix the condition for iommu_platform not > supported" Daniel raised the question why do we "force IOMMU_PLATFORM" > iff has_iommu && !!klass->get_dma_as. My answer to that was, that > this logic ain't right. > > While at it I used the opportunity to re-organize the code a little > and provide an explanatory comment. > --- > hw/virtio/virtio-bus.c | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-)
Reviewed-by: Cornelia Huck <[email protected]>
