From fa957039018e1470737e785d849e5eee12ae3786 Mon Sep 17 00:00:00 2001 From: Andrew Cooper Date: Mon, 4 Nov 2013 14:55:12 +0100 Subject: [PATCH] gnttab: correct locking order reversal Coverity ID 1087189 Correct a lock order reversal between a domains page allocation and grant table locks. This is CVE-2013-4494 / XSA-73. Signed-off-by: Andrew Cooper Consolidate error handling. Signed-off-by: Jan Beulich Reviewed-by: Keir Fraser Tested-by: Matthew Daley master commit: a321abc6d8122f8cb3928066cc74363c4fdddcfd master date: 2013-11-04 10:06:36 +0100 --- xen/common/grant_table.c | 51 +++++++++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 8 deletions(-) diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index e0f91ab9bc..156ef9a204 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -1459,6 +1459,8 @@ gnttab_transfer( for ( i = 0; i < count; i++ ) { + bool_t okay; + if (i && hypercall_preempt_check()) return i; @@ -1555,16 +1557,18 @@ gnttab_transfer( * pages when it is dying. */ if ( unlikely(e->is_dying) || - unlikely(e->tot_pages >= e->max_pages) || - unlikely(!gnttab_prepare_for_transfer(e, d, gop.ref)) ) + unlikely(e->tot_pages >= e->max_pages) ) { - if ( !e->is_dying ) - gdprintk(XENLOG_INFO, "gnttab_transfer: " - "Transferee has no reservation " - "headroom (%d,%d) or provided a bad grant ref (%08x) " - "or is dying (%d)\n", - e->tot_pages, e->max_pages, gop.ref, e->is_dying); spin_unlock(&e->page_alloc_lock); + + if ( e->is_dying ) + gdprintk(XENLOG_INFO, "gnttab_transfer: " + "Transferee (d%d) is dying\n", e->domain_id); + else + gdprintk(XENLOG_INFO, "gnttab_transfer: " + "Transferee (d%d) has no headroom (tot %u, max %u)\n", + e->domain_id, e->tot_pages, e->max_pages); + rcu_unlock_domain(e); page->count_info &= ~(PGC_count_mask|PGC_allocated); free_domheap_page(page); @@ -1575,6 +1579,37 @@ gnttab_transfer( /* Okay, add the page to 'e'. */ if ( unlikely(e->tot_pages++ == 0) ) get_knownalive_domain(e); + + /* + * We must drop the lock to avoid a possible deadlock in + * gnttab_prepare_for_transfer. We have reserved a page in e so can + * safely drop the lock and re-aquire it later to add page to the + * pagelist. + */ + spin_unlock(&e->page_alloc_lock); + okay = gnttab_prepare_for_transfer(e, d, gop.ref); + spin_lock(&e->page_alloc_lock); + + if ( unlikely(!okay) || unlikely(e->is_dying) ) + { + bool_t drop_dom_ref = !--e->tot_pages; + + spin_unlock(&e->page_alloc_lock); + + if ( okay /* i.e. e->is_dying due to the surrounding if() */ ) + gdprintk(XENLOG_INFO, "gnttab_transfer: " + "Transferee (d%d) is now dying\n", e->domain_id); + + if ( drop_dom_ref ) + put_domain(e); + rcu_unlock_domain(e); + + page->count_info &= ~(PGC_count_mask|PGC_allocated); + free_domheap_page(page); + gop.status = GNTST_general_error; + goto copyback; + } + page_list_add_tail(page, &e->page_list); page_set_owner(page, e); -- 2.39.5