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
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
|
From 4e718be6f59526927d5cd31ecd80c5c758dca3f5 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Fri, 2 Jun 2017 15:21:27 +0100
Subject: [PATCH 3/4] gnttab: correct logic to get page references during map
requests
The rules for reference counting are somewhat complicated:
* Each of GNTTAB_host_map and GNTTAB_device_map need their own
reference count
* If the mapping is writeable:
- GNTTAB_host_map needs a type count under only some conditions
- GNTTAB_device_map always needs a type count
If the mapping succeeds, we need to keep all of these; if the mapping
fails, we need to release whatever references we have acquired so far.
Additionally, the code that does a lot of this calculation "inherits"
a reference as part of the process of finding out who the owner is.
Finally, if the grant is mapped as writeable (without the
GNTMAP_readonly flag), but the hypervisor cannot grab a
PGT_writeable_page type, the entire operation should fail.
Unfortunately, the current code has several logic holes:
* If a grant is mapped only GNTTAB_device_map, and with a writeable
mapping, but in conditions where a *host* type count is not
necessary, the code will fail to grab the necessary type count.
* If a grant is mapped both GNTTAB_device_map and GNTTAB_host_map,
with a writeable mapping, in conditions where the host type count is
not necessary, *and* where the page cannot be changed to type
PGT_writeable, the condition will not be detected.
In both cases, this means that on success, the type count will be
erroneously reduced when the grant is unmapped. In the second case,
the type count will be erroneously reduced on the failure path as
well. (In the first case the failure path logic has the same hole
as the reference grabbing logic.)
Additionally, the return value of get_page() is not checked; but this
may fail even if the first get_page() succeeded due to a reference
counting overflow.
First of all, simplify the restoration logic by explicitly counting
the reference and type references acquired.
Consider each mapping type separately, explicitly marking the
'incoming' reference as used so we know when we need to grab a second
one.
Finally, always check the return value of get_page[_type]() and go to
the failure path if appropriate.
This is part of XSA-224.
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
xen/common/grant_table.c | 58 +++++++++++++++++++++++++++---------------------
1 file changed, 33 insertions(+), 25 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 5baae24..d07b931 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -754,12 +754,12 @@ __gnttab_map_grant_ref(
struct grant_table *lgt, *rgt;
struct vcpu *led;
int handle;
- unsigned long frame = 0, nr_gets = 0;
+ unsigned long frame = 0;
struct page_info *pg = NULL;
int rc = GNTST_okay;
u32 old_pin;
u32 act_pin;
- unsigned int cache_flags;
+ unsigned int cache_flags, refcnt = 0, typecnt = 0;
struct active_grant_entry *act = NULL;
struct grant_mapping *mt;
grant_entry_header_t *shah;
@@ -885,11 +885,17 @@ __gnttab_map_grant_ref(
else
owner = page_get_owner(pg);
+ if ( owner )
+ refcnt++;
+
if ( !pg || (owner == dom_io) )
{
/* Only needed the reference to confirm dom_io ownership. */
if ( pg )
+ {
put_page(pg);
+ refcnt--;
+ }
if ( paging_mode_external(ld) )
{
@@ -917,27 +923,38 @@ __gnttab_map_grant_ref(
}
else if ( owner == rd || owner == dom_cow )
{
- if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
+ if ( (op->flags & GNTMAP_device_map) && !(op->flags & GNTMAP_readonly) )
{
if ( (owner == dom_cow) ||
!get_page_type(pg, PGT_writable_page) )
goto could_not_pin;
+ typecnt++;
}
- nr_gets++;
if ( op->flags & GNTMAP_host_map )
{
- rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0);
- if ( rc != GNTST_okay )
- goto undo_out;
-
+ /*
+ * Only need to grab another reference if device_map claimed
+ * the other one.
+ */
if ( op->flags & GNTMAP_device_map )
{
- nr_gets++;
- (void)get_page(pg, rd);
- if ( !(op->flags & GNTMAP_readonly) )
- get_page_type(pg, PGT_writable_page);
+ if ( !get_page(pg, rd) )
+ goto could_not_pin;
+ refcnt++;
+ }
+
+ if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
+ {
+ if ( (owner == dom_cow) ||
+ !get_page_type(pg, PGT_writable_page) )
+ goto could_not_pin;
+ typecnt++;
}
+
+ rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0);
+ if ( rc != GNTST_okay )
+ goto undo_out;
}
}
else
@@ -946,8 +963,6 @@ __gnttab_map_grant_ref(
if ( !rd->is_dying )
gdprintk(XENLOG_WARNING, "Could not pin grant frame %lx\n",
frame);
- if ( owner != NULL )
- put_page(pg);
rc = GNTST_general_error;
goto undo_out;
}
@@ -1010,18 +1025,11 @@ __gnttab_map_grant_ref(
return;
undo_out:
- if ( nr_gets > 1 )
- {
- if ( !(op->flags & GNTMAP_readonly) )
- put_page_type(pg);
- put_page(pg);
- }
- if ( nr_gets > 0 )
- {
- if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
- put_page_type(pg);
+ while ( typecnt-- )
+ put_page_type(pg);
+
+ while ( refcnt-- )
put_page(pg);
- }
grant_read_lock(rgt);
--
2.1.4
|
