linux-kernel - Re: [PATCH] vfio/type1: Add proper error unwind for vfio_iommu

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: <20200810182621.287f1689.cohuck@redhat.com>
Date:   Mon, 10 Aug 2020 18:26:21 +0200
From:   Cornelia Huck <cohuck@...hat.com>
To:     Alex Williamson <alex.williamson@...hat.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vfio/type1: Add proper error unwind for
 vfio_iommu_replay()

On Fri, 07 Aug 2020 14:14:48 -0600
Alex Williamson <alex.williamson@...hat.com> wrote:

> The vfio_iommu_replay() function does not currently unwind on error,
> yet it does pin pages, perform IOMMU mapping, and modify the vfio_dma
> structure to indicate IOMMU mapping.  The IOMMU mappings are torn down
> when the domain is destroyed, but the other actions go on to cause
> trouble later.  For example, the iommu->domain_list can be empty if we
> only have a non-IOMMU backed mdev attached.  We don't currently check
> if the list is empty before getting the first entry in the list, which
> leads to a bogus domain pointer.  If a vfio_dma entry is erroneously
> marked as iommu_mapped, we'll attempt to use that bogus pointer to
> retrieve the existing physical page addresses.
> 
> This is the scenario that uncovered this issue, attempting to hot-add
> a vfio-pci device to a container with an existing mdev device and DMA
> mappings, one of which could not be pinned, causing a failure adding
> the new group to the existing container and setting the conditions
> for a subsequent attempt to explode.
> 
> To resolve this, we can first check if the domain_list is empty so
> that we can reject replay of a bogus domain, should we ever encounter
> this inconsistent state again in the future.  The real fix though is
> to add the necessary unwind support, which means cleaning up the
> current pinning if an IOMMU mapping fails, then walking back through
> the r-b tree of DMA entries, reading from the IOMMU which ranges are
> mapped, and unmapping and unpinning those ranges.  To be able to do
> this, we also defer marking the DMA entry as IOMMU mapped until all
> entries are processed, in order to allow the unwind to know the
> disposition of each entry.
> 
> Fixes: a54eb55045ae ("vfio iommu type1: Add support for mediated devices")
> Signed-off-by: Alex Williamson <alex.williamson@...hat.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c |   71 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 66 insertions(+), 5 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@...hat.com>