From 350b8bdd689cd2ab2c67c8a86a0be86cfa0751a7 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Tue, 19 Aug 2014 19:14:50 +0800 Subject: [PATCH 1/6] kvm: iommu: fix the third parameter of kvm_iommu_put_pages (CVE-2014-3601) The third parameter of kvm_iommu_put_pages is wrong, It should be 'gfn - slot->base_gfn'. By making gfn very large, malicious guest or userspace can cause kvm to go to this error path, and subsequently to pass a huge value as size. Alternatively if gfn is small, then pages would be pinned but never unpinned, causing host memory leak and local DOS. Passing a reasonable but large value could be the most dangerous case, because it would unpin a page that should have stayed pinned, and thus allow the device to DMA into arbitrary memory. However, this cannot happen because of the condition that can trigger the error: - out of memory (where you can't allocate even a single page) should not be possible for the attacker to trigger - when exceeding the iommu's address space, guest pages after gfn will also exceed the iommu's address space, and inside kvm_iommu_put_pages() the iommu_iova_to_phys() will fail. The page thus would not be unpinned at all. Reported-by: Jack Morgenstein Cc: stable@vger.kernel.org Signed-off-by: Michael S. Tsirkin Signed-off-by: Paolo Bonzini --- virt/kvm/iommu.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c index 0df7d4b34dfe..714b94932312 100644 --- a/virt/kvm/iommu.c +++ b/virt/kvm/iommu.c @@ -61,6 +61,14 @@ static pfn_t kvm_pin_pages(struct kvm_memory_slot *slot, gfn_t gfn, return pfn; } +static void kvm_unpin_pages(struct kvm *kvm, pfn_t pfn, unsigned long npages) +{ + unsigned long i; + + for (i = 0; i < npages; ++i) + kvm_release_pfn_clean(pfn + i); +} + int kvm_iommu_map_pages(struct kvm *kvm, struct kvm_memory_slot *slot) { gfn_t gfn, end_gfn; @@ -123,6 +131,7 @@ int kvm_iommu_map_pages(struct kvm *kvm, struct kvm_memory_slot *slot) if (r) { printk(KERN_ERR "kvm_iommu_map_address:" "iommu failed to map pfn=%llx\n", pfn); + kvm_unpin_pages(kvm, pfn, page_size); goto unmap_pages; } @@ -134,7 +143,7 @@ int kvm_iommu_map_pages(struct kvm *kvm, struct kvm_memory_slot *slot) return 0; unmap_pages: - kvm_iommu_put_pages(kvm, slot->base_gfn, gfn); + kvm_iommu_put_pages(kvm, slot->base_gfn, gfn - slot->base_gfn); return r; } @@ -266,14 +275,6 @@ out_unlock: return r; } -static void kvm_unpin_pages(struct kvm *kvm, pfn_t pfn, unsigned long npages) -{ - unsigned long i; - - for (i = 0; i < npages; ++i) - kvm_release_pfn_clean(pfn + i); -} - static void kvm_iommu_put_pages(struct kvm *kvm, gfn_t base_gfn, unsigned long npages) { From c04fa5831d4d89dfbc88406f4a46f9846841a560 Mon Sep 17 00:00:00 2001 From: Alexey Kardashevskiy Date: Thu, 14 Aug 2014 15:03:07 +1000 Subject: [PATCH 2/6] PC, KVM, CMA: Fix regression caused by wrong get_order() use fc95ca7284bc54953165cba76c3228bd2cdb9591 claims that there is no functional change but this is not true as it calls get_order() (which takes bytes) where it should have called order_base_2() and the kernel stops on VM_BUG_ON(). This replaces get_order() with order_base_2() (round-up version of ilog2). Suggested-by: Paul Mackerras Cc: Alexander Graf Cc: Joonsoo Kim Cc: Benjamin Herrenschmidt Reviewed-by: Aneesh Kumar K.V Signed-off-by: Alexey Kardashevskiy Signed-off-by: Paolo Bonzini --- arch/powerpc/kvm/book3s_hv_builtin.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/powerpc/kvm/book3s_hv_builtin.c b/arch/powerpc/kvm/book3s_hv_builtin.c index 329d7fdd0a6a..b9615ba5b083 100644 --- a/arch/powerpc/kvm/book3s_hv_builtin.c +++ b/arch/powerpc/kvm/book3s_hv_builtin.c @@ -101,7 +101,7 @@ struct kvm_rma_info *kvm_alloc_rma() ri = kmalloc(sizeof(struct kvm_rma_info), GFP_KERNEL); if (!ri) return NULL; - page = cma_alloc(kvm_cma, kvm_rma_pages, get_order(kvm_rma_pages)); + page = cma_alloc(kvm_cma, kvm_rma_pages, order_base_2(kvm_rma_pages)); if (!page) goto err_out; atomic_set(&ri->use_count, 1); @@ -135,12 +135,12 @@ struct page *kvm_alloc_hpt(unsigned long nr_pages) { unsigned long align_pages = HPT_ALIGN_PAGES; - VM_BUG_ON(get_order(nr_pages) < KVM_CMA_CHUNK_ORDER - PAGE_SHIFT); + VM_BUG_ON(order_base_2(nr_pages) < KVM_CMA_CHUNK_ORDER - PAGE_SHIFT); /* Old CPUs require HPT aligned on a multiple of its size */ if (!cpu_has_feature(CPU_FTR_ARCH_206)) align_pages = nr_pages; - return cma_alloc(kvm_cma, nr_pages, get_order(align_pages)); + return cma_alloc(kvm_cma, nr_pages, order_base_2(align_pages)); } EXPORT_SYMBOL_GPL(kvm_alloc_hpt); From 3a6095a0173ad8f20c508446880558c9f9224324 Mon Sep 17 00:00:00 2001 From: Nadav Amit Date: Wed, 13 Aug 2014 16:50:13 +0300 Subject: [PATCH 3/6] KVM: x86: Avoid emulating instructions on #UD mistakenly Commit d40a6898e5 mistakenly caused instructions which are not marked as EmulateOnUD to be emulated upon #UD exception. The commit caused the check of whether the instruction flags include EmulateOnUD to never be evaluated. As a result instructions whose emulation is broken may be emulated. This fix moves the evaluation of EmulateOnUD so it would be evaluated. Signed-off-by: Nadav Amit [Tweak operand order in &&, remove EmulateOnUD where it's now superfluous. - Paolo] Signed-off-by: Paolo Bonzini --- arch/x86/kvm/emulate.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c index 56657b0bb3bb..ef117b842334 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -4394,8 +4394,11 @@ done_prefixes: ctxt->execute = opcode.u.execute; + if (unlikely(ctxt->ud) && likely(!(ctxt->d & EmulateOnUD))) + return EMULATION_FAILED; + if (unlikely(ctxt->d & - (NotImpl|EmulateOnUD|Stack|Op3264|Sse|Mmx|Intercept|CheckPerm))) { + (NotImpl|Stack|Op3264|Sse|Mmx|Intercept|CheckPerm))) { /* * These are copied unconditionally here, and checked unconditionally * in x86_emulate_insn. @@ -4406,9 +4409,6 @@ done_prefixes: if (ctxt->d & NotImpl) return EMULATION_FAILED; - if (!(ctxt->d & EmulateOnUD) && ctxt->ud) - return EMULATION_FAILED; - if (mode == X86EMUL_MODE_PROT64 && (ctxt->d & Stack)) ctxt->op_bytes = 8; From 9a4cfb27f7233610c5ddc82329c26d39683fad72 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 18 Aug 2014 13:15:51 +0200 Subject: [PATCH 4/6] KVM: x86: do not check CS.DPL against RPL during task switch This reverts the check added by commit 5045b468037d (KVM: x86: check CS.DPL against RPL during task switch, 2014-05-15). Although the CS.DPL=CS.RPL check is mentioned in table 7-1 of the SDM as causing a #TSS exception, it is not mentioned in table 6-6 that lists "invalid TSS conditions" which cause #TSS exceptions. In fact it causes some tests to fail, which pass on bare-metal. Keep the rest of the commit, since we will find new uses for it in 3.18. Reported-by: Nadav Amit Signed-off-by: Paolo Bonzini --- arch/x86/kvm/emulate.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c index ef117b842334..03954f7900f5 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -1491,9 +1491,6 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt, goto exception; break; case VCPU_SREG_CS: - if (in_task_switch && rpl != dpl) - goto exception; - if (!(seg_desc.type & 8)) goto exception; From 0d234daf7e0a3290a3a20c8087eefbd6335a5bd4 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 18 Aug 2014 16:39:48 +0200 Subject: [PATCH 5/6] Revert "KVM: x86: Increase the number of fixed MTRR regs to 10" This reverts commit 682367c494869008eb89ef733f196e99415ae862, which causes 32-bit SMP Windows 7 guests to panic. SeaBIOS has a limit on the number of MTRRs that it can handle, and this patch exceeded the limit. Better revert it. Thanks to Nadav Amit for debugging the cause. Cc: stable@nongnu.org Reported-by: Wanpeng Li Signed-off-by: Paolo Bonzini --- arch/x86/include/asm/kvm_host.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 572460175ba5..7c492ed9087b 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -95,7 +95,7 @@ static inline gfn_t gfn_to_index(gfn_t gfn, gfn_t base_gfn, int level) #define KVM_REFILL_PAGES 25 #define KVM_MAX_CPUID_ENTRIES 80 #define KVM_NR_FIXED_MTRR_REGION 88 -#define KVM_NR_VAR_MTRR 10 +#define KVM_NR_VAR_MTRR 8 #define ASYNC_PF_PER_VCPU 64 From 30d1e0e806e5b2fadc297ba78f2d7afd6ba309cf Mon Sep 17 00:00:00 2001 From: Chen Gang Date: Fri, 8 Aug 2014 23:37:59 +0800 Subject: [PATCH 6/6] virt/kvm/assigned-dev.c: Set 'dev->irq_source_id' to '-1' after free it As a generic function, deassign_guest_irq() assumes it can be called even if assign_guest_irq() is not be called successfully (which can be triggered by ioctl from user mode, indirectly). So for assign_guest_irq() failure process, need set 'dev->irq_source_id' to -1 after free 'dev->irq_source_id', or deassign_guest_irq() may free it again. Signed-off-by: Chen Gang Signed-off-by: Paolo Bonzini --- virt/kvm/assigned-dev.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c index bf06577fea51..5819a2708d7e 100644 --- a/virt/kvm/assigned-dev.c +++ b/virt/kvm/assigned-dev.c @@ -526,8 +526,10 @@ static int assign_guest_irq(struct kvm *kvm, dev->irq_requested_type |= guest_irq_type; if (dev->ack_notifier.gsi != -1) kvm_register_irq_ack_notifier(kvm, &dev->ack_notifier); - } else + } else { kvm_free_irq_source_id(kvm, dev->irq_source_id); + dev->irq_source_id = -1; + } return r; }