1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
|
From 068bfa76bbd52430e65853375e1d5db99d193e2f Mon Sep 17 00:00:00 2001
From: Andrew Cooper <andrew.cooper3@citrix.com>
Date: Thu, 31 Oct 2013 20:49:00 +0000
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 <andrew.cooper3@citrix.com>
Consolidate error handling.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Keir Fraser <keir@xen.org>
Tested-by: Matthew Daley <mattjd@gmail.com>
---
xen/common/grant_table.c | 52 +++++++++++++++++++++++++++++++++++++++-------
1 file changed, 44 insertions(+), 8 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index f42bc7a..48df928 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1518,6 +1518,8 @@ gnttab_transfer(
for ( i = 0; i < count; i++ )
{
+ bool_t okay;
+
if (i && hypercall_preempt_check())
return i;
@@ -1626,16 +1628,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);
put_gfn(d, gop.mfn);
page->count_info &= ~(PGC_count_mask|PGC_allocated);
@@ -1647,6 +1651,38 @@ gnttab_transfer(
/* Okay, add the page to 'e'. */
if ( unlikely(domain_adjust_tot_pages(e, 1) == 1) )
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 = (domain_adjust_tot_pages(e, -1) == 0);
+
+ 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);
+
+ put_gfn(d, gop.mfn);
+ 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);
--
1.7.10.4
|
