From: Alex Williamson <alex.williamson@redhat.com>
To: Yan Zhao <yan.y.zhao@intel.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
x86@kernel.org, jgg@nvidia.com, kevin.tian@intel.com,
iommu@lists.linux.dev, pbonzini@redhat.com, seanjc@google.com,
dave.hansen@linux.intel.com, luto@kernel.org,
peterz@infradead.org, tglx@linutronix.de, mingo@redhat.com,
bp@alien8.de, hpa@zytor.com, corbet@lwn.net, joro@8bytes.org,
will@kernel.org, robin.murphy@arm.com, baolu.lu@linux.intel.com,
yi.l.liu@intel.com
Subject: Re: [PATCH 4/5] vfio/type1: Flush CPU caches on DMA pages in non-coherent domains
Date: Thu, 9 May 2024 12:10:49 -0600 [thread overview]
Message-ID: <20240509121049.58238a6f.alex.williamson@redhat.com> (raw)
In-Reply-To: <20240507062138.20465-1-yan.y.zhao@intel.com>
On Tue, 7 May 2024 14:21:38 +0800
Yan Zhao <yan.y.zhao@intel.com> wrote:
> Flush CPU cache on DMA pages before mapping them into the first
> non-coherent domain (domain that does not enforce cache coherency, i.e. CPU
> caches are not force-snooped) and after unmapping them from the last
> domain.
>
> Devices attached to non-coherent domains can execute non-coherent DMAs
> (DMAs that lack CPU cache snooping) to access physical memory with CPU
> caches bypassed.
>
> Such a scenario could be exploited by a malicious guest, allowing them to
> access stale host data in memory rather than the data initialized by the
> host (e.g., zeros) in the cache, thus posing a risk of information leakage
> attack.
>
> Furthermore, the host kernel (e.g. a ksm thread) might encounter
> inconsistent data between the CPU cache and memory (left by a malicious
> guest) after a page is unpinned for DMA but before it's recycled.
>
> Therefore, it is required to flush the CPU cache before a page is
> accessible to non-coherent DMAs and after the page is inaccessible to
> non-coherent DMAs.
>
> However, the CPU cache is not flushed immediately when the page is unmapped
> from the last non-coherent domain. Instead, the flushing is performed
> lazily, right before the page is unpinned.
> Take the following example to illustrate the process. The CPU cache is
> flushed right before step 2 and step 5.
> 1. A page is mapped into a coherent domain.
> 2. The page is mapped into a non-coherent domain.
> 3. The page is unmapped from the non-coherent domain e.g.due to hot-unplug.
> 4. The page is unmapped from the coherent domain.
> 5. The page is unpinned.
>
> Reasons for adopting this lazily flushing design include:
> - There're several unmap paths and only one unpin path. Lazily flush before
> unpin wipes out the inconsistency between cache and physical memory
> before a page is globally visible and produces code that is simpler, more
> maintainable and easier to backport.
> - Avoid dividing a large unmap range into several smaller ones or
> allocating additional memory to hold IOVA to HPA relationship.
>
> Reported-by: Jason Gunthorpe <jgg@nvidia.com>
> Closes: https://lore.kernel.org/lkml/20240109002220.GA439767@nvidia.com
> Fixes: 73fa0d10d077 ("vfio: Type1 IOMMU implementation")
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Jason Gunthorpe <jgg@nvidia.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> ---
> drivers/vfio/vfio_iommu_type1.c | 51 +++++++++++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index b5c15fe8f9fc..ce873f4220bf 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -74,6 +74,7 @@ struct vfio_iommu {
> bool v2;
> bool nesting;
> bool dirty_page_tracking;
> + bool has_noncoherent_domain;
> struct list_head emulated_iommu_groups;
> };
>
> @@ -99,6 +100,7 @@ struct vfio_dma {
> unsigned long *bitmap;
> struct mm_struct *mm;
> size_t locked_vm;
> + bool cache_flush_required; /* For noncoherent domain */
Poor packing, minimally this should be grouped with the other bools in
the structure, longer term they should likely all be converted to
bit fields.
> };
>
> struct vfio_batch {
> @@ -716,6 +718,9 @@ static long vfio_unpin_pages_remote(struct vfio_dma *dma, dma_addr_t iova,
> long unlocked = 0, locked = 0;
> long i;
>
> + if (dma->cache_flush_required)
> + arch_clean_nonsnoop_dma(pfn << PAGE_SHIFT, npage << PAGE_SHIFT);
> +
> for (i = 0; i < npage; i++, iova += PAGE_SIZE) {
> if (put_pfn(pfn++, dma->prot)) {
> unlocked++;
> @@ -1099,6 +1104,8 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
> &iotlb_gather);
> }
>
> + dma->cache_flush_required = false;
> +
> if (do_accounting) {
> vfio_lock_acct(dma, -unlocked, true);
> return 0;
> @@ -1120,6 +1127,21 @@ static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
> iommu->dma_avail++;
> }
>
> +static void vfio_update_noncoherent_domain_state(struct vfio_iommu *iommu)
> +{
> + struct vfio_domain *domain;
> + bool has_noncoherent = false;
> +
> + list_for_each_entry(domain, &iommu->domain_list, next) {
> + if (domain->enforce_cache_coherency)
> + continue;
> +
> + has_noncoherent = true;
> + break;
> + }
> + iommu->has_noncoherent_domain = has_noncoherent;
> +}
This should be merged with vfio_domains_have_enforce_cache_coherency()
and the VFIO_DMA_CC_IOMMU extension (if we keep it, see below).
> +
> static void vfio_update_pgsize_bitmap(struct vfio_iommu *iommu)
> {
> struct vfio_domain *domain;
> @@ -1455,6 +1477,12 @@ static int vfio_pin_map_dma(struct vfio_iommu *iommu, struct vfio_dma *dma,
>
> vfio_batch_init(&batch);
>
> + /*
> + * Record necessity to flush CPU cache to make sure CPU cache is flushed
> + * for both pin & map and unmap & unpin (for unwind) paths.
> + */
> + dma->cache_flush_required = iommu->has_noncoherent_domain;
> +
> while (size) {
> /* Pin a contiguous chunk of memory */
> npage = vfio_pin_pages_remote(dma, vaddr + dma->size,
> @@ -1466,6 +1494,10 @@ static int vfio_pin_map_dma(struct vfio_iommu *iommu, struct vfio_dma *dma,
> break;
> }
>
> + if (dma->cache_flush_required)
> + arch_clean_nonsnoop_dma(pfn << PAGE_SHIFT,
> + npage << PAGE_SHIFT);
> +
> /* Map it! */
> ret = vfio_iommu_map(iommu, iova + dma->size, pfn, npage,
> dma->prot);
> @@ -1683,9 +1715,14 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
> for (; n; n = rb_next(n)) {
> struct vfio_dma *dma;
> dma_addr_t iova;
> + bool cache_flush_required;
>
> dma = rb_entry(n, struct vfio_dma, node);
> iova = dma->iova;
> + cache_flush_required = !domain->enforce_cache_coherency &&
> + !dma->cache_flush_required;
> + if (cache_flush_required)
> + dma->cache_flush_required = true;
The variable name here isn't accurate and the logic is confusing. If
the domain does not enforce coherency and the mapping is not tagged as
requiring a cache flush, then we need to mark the mapping as requiring
a cache flush. So the variable state is something more akin to
set_cache_flush_required. But all we're saving with this is a
redundant set if the mapping is already tagged as requiring a cache
flush, so it could really be simplified to:
dma->cache_flush_required = !domain->enforce_cache_coherency;
It might add more clarity to just name the mapping flag
dma->mapped_noncoherent.
>
> while (iova < dma->iova + dma->size) {
> phys_addr_t phys;
> @@ -1737,6 +1774,9 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
> size = npage << PAGE_SHIFT;
> }
>
> + if (cache_flush_required)
> + arch_clean_nonsnoop_dma(phys, size);
> +
I agree with others as well that this arch callback should be named
something relative to the cache-flush/write-back operation that it
actually performs instead of the overall reason for us requiring it.
> ret = iommu_map(domain->domain, iova, phys, size,
> dma->prot | IOMMU_CACHE,
> GFP_KERNEL_ACCOUNT);
> @@ -1801,6 +1841,7 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
> vfio_unpin_pages_remote(dma, iova, phys >> PAGE_SHIFT,
> size >> PAGE_SHIFT, true);
> }
> + dma->cache_flush_required = false;
> }
>
> vfio_batch_fini(&batch);
> @@ -1828,6 +1869,9 @@ static void vfio_test_domain_fgsp(struct vfio_domain *domain, struct list_head *
> if (!pages)
> return;
>
> + if (!domain->enforce_cache_coherency)
> + arch_clean_nonsnoop_dma(page_to_phys(pages), PAGE_SIZE * 2);
> +
> list_for_each_entry(region, regions, list) {
> start = ALIGN(region->start, PAGE_SIZE * 2);
> if (start >= region->end || (region->end - start < PAGE_SIZE * 2))
> @@ -1847,6 +1891,9 @@ static void vfio_test_domain_fgsp(struct vfio_domain *domain, struct list_head *
> break;
> }
>
> + if (!domain->enforce_cache_coherency)
> + arch_clean_nonsnoop_dma(page_to_phys(pages), PAGE_SIZE * 2);
> +
Seems like this use case isn't subject to the unmap aspect since these
are kernel allocated and freed pages rather than userspace pages.
There's not an "ongoing use of the page" concern.
The window of opportunity for a device to discover and exploit the
mapping side issue appears almost impossibly small.
> __free_pages(pages, order);
> }
>
> @@ -2308,6 +2355,8 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>
> list_add(&domain->next, &iommu->domain_list);
> vfio_update_pgsize_bitmap(iommu);
> + if (!domain->enforce_cache_coherency)
> + vfio_update_noncoherent_domain_state(iommu);
Why isn't this simply:
if (!domain->enforce_cache_coherency)
iommu->has_noncoherent_domain = true;
Or maybe:
if (!domain->enforce_cache_coherency)
iommu->noncoherent_domains++;
> done:
> /* Delete the old one and insert new iova list */
> vfio_iommu_iova_insert_copy(iommu, &iova_copy);
> @@ -2508,6 +2557,8 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
> }
> iommu_domain_free(domain->domain);
> list_del(&domain->next);
> + if (!domain->enforce_cache_coherency)
> + vfio_update_noncoherent_domain_state(iommu);
If we were to just track the number of noncoherent domains, this could
simply be iommu->noncoherent_domains-- and VFIO_DMA_CC_DMA could be:
return iommu->noncoherent_domains ? 1 : 0;
Maybe there should be wrappers for list_add() and list_del() relative
to the iommu domain list to make it just be a counter. Thanks,
Alex
> kfree(domain);
> vfio_iommu_aper_expand(iommu, &iova_copy);
> vfio_update_pgsize_bitmap(iommu);
next prev parent reply other threads:[~2024-05-09 18:10 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-07 6:18 [PATCH 0/5] Enforce CPU cache flush for non-coherent device assignment Yan Zhao
2024-05-07 6:19 ` [PATCH 1/5] x86/pat: Let pat_pfn_immune_to_uc_mtrr() check MTRR for untracked PAT range Yan Zhao
2024-05-07 8:26 ` Tian, Kevin
2024-05-07 9:12 ` Yan Zhao
2024-05-08 22:14 ` Alex Williamson
2024-05-09 3:36 ` Yan Zhao
2024-05-16 7:42 ` Tian, Kevin
2024-05-16 14:07 ` Sean Christopherson
2024-05-20 2:36 ` Tian, Kevin
2024-05-07 6:20 ` [PATCH 2/5] KVM: x86/mmu: Fine-grained check of whether a invalid & RAM PFN is MMIO Yan Zhao
2024-05-07 8:39 ` Tian, Kevin
2024-05-07 9:19 ` Yan Zhao
2024-05-07 6:20 ` [PATCH 3/5] x86/mm: Introduce and export interface arch_clean_nonsnoop_dma() Yan Zhao
2024-05-07 8:51 ` Tian, Kevin
2024-05-07 9:40 ` Yan Zhao
2024-05-20 14:07 ` Christoph Hellwig
2024-05-07 6:21 ` [PATCH 4/5] vfio/type1: Flush CPU caches on DMA pages in non-coherent domains Yan Zhao
2024-05-09 18:10 ` Alex Williamson [this message]
2024-05-10 10:31 ` Yan Zhao
2024-05-10 16:57 ` Alex Williamson
2024-05-13 7:11 ` Yan Zhao
2024-05-16 7:53 ` Tian, Kevin
2024-05-16 8:34 ` Tian, Kevin
2024-05-16 20:31 ` Alex Williamson
2024-05-17 17:11 ` Jason Gunthorpe
2024-05-20 2:52 ` Tian, Kevin
2024-05-16 20:50 ` Alex Williamson
2024-05-17 3:11 ` Yan Zhao
2024-05-17 4:44 ` Alex Williamson
2024-05-17 5:00 ` Yan Zhao
2024-05-07 6:22 ` [PATCH 5/5] iommufd: " Yan Zhao
2024-05-09 14:13 ` Jason Gunthorpe
2024-05-10 8:03 ` Yan Zhao
2024-05-10 13:29 ` Jason Gunthorpe
2024-05-13 7:43 ` Yan Zhao
2024-05-14 15:11 ` Jason Gunthorpe
2024-05-15 7:06 ` Yan Zhao
2024-05-15 20:43 ` Jason Gunthorpe
2024-05-16 2:32 ` Yan Zhao
2024-05-16 8:38 ` Tian, Kevin
2024-05-16 9:48 ` Yan Zhao
2024-05-17 17:04 ` Jason Gunthorpe
2024-05-20 2:45 ` Yan Zhao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240509121049.58238a6f.alex.williamson@redhat.com \
--to=alex.williamson@redhat.com \
--cc=baolu.lu@linux.intel.com \
--cc=bp@alien8.de \
--cc=corbet@lwn.net \
--cc=dave.hansen@linux.intel.com \
--cc=hpa@zytor.com \
--cc=iommu@lists.linux.dev \
--cc=jgg@nvidia.com \
--cc=joro@8bytes.org \
--cc=kevin.tian@intel.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mingo@redhat.com \
--cc=pbonzini@redhat.com \
--cc=peterz@infradead.org \
--cc=robin.murphy@arm.com \
--cc=seanjc@google.com \
--cc=tglx@linutronix.de \
--cc=will@kernel.org \
--cc=x86@kernel.org \
--cc=yan.y.zhao@intel.com \
--cc=yi.l.liu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).