* [PATCH kernel v3 0/2] KVM: PPC: Check if IOMMU page is contained in the pinned physical page @ 2018-07-04 5:00 Alexey Kardashevskiy 2018-07-04 5:00 ` [PATCH kernel v3 1/2] vfio/spapr: Use IOMMU pageshift rather than pagesize Alexey Kardashevskiy 2018-07-04 5:00 ` [PATCH kernel v3 2/2] KVM: PPC: Check if IOMMU page is contained in the pinned physical page Alexey Kardashevskiy 0 siblings, 2 replies; 7+ messages in thread From: Alexey Kardashevskiy @ 2018-07-04 5:00 UTC (permalink / raw) To: linuxppc-dev Cc: Alexey Kardashevskiy, David Gibson, kvm-ppc, Alex Williamson, Michael Ellerman, Paul Mackerras This is to improve page boundaries checking and should probably be cc:stable. I came accross this while debugging nvlink2 passthrough but the lack of checking might be exploited by the existing userspace. Changes: v3: * enforced huge pages not to cross preregistered chunk boundaries v2: * 2/2: explicitly check for compound pages before calling compound_order() This is based on sha1 021c917 Linus Torvalds "Linux 4.18-rc3". Please comment. Thanks. Alexey Kardashevskiy (2): vfio/spapr: Use IOMMU pageshift rather than pagesize KVM: PPC: Check if IOMMU page is contained in the pinned physical page arch/powerpc/include/asm/mmu_context.h | 4 ++-- arch/powerpc/kvm/book3s_64_vio.c | 2 +- arch/powerpc/kvm/book3s_64_vio_hv.c | 6 +++-- arch/powerpc/mm/mmu_context_iommu.c | 41 +++++++++++++++++++++++++++++++--- drivers/vfio/vfio_iommu_spapr_tce.c | 10 ++++----- 5 files changed, 50 insertions(+), 13 deletions(-) -- 2.11.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH kernel v3 1/2] vfio/spapr: Use IOMMU pageshift rather than pagesize 2018-07-04 5:00 [PATCH kernel v3 0/2] KVM: PPC: Check if IOMMU page is contained in the pinned physical page Alexey Kardashevskiy @ 2018-07-04 5:00 ` Alexey Kardashevskiy 2018-07-04 5:00 ` [PATCH kernel v3 2/2] KVM: PPC: Check if IOMMU page is contained in the pinned physical page Alexey Kardashevskiy 1 sibling, 0 replies; 7+ messages in thread From: Alexey Kardashevskiy @ 2018-07-04 5:00 UTC (permalink / raw) To: linuxppc-dev Cc: Alexey Kardashevskiy, David Gibson, kvm-ppc, Alex Williamson, Michael Ellerman, Paul Mackerras The size is always equal to 1 page so let's use this. Later on this will be used for other checks which use page shifts to check the granularity of access. This should cause no behavioral change. Reviewed-by: David Gibson <david@gibson.dropbear.id.au> Acked-by: Alex Williamson <alex.williamson@redhat.com> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- As Alex suggested, this should go via the ppc tree which the next patch is going to (which is ppc-kvm, probably?). --- drivers/vfio/vfio_iommu_spapr_tce.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c index 759a5bd..2da5f05 100644 --- a/drivers/vfio/vfio_iommu_spapr_tce.c +++ b/drivers/vfio/vfio_iommu_spapr_tce.c @@ -457,13 +457,13 @@ static void tce_iommu_unuse_page(struct tce_container *container, } static int tce_iommu_prereg_ua_to_hpa(struct tce_container *container, - unsigned long tce, unsigned long size, + unsigned long tce, unsigned long shift, unsigned long *phpa, struct mm_iommu_table_group_mem_t **pmem) { long ret = 0; struct mm_iommu_table_group_mem_t *mem; - mem = mm_iommu_lookup(container->mm, tce, size); + mem = mm_iommu_lookup(container->mm, tce, 1ULL << shift); if (!mem) return -EINVAL; @@ -487,7 +487,7 @@ static void tce_iommu_unuse_page_v2(struct tce_container *container, if (!pua) return; - ret = tce_iommu_prereg_ua_to_hpa(container, *pua, IOMMU_PAGE_SIZE(tbl), + ret = tce_iommu_prereg_ua_to_hpa(container, *pua, tbl->it_page_shift, &hpa, &mem); if (ret) pr_debug("%s: tce %lx at #%lx was not cached, ret=%d\n", @@ -611,7 +611,7 @@ static long tce_iommu_build_v2(struct tce_container *container, entry + i); ret = tce_iommu_prereg_ua_to_hpa(container, - tce, IOMMU_PAGE_SIZE(tbl), &hpa, &mem); + tce, tbl->it_page_shift, &hpa, &mem); if (ret) break; -- 2.11.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH kernel v3 2/2] KVM: PPC: Check if IOMMU page is contained in the pinned physical page 2018-07-04 5:00 [PATCH kernel v3 0/2] KVM: PPC: Check if IOMMU page is contained in the pinned physical page Alexey Kardashevskiy 2018-07-04 5:00 ` [PATCH kernel v3 1/2] vfio/spapr: Use IOMMU pageshift rather than pagesize Alexey Kardashevskiy @ 2018-07-04 5:00 ` Alexey Kardashevskiy 2018-07-05 2:42 ` David Gibson 1 sibling, 1 reply; 7+ messages in thread From: Alexey Kardashevskiy @ 2018-07-04 5:00 UTC (permalink / raw) To: linuxppc-dev Cc: Alexey Kardashevskiy, David Gibson, kvm-ppc, Alex Williamson, Michael Ellerman, Paul Mackerras A VM which has: - a DMA capable device passed through to it (eg. network card); - running a malicious kernel that ignores H_PUT_TCE failure; - capability of using IOMMU pages bigger that physical pages can create an IOMMU mapping that exposes (for example) 16MB of the host physical memory to the device when only 64K was allocated to the VM. The remaining 16MB - 64K will be some other content of host memory, possibly including pages of the VM, but also pages of host kernel memory, host programs or other VMs. The attacking VM does not control the location of the page it can map, and is only allowed to map as many pages as it has pages of RAM. We already have a check in drivers/vfio/vfio_iommu_spapr_tce.c that an IOMMU page is contained in the physical page so the PCI hardware won't get access to unassigned host memory; however this check is missing in the KVM fastpath (H_PUT_TCE accelerated code). We were lucky so far and did not hit this yet as the very first time when the mapping happens we do not have tbl::it_userspace allocated yet and fall back to the userspace which in turn calls VFIO IOMMU driver, this fails and the guest does not retry, This stores the smallest preregistered page size in the preregistered region descriptor and changes the mm_iommu_xxx API to check this against the IOMMU page size. This only allows huge pages use if the entire preregistered block is backed with huge pages which are completely contained the preregistered chunk; otherwise this defaults to PAGE_SIZE. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- Changes: v3: * fixed upper limit for the page size * added checks that we don't register parts of a huge page v2: * explicitely check for compound pages before calling compound_order() --- The bug is: run QEMU _without_ hugepages (no -mempath) and tell it to advertise 16MB pages to the guest; a typical pseries guest will use 16MB for IOMMU pages without checking the mmu pagesize and this will fail at https://git.qemu.org/?p=qemu.git;a=blob;f=hw/vfio/common.c;h=fb396cf00ac40eb35967a04c9cc798ca896eed57;hb=refs/heads/master#l256 With the change, mapping will fail in KVM and the guest will print: mlx5_core 0000:00:00.0: ibm,create-pe-dma-window(2027) 0 8000000 20000000 18 1f returned 0 (liobn = 0x80000001 starting addr = 8000000 0) mlx5_core 0000:00:00.0: created tce table LIOBN 0x80000001 for /pci@800000020000000/ethernet@0 mlx5_core 0000:00:00.0: failed to map direct window for /pci@800000020000000/ethernet@0: -1 --- arch/powerpc/include/asm/mmu_context.h | 4 ++-- arch/powerpc/kvm/book3s_64_vio.c | 2 +- arch/powerpc/kvm/book3s_64_vio_hv.c | 6 +++-- arch/powerpc/mm/mmu_context_iommu.c | 41 +++++++++++++++++++++++++++++++--- drivers/vfio/vfio_iommu_spapr_tce.c | 2 +- 5 files changed, 46 insertions(+), 9 deletions(-) diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h index 896efa5..79d570c 100644 --- a/arch/powerpc/include/asm/mmu_context.h +++ b/arch/powerpc/include/asm/mmu_context.h @@ -35,9 +35,9 @@ extern struct mm_iommu_table_group_mem_t *mm_iommu_lookup_rm( extern struct mm_iommu_table_group_mem_t *mm_iommu_find(struct mm_struct *mm, unsigned long ua, unsigned long entries); extern long mm_iommu_ua_to_hpa(struct mm_iommu_table_group_mem_t *mem, - unsigned long ua, unsigned long *hpa); + unsigned long ua, unsigned int pageshift, unsigned long *hpa); extern long mm_iommu_ua_to_hpa_rm(struct mm_iommu_table_group_mem_t *mem, - unsigned long ua, unsigned long *hpa); + unsigned long ua, unsigned int pageshift, unsigned long *hpa); extern long mm_iommu_mapped_inc(struct mm_iommu_table_group_mem_t *mem); extern void mm_iommu_mapped_dec(struct mm_iommu_table_group_mem_t *mem); #endif diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c index d066e37..8c456fa 100644 --- a/arch/powerpc/kvm/book3s_64_vio.c +++ b/arch/powerpc/kvm/book3s_64_vio.c @@ -449,7 +449,7 @@ long kvmppc_tce_iommu_do_map(struct kvm *kvm, struct iommu_table *tbl, /* This only handles v2 IOMMU type, v1 is handled via ioctl() */ return H_TOO_HARD; - if (WARN_ON_ONCE(mm_iommu_ua_to_hpa(mem, ua, &hpa))) + if (WARN_ON_ONCE(mm_iommu_ua_to_hpa(mem, ua, tbl->it_page_shift, &hpa))) return H_HARDWARE; if (mm_iommu_mapped_inc(mem)) diff --git a/arch/powerpc/kvm/book3s_64_vio_hv.c b/arch/powerpc/kvm/book3s_64_vio_hv.c index 925fc31..5b298f5 100644 --- a/arch/powerpc/kvm/book3s_64_vio_hv.c +++ b/arch/powerpc/kvm/book3s_64_vio_hv.c @@ -279,7 +279,8 @@ static long kvmppc_rm_tce_iommu_do_map(struct kvm *kvm, struct iommu_table *tbl, if (!mem) return H_TOO_HARD; - if (WARN_ON_ONCE_RM(mm_iommu_ua_to_hpa_rm(mem, ua, &hpa))) + if (WARN_ON_ONCE_RM(mm_iommu_ua_to_hpa_rm(mem, ua, tbl->it_page_shift, + &hpa))) return H_HARDWARE; pua = (void *) vmalloc_to_phys(pua); @@ -469,7 +470,8 @@ long kvmppc_rm_h_put_tce_indirect(struct kvm_vcpu *vcpu, mem = mm_iommu_lookup_rm(vcpu->kvm->mm, ua, IOMMU_PAGE_SIZE_4K); if (mem) - prereg = mm_iommu_ua_to_hpa_rm(mem, ua, &tces) == 0; + prereg = mm_iommu_ua_to_hpa_rm(mem, ua, + IOMMU_PAGE_SHIFT_4K, &tces) == 0; } if (!prereg) { diff --git a/arch/powerpc/mm/mmu_context_iommu.c b/arch/powerpc/mm/mmu_context_iommu.c index abb4364..842dfce 100644 --- a/arch/powerpc/mm/mmu_context_iommu.c +++ b/arch/powerpc/mm/mmu_context_iommu.c @@ -27,6 +27,7 @@ struct mm_iommu_table_group_mem_t { struct rcu_head rcu; unsigned long used; atomic64_t mapped; + unsigned int pageshift; u64 ua; /* userspace address */ u64 entries; /* number of entries in hpas[] */ u64 *hpas; /* vmalloc'ed */ @@ -125,7 +126,8 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries, { struct mm_iommu_table_group_mem_t *mem; long i, j, ret = 0, locked_entries = 0; - struct page *page = NULL; + unsigned int pageshift; + struct page *page = NULL, *head = NULL; mutex_lock(&mem_list_mutex); @@ -159,6 +161,7 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries, goto unlock_exit; } + mem->pageshift = 64; mem->hpas = vzalloc(array_size(entries, sizeof(mem->hpas[0]))); if (!mem->hpas) { kfree(mem); @@ -199,9 +202,35 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries, } } populate: + pageshift = PAGE_SHIFT; + if (PageCompound(page)) { + /* Make sure huge page is contained completely */ + struct page *tmphead = compound_head(page); + unsigned int n = compound_order(tmphead); + + if (!head) { + /* Is it a head of a huge page? */ + if (page == tmphead) { + head = tmphead; + pageshift += n; + } + } else if (head == tmphead) { + /* Still same huge page, good */ + pageshift += n; + + /* End of the huge page */ + if (page - head == (1UL << n) - 1) + head = NULL; + } + } + mem->pageshift = min(mem->pageshift, pageshift); mem->hpas[i] = page_to_pfn(page) << PAGE_SHIFT; } + /* We have an incomplete huge page, default to PAGE_SHIFT */ + if (head) + mem->pageshift = PAGE_SHIFT; + atomic64_set(&mem->mapped, 1); mem->used = 1; mem->ua = ua; @@ -349,7 +378,7 @@ struct mm_iommu_table_group_mem_t *mm_iommu_find(struct mm_struct *mm, EXPORT_SYMBOL_GPL(mm_iommu_find); long mm_iommu_ua_to_hpa(struct mm_iommu_table_group_mem_t *mem, - unsigned long ua, unsigned long *hpa) + unsigned long ua, unsigned int pageshift, unsigned long *hpa) { const long entry = (ua - mem->ua) >> PAGE_SHIFT; u64 *va = &mem->hpas[entry]; @@ -357,6 +386,9 @@ long mm_iommu_ua_to_hpa(struct mm_iommu_table_group_mem_t *mem, if (entry >= mem->entries) return -EFAULT; + if (pageshift > mem->pageshift) + return -EFAULT; + *hpa = *va | (ua & ~PAGE_MASK); return 0; @@ -364,7 +396,7 @@ long mm_iommu_ua_to_hpa(struct mm_iommu_table_group_mem_t *mem, EXPORT_SYMBOL_GPL(mm_iommu_ua_to_hpa); long mm_iommu_ua_to_hpa_rm(struct mm_iommu_table_group_mem_t *mem, - unsigned long ua, unsigned long *hpa) + unsigned long ua, unsigned int pageshift, unsigned long *hpa) { const long entry = (ua - mem->ua) >> PAGE_SHIFT; void *va = &mem->hpas[entry]; @@ -373,6 +405,9 @@ long mm_iommu_ua_to_hpa_rm(struct mm_iommu_table_group_mem_t *mem, if (entry >= mem->entries) return -EFAULT; + if (pageshift > mem->pageshift) + return -EFAULT; + pa = (void *) vmalloc_to_phys(va); if (!pa) return -EFAULT; diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c index 2da5f05..7cd63b0 100644 --- a/drivers/vfio/vfio_iommu_spapr_tce.c +++ b/drivers/vfio/vfio_iommu_spapr_tce.c @@ -467,7 +467,7 @@ static int tce_iommu_prereg_ua_to_hpa(struct tce_container *container, if (!mem) return -EINVAL; - ret = mm_iommu_ua_to_hpa(mem, tce, phpa); + ret = mm_iommu_ua_to_hpa(mem, tce, shift, phpa); if (ret) return -EINVAL; -- 2.11.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH kernel v3 2/2] KVM: PPC: Check if IOMMU page is contained in the pinned physical page 2018-07-04 5:00 ` [PATCH kernel v3 2/2] KVM: PPC: Check if IOMMU page is contained in the pinned physical page Alexey Kardashevskiy @ 2018-07-05 2:42 ` David Gibson 2018-07-05 5:19 ` Alexey Kardashevskiy 0 siblings, 1 reply; 7+ messages in thread From: David Gibson @ 2018-07-05 2:42 UTC (permalink / raw) To: Alexey Kardashevskiy Cc: linuxppc-dev, kvm-ppc, Alex Williamson, Michael Ellerman, Paul Mackerras [-- Attachment #1: Type: text/plain, Size: 4494 bytes --] On Wed, Jul 04, 2018 at 03:00:52PM +1000, Alexey Kardashevskiy wrote: > A VM which has: > - a DMA capable device passed through to it (eg. network card); > - running a malicious kernel that ignores H_PUT_TCE failure; > - capability of using IOMMU pages bigger that physical pages > can create an IOMMU mapping that exposes (for example) 16MB of > the host physical memory to the device when only 64K was allocated to the VM. > > The remaining 16MB - 64K will be some other content of host memory, possibly > including pages of the VM, but also pages of host kernel memory, host > programs or other VMs. > > The attacking VM does not control the location of the page it can map, > and is only allowed to map as many pages as it has pages of RAM. > > We already have a check in drivers/vfio/vfio_iommu_spapr_tce.c that > an IOMMU page is contained in the physical page so the PCI hardware won't > get access to unassigned host memory; however this check is missing in > the KVM fastpath (H_PUT_TCE accelerated code). We were lucky so far and > did not hit this yet as the very first time when the mapping happens > we do not have tbl::it_userspace allocated yet and fall back to > the userspace which in turn calls VFIO IOMMU driver, this fails and > the guest does not retry, > > This stores the smallest preregistered page size in the preregistered > region descriptor and changes the mm_iommu_xxx API to check this against > the IOMMU page size. This only allows huge pages use if the entire > preregistered block is backed with huge pages which are completely > contained the preregistered chunk; otherwise this defaults to PAGE_SIZE. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> On the grounds that I think this version is safe, which the old one wasn't. However it still has some flaws.. [snip] > @@ -125,7 +126,8 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries, > { > struct mm_iommu_table_group_mem_t *mem; > long i, j, ret = 0, locked_entries = 0; > - struct page *page = NULL; > + unsigned int pageshift; > + struct page *page = NULL, *head = NULL; > > mutex_lock(&mem_list_mutex); > > @@ -159,6 +161,7 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries, > goto unlock_exit; > } > > + mem->pageshift = 64; > mem->hpas = vzalloc(array_size(entries, sizeof(mem->hpas[0]))); > if (!mem->hpas) { > kfree(mem); > @@ -199,9 +202,35 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries, > } > } > populate: > + pageshift = PAGE_SHIFT; > + if (PageCompound(page)) { > + /* Make sure huge page is contained completely */ > + struct page *tmphead = compound_head(page); > + unsigned int n = compound_order(tmphead); > + > + if (!head) { > + /* Is it a head of a huge page? */ > + if (page == tmphead) { > + head = tmphead; > + pageshift += n; > + } > + } else if (head == tmphead) { > + /* Still same huge page, good */ > + pageshift += n; > + > + /* End of the huge page */ > + if (page - head == (1UL << n) - 1) > + head = NULL; > + } > + } > + mem->pageshift = min(mem->pageshift, pageshift); > mem->hpas[i] = page_to_pfn(page) << PAGE_SHIFT; > } > > + /* We have an incomplete huge page, default to PAGE_SHIFT */ > + if (head) > + mem->pageshift = PAGE_SHIFT; > + So, if the user attempts to prereg a region which starts or ends in the middle of a hugepage, this logic will clamp the region's max page shift down to PAGE_SHIFT. That's safe, but not optimal. Suppose userspace had an area backed with 16MiB hugepages, and wanted to pre-reg a window that was 2MiB aligned, but not 16MiB aligned. It would still be safe to allow 2MiB TCEs, but the code above would clamp it down to 64kiB (or 4kiB). The code to do it is also pretty convoluted. I think you'd be better off initializing mem->pageshift to the largest possible natural alignment of the region: mem->pageshift = ctz64(ua | (entries << PAGE_SHIFT)); Then it should just be sufficient to clamp pageshift down to compound_order() + PAGE_SHIFT for each entry. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH kernel v3 2/2] KVM: PPC: Check if IOMMU page is contained in the pinned physical page 2018-07-05 2:42 ` David Gibson @ 2018-07-05 5:19 ` Alexey Kardashevskiy 2018-07-05 8:04 ` Alexey Kardashevskiy 2018-07-06 5:06 ` David Gibson 0 siblings, 2 replies; 7+ messages in thread From: Alexey Kardashevskiy @ 2018-07-05 5:19 UTC (permalink / raw) To: David Gibson Cc: linuxppc-dev, kvm-ppc, Alex Williamson, Michael Ellerman, Paul Mackerras [-- Attachment #1: Type: text/plain, Size: 4745 bytes --] On Thu, 5 Jul 2018 12:42:20 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Wed, Jul 04, 2018 at 03:00:52PM +1000, Alexey Kardashevskiy wrote: > > A VM which has: > > - a DMA capable device passed through to it (eg. network card); > > - running a malicious kernel that ignores H_PUT_TCE failure; > > - capability of using IOMMU pages bigger that physical pages > > can create an IOMMU mapping that exposes (for example) 16MB of > > the host physical memory to the device when only 64K was allocated to the VM. > > > > The remaining 16MB - 64K will be some other content of host memory, possibly > > including pages of the VM, but also pages of host kernel memory, host > > programs or other VMs. > > > > The attacking VM does not control the location of the page it can map, > > and is only allowed to map as many pages as it has pages of RAM. > > > > We already have a check in drivers/vfio/vfio_iommu_spapr_tce.c that > > an IOMMU page is contained in the physical page so the PCI hardware won't > > get access to unassigned host memory; however this check is missing in > > the KVM fastpath (H_PUT_TCE accelerated code). We were lucky so far and > > did not hit this yet as the very first time when the mapping happens > > we do not have tbl::it_userspace allocated yet and fall back to > > the userspace which in turn calls VFIO IOMMU driver, this fails and > > the guest does not retry, > > > > This stores the smallest preregistered page size in the preregistered > > region descriptor and changes the mm_iommu_xxx API to check this against > > the IOMMU page size. This only allows huge pages use if the entire > > preregistered block is backed with huge pages which are completely > > contained the preregistered chunk; otherwise this defaults to PAGE_SIZE. > > > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > On the grounds that I think this version is safe, which the old one > wasn't. However it still has some flaws.. > > [snip] > > @@ -125,7 +126,8 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries, > > { > > struct mm_iommu_table_group_mem_t *mem; > > long i, j, ret = 0, locked_entries = 0; > > - struct page *page = NULL; > > + unsigned int pageshift; > > + struct page *page = NULL, *head = NULL; > > > > mutex_lock(&mem_list_mutex); > > > > @@ -159,6 +161,7 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries, > > goto unlock_exit; > > } > > > > + mem->pageshift = 64; > > mem->hpas = vzalloc(array_size(entries, sizeof(mem->hpas[0]))); > > if (!mem->hpas) { > > kfree(mem); > > @@ -199,9 +202,35 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries, > > } > > } > > populate: > > + pageshift = PAGE_SHIFT; > > + if (PageCompound(page)) { > > + /* Make sure huge page is contained completely */ > > + struct page *tmphead = compound_head(page); > > + unsigned int n = compound_order(tmphead); > > + > > + if (!head) { > > + /* Is it a head of a huge page? */ > > + if (page == tmphead) { > > + head = tmphead; > > + pageshift += n; > > + } > > + } else if (head == tmphead) { > > + /* Still same huge page, good */ > > + pageshift += n; > > + > > + /* End of the huge page */ > > + if (page - head == (1UL << n) - 1) > > + head = NULL; > > + } > > + } > > + mem->pageshift = min(mem->pageshift, pageshift); > > mem->hpas[i] = page_to_pfn(page) << PAGE_SHIFT; > > } > > > > + /* We have an incomplete huge page, default to PAGE_SHIFT */ > > + if (head) > > + mem->pageshift = PAGE_SHIFT; > > + > > So, if the user attempts to prereg a region which starts or ends in > the middle of a hugepage, this logic will clamp the region's max page > shift down to PAGE_SHIFT. That's safe, but not optimal. > > Suppose userspace had an area backed with 16MiB hugepages, and wanted > to pre-reg a window that was 2MiB aligned, but not 16MiB aligned. It > would still be safe to allow 2MiB TCEs, but the code above would clamp > it down to 64kiB (or 4kiB). > > The code to do it is also pretty convoluted. > > I think you'd be better off initializing mem->pageshift to the largest > possible natural alignment of the region: > mem->pageshift = ctz64(ua | (entries << PAGE_SHIFT)); > > Then it should just be sufficient to clamp pageshift down to > compound_order() + PAGE_SHIFT for each entry. I like this better, just one question - does hugetlbfs guarantee the @ua alignment if backed with an actual huge page? -- Alexey [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH kernel v3 2/2] KVM: PPC: Check if IOMMU page is contained in the pinned physical page 2018-07-05 5:19 ` Alexey Kardashevskiy @ 2018-07-05 8:04 ` Alexey Kardashevskiy 2018-07-06 5:06 ` David Gibson 1 sibling, 0 replies; 7+ messages in thread From: Alexey Kardashevskiy @ 2018-07-05 8:04 UTC (permalink / raw) To: David Gibson Cc: linuxppc-dev, kvm-ppc, Alex Williamson, Michael Ellerman, Paul Mackerras [-- Attachment #1: Type: text/plain, Size: 5122 bytes --] On Thu, 5 Jul 2018 15:19:04 +1000 Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > On Thu, 5 Jul 2018 12:42:20 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Wed, Jul 04, 2018 at 03:00:52PM +1000, Alexey Kardashevskiy wrote: > > > A VM which has: > > > - a DMA capable device passed through to it (eg. network card); > > > - running a malicious kernel that ignores H_PUT_TCE failure; > > > - capability of using IOMMU pages bigger that physical pages > > > can create an IOMMU mapping that exposes (for example) 16MB of > > > the host physical memory to the device when only 64K was allocated to the VM. > > > > > > The remaining 16MB - 64K will be some other content of host memory, possibly > > > including pages of the VM, but also pages of host kernel memory, host > > > programs or other VMs. > > > > > > The attacking VM does not control the location of the page it can map, > > > and is only allowed to map as many pages as it has pages of RAM. > > > > > > We already have a check in drivers/vfio/vfio_iommu_spapr_tce.c that > > > an IOMMU page is contained in the physical page so the PCI hardware won't > > > get access to unassigned host memory; however this check is missing in > > > the KVM fastpath (H_PUT_TCE accelerated code). We were lucky so far and > > > did not hit this yet as the very first time when the mapping happens > > > we do not have tbl::it_userspace allocated yet and fall back to > > > the userspace which in turn calls VFIO IOMMU driver, this fails and > > > the guest does not retry, > > > > > > This stores the smallest preregistered page size in the preregistered > > > region descriptor and changes the mm_iommu_xxx API to check this against > > > the IOMMU page size. This only allows huge pages use if the entire > > > preregistered block is backed with huge pages which are completely > > > contained the preregistered chunk; otherwise this defaults to PAGE_SIZE. > > > > > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > > > On the grounds that I think this version is safe, which the old one > > wasn't. However it still has some flaws.. > > > > [snip] > > > @@ -125,7 +126,8 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries, > > > { > > > struct mm_iommu_table_group_mem_t *mem; > > > long i, j, ret = 0, locked_entries = 0; > > > - struct page *page = NULL; > > > + unsigned int pageshift; > > > + struct page *page = NULL, *head = NULL; > > > > > > mutex_lock(&mem_list_mutex); > > > > > > @@ -159,6 +161,7 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries, > > > goto unlock_exit; > > > } > > > > > > + mem->pageshift = 64; > > > mem->hpas = vzalloc(array_size(entries, sizeof(mem->hpas[0]))); > > > if (!mem->hpas) { > > > kfree(mem); > > > @@ -199,9 +202,35 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries, > > > } > > > } > > > populate: > > > + pageshift = PAGE_SHIFT; > > > + if (PageCompound(page)) { > > > + /* Make sure huge page is contained completely */ > > > + struct page *tmphead = compound_head(page); > > > + unsigned int n = compound_order(tmphead); > > > + > > > + if (!head) { > > > + /* Is it a head of a huge page? */ > > > + if (page == tmphead) { > > > + head = tmphead; > > > + pageshift += n; > > > + } > > > + } else if (head == tmphead) { > > > + /* Still same huge page, good */ > > > + pageshift += n; > > > + > > > + /* End of the huge page */ > > > + if (page - head == (1UL << n) - 1) > > > + head = NULL; > > > + } > > > + } > > > + mem->pageshift = min(mem->pageshift, pageshift); > > > mem->hpas[i] = page_to_pfn(page) << PAGE_SHIFT; > > > } > > > > > > + /* We have an incomplete huge page, default to PAGE_SHIFT */ > > > + if (head) > > > + mem->pageshift = PAGE_SHIFT; > > > + > > > > So, if the user attempts to prereg a region which starts or ends in > > the middle of a hugepage, this logic will clamp the region's max page > > shift down to PAGE_SHIFT. That's safe, but not optimal. > > > > Suppose userspace had an area backed with 16MiB hugepages, and wanted > > to pre-reg a window that was 2MiB aligned, but not 16MiB aligned. It > > would still be safe to allow 2MiB TCEs, but the code above would clamp > > it down to 64kiB (or 4kiB). > > > > The code to do it is also pretty convoluted. > > > > I think you'd be better off initializing mem->pageshift to the largest > > possible natural alignment of the region: > > mem->pageshift = ctz64(ua | (entries << PAGE_SHIFT)); > > > > Then it should just be sufficient to clamp pageshift down to > > compound_order() + PAGE_SHIFT for each entry. > > > I like this better, just one question - does hugetlbfs guarantee the @ua > alignment if backed with an actual huge page? Turns out it does, never mind, posted v4. Cheers. -- Alexey [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH kernel v3 2/2] KVM: PPC: Check if IOMMU page is contained in the pinned physical page 2018-07-05 5:19 ` Alexey Kardashevskiy 2018-07-05 8:04 ` Alexey Kardashevskiy @ 2018-07-06 5:06 ` David Gibson 1 sibling, 0 replies; 7+ messages in thread From: David Gibson @ 2018-07-06 5:06 UTC (permalink / raw) To: Alexey Kardashevskiy Cc: linuxppc-dev, kvm-ppc, Alex Williamson, Michael Ellerman, Paul Mackerras [-- Attachment #1: Type: text/plain, Size: 6662 bytes --] On Thu, Jul 05, 2018 at 03:19:04PM +1000, Alexey Kardashevskiy wrote: > On Thu, 5 Jul 2018 12:42:20 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Wed, Jul 04, 2018 at 03:00:52PM +1000, Alexey Kardashevskiy wrote: > > > A VM which has: > > > - a DMA capable device passed through to it (eg. network card); > > > - running a malicious kernel that ignores H_PUT_TCE failure; > > > - capability of using IOMMU pages bigger that physical pages > > > can create an IOMMU mapping that exposes (for example) 16MB of > > > the host physical memory to the device when only 64K was allocated to the VM. > > > > > > The remaining 16MB - 64K will be some other content of host memory, possibly > > > including pages of the VM, but also pages of host kernel memory, host > > > programs or other VMs. > > > > > > The attacking VM does not control the location of the page it can map, > > > and is only allowed to map as many pages as it has pages of RAM. > > > > > > We already have a check in drivers/vfio/vfio_iommu_spapr_tce.c that > > > an IOMMU page is contained in the physical page so the PCI hardware won't > > > get access to unassigned host memory; however this check is missing in > > > the KVM fastpath (H_PUT_TCE accelerated code). We were lucky so far and > > > did not hit this yet as the very first time when the mapping happens > > > we do not have tbl::it_userspace allocated yet and fall back to > > > the userspace which in turn calls VFIO IOMMU driver, this fails and > > > the guest does not retry, > > > > > > This stores the smallest preregistered page size in the preregistered > > > region descriptor and changes the mm_iommu_xxx API to check this against > > > the IOMMU page size. This only allows huge pages use if the entire > > > preregistered block is backed with huge pages which are completely > > > contained the preregistered chunk; otherwise this defaults to PAGE_SIZE. > > > > > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > > > On the grounds that I think this version is safe, which the old one > > wasn't. However it still has some flaws.. > > > > [snip] > > > @@ -125,7 +126,8 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries, > > > { > > > struct mm_iommu_table_group_mem_t *mem; > > > long i, j, ret = 0, locked_entries = 0; > > > - struct page *page = NULL; > > > + unsigned int pageshift; > > > + struct page *page = NULL, *head = NULL; > > > > > > mutex_lock(&mem_list_mutex); > > > > > > @@ -159,6 +161,7 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries, > > > goto unlock_exit; > > > } > > > > > > + mem->pageshift = 64; > > > mem->hpas = vzalloc(array_size(entries, sizeof(mem->hpas[0]))); > > > if (!mem->hpas) { > > > kfree(mem); > > > @@ -199,9 +202,35 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries, > > > } > > > } > > > populate: > > > + pageshift = PAGE_SHIFT; > > > + if (PageCompound(page)) { > > > + /* Make sure huge page is contained completely */ > > > + struct page *tmphead = compound_head(page); > > > + unsigned int n = compound_order(tmphead); > > > + > > > + if (!head) { > > > + /* Is it a head of a huge page? */ > > > + if (page == tmphead) { > > > + head = tmphead; > > > + pageshift += n; > > > + } > > > + } else if (head == tmphead) { > > > + /* Still same huge page, good */ > > > + pageshift += n; > > > + > > > + /* End of the huge page */ > > > + if (page - head == (1UL << n) - 1) > > > + head = NULL; > > > + } > > > + } > > > + mem->pageshift = min(mem->pageshift, pageshift); > > > mem->hpas[i] = page_to_pfn(page) << PAGE_SHIFT; > > > } > > > > > > + /* We have an incomplete huge page, default to PAGE_SHIFT */ > > > + if (head) > > > + mem->pageshift = PAGE_SHIFT; > > > + > > > > So, if the user attempts to prereg a region which starts or ends in > > the middle of a hugepage, this logic will clamp the region's max page > > shift down to PAGE_SHIFT. That's safe, but not optimal. > > > > Suppose userspace had an area backed with 16MiB hugepages, and wanted > > to pre-reg a window that was 2MiB aligned, but not 16MiB aligned. It > > would still be safe to allow 2MiB TCEs, but the code above would clamp > > it down to 64kiB (or 4kiB). > > > > The code to do it is also pretty convoluted. > > > > I think you'd be better off initializing mem->pageshift to the largest > > possible natural alignment of the region: > > mem->pageshift = ctz64(ua | (entries << PAGE_SHIFT)); > > > > Then it should just be sufficient to clamp pageshift down to > > compound_order() + PAGE_SHIFT for each entry. > > > I like this better, just one question - does hugetlbfs guarantee the @ua > alignment if backed with an actual huge page? So, yeah it does, as you determined. And it has to - I don't know of any MMU that allows for large pages that aren't naturally aligned, so the uas would have to be aligned to actually map the pages into userspace. But... there's another more subtle case that I'm less sure about. What you're actually checking for here is a compound page on the physical side. A hugetlbfs mapping in userspace is the main case where I'd expect that, but, I'm not absolutely certain there can't be some other case where a compound page is used to back a normal 64k mapping in a user process. If that is possible, it would probably also be possible for the UA to end up misaligned with the compound page's natural alignment. I don't know of any case where that could happen, but I'm far from confident it doesn't exist. Things to consider: - mapping hugetlbfs, then mremap()ing part of it - a SHARED mapping, where it's aligned in one process and gets THPed, but is not aligned in the other - mmap() from a device or subsystem that provides some kind of IO or special memory that's handled with compound pages on the kernel side, but is just mapped into userspace with regular 64k PTEs - One process mapping libhugetlbfs, then another (say a debugger_ attempting to map the first process's address space via /proc/pid/mem) - ..and that's just the ones I could think of quickly -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-07-06 5:13 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-07-04 5:00 [PATCH kernel v3 0/2] KVM: PPC: Check if IOMMU page is contained in the pinned physical page Alexey Kardashevskiy 2018-07-04 5:00 ` [PATCH kernel v3 1/2] vfio/spapr: Use IOMMU pageshift rather than pagesize Alexey Kardashevskiy 2018-07-04 5:00 ` [PATCH kernel v3 2/2] KVM: PPC: Check if IOMMU page is contained in the pinned physical page Alexey Kardashevskiy 2018-07-05 2:42 ` David Gibson 2018-07-05 5:19 ` Alexey Kardashevskiy 2018-07-05 8:04 ` Alexey Kardashevskiy 2018-07-06 5:06 ` David Gibson
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 NNTP newsgroup(s).