From 4552269c2bb3d34a3f3102f72c0edc82c70138ad Mon Sep 17 00:00:00 2001 From: Jan Beulich Date: Wed, 26 Jun 2013 15:37:33 +0200 Subject: x86: fix page refcount handling in page table pin error path In the original patch 7 of the series addressing XSA-45 I mistakenly took the addition of the call to get_page_light() in alloc_page_type() to cover two decrements that would happen: One for the PGT_partial bit that is getting set along with the call, and the other for the page reference the caller hold (and would be dropping on its error path). But of course the additional page reference is tied to the PGT_partial bit, and hence any caller of a function that may leave ->arch.old_guest_table non-NULL for error cleanup purposes has to make sure a respective page reference gets retained. Similar issues were then also spotted elsewhere: In effect all callers of get_page_type_preemptible() need to deal with errors in similar ways. To make sure error handling can work this way without leaking page references, a respective assertion gets added to that function. This is CVE-2013-1432 / XSA-58. Reported-by: Andrew Cooper Signed-off-by: Jan Beulich Tested-by: Andrew Cooper Reviewed-by: Tim Deegan master commit: 9b167bd2f394f821ae3252d74a15704a4bf91f6d master date: 2013-06-26 15:32:58 +0200 --- xen/arch/x86/domain.c | 27 +++++++++++++++++++++------ xen/arch/x86/mm.c | 6 ++++-- xen/include/asm-x86/mm.h | 1 + 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 7b544b27e8..5a223ce088 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -798,6 +798,10 @@ int arch_set_info_guest( if ( v->vcpu_id == 0 ) d->vm_assist = c(vm_assist); + rc = put_old_guest_table(current); + if ( rc ) + return rc; + if ( !compat ) rc = (int)set_gdt(v, c.nat->gdt_frames, c.nat->gdt_ents); #ifdef CONFIG_COMPAT @@ -840,18 +844,24 @@ int arch_set_info_guest( } else { - /* - * Since v->arch.guest_table{,_user} are both NULL, this effectively - * is just a call to put_old_guest_table(). - */ if ( !compat ) - rc = vcpu_destroy_pagetables(v); + rc = put_old_guest_table(v); if ( !rc ) rc = get_page_type_preemptible(cr3_page, !compat ? PGT_root_page_table : PGT_l3_page_table); - if ( rc == -EINTR ) + switch ( rc ) + { + case -EINTR: rc = -EAGAIN; + case -EAGAIN: + case 0: + break; + default: + if ( cr3_page == current->arch.old_guest_table ) + cr3_page = NULL; + break; + } } if ( rc ) @@ -883,6 +893,11 @@ int arch_set_info_guest( pagetable_get_page(v->arch.guest_table); v->arch.guest_table = pagetable_null(); break; + default: + if ( cr3_page == current->arch.old_guest_table ) + cr3_page = NULL; + case 0: + break; } } diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 29b0ede4b0..057404b6db 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -682,7 +682,8 @@ static int get_page_and_type_from_pagenr(unsigned long page_nr, get_page_type_preemptible(page, type) : (get_page_type(page, type) ? 0 : -EINVAL)); - if ( unlikely(rc) && partial >= 0 ) + if ( unlikely(rc) && partial >= 0 && + (!preemptible || page != current->arch.old_guest_table) ) put_page(page); return rc; @@ -2555,6 +2556,7 @@ int put_page_type_preemptible(struct page_info *page) int get_page_type_preemptible(struct page_info *page, unsigned long type) { + ASSERT(!current->arch.old_guest_table); return __get_page_type(page, type, 1); } @@ -2765,7 +2767,7 @@ static void put_superpage(unsigned long mfn) #endif -static int put_old_guest_table(struct vcpu *v) +int put_old_guest_table(struct vcpu *v) { int rc; diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h index df7161a426..905efddc3c 100644 --- a/xen/include/asm-x86/mm.h +++ b/xen/include/asm-x86/mm.h @@ -337,6 +337,7 @@ void put_page_type(struct page_info *page); int get_page_type(struct page_info *page, unsigned long type); int put_page_type_preemptible(struct page_info *page); int get_page_type_preemptible(struct page_info *page, unsigned long type); +int put_old_guest_table(struct vcpu *); int get_page_from_l1e( l1_pgentry_t l1e, struct domain *l1e_owner, struct domain *pg_owner); void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner); -- cgit v1.2.3