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
|
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/MSI-X: restrict reading of table/PBA bases from BARs
When assigned to less trusted or un-trusted guests, devices may change
state behind our backs (they may e.g. get reset by means we may not know
about). Therefore we should avoid reading BARs from hardware once a
device is no longer owned by Dom0. Furthermore when we can't read a BAR,
or when we read zero, we shouldn't instead use the caller provided
address unless that caller can be trusted.
Re-arrange the logic in msix_capability_init() such that only Dom0 (and
only if the device isn't DomU-owned yet) or calls through
PHYSDEVOP_prepare_msix will actually result in the reading of the
respective BAR register(s). Additionally do so only as long as in-use
table entries are known (note that invocation of PHYSDEVOP_prepare_msix
counts as a "pseudo" entry). In all other uses the value already
recorded will get used instead.
Clear the recorded values in _pci_cleanup_msix() as well as on the one
affected error path. (Adjust this error path to also avoid blindly
disabling MSI-X when it was enabled on entry to the function.)
While moving around variable declarations (in many cases to reduce their
scopes), also adjust some of their types.
This is part of XSA-337.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -769,16 +769,14 @@ static int msix_capability_init(struct p
{
struct arch_msix *msix = dev->msix;
struct msi_desc *entry = NULL;
- int vf;
u16 control;
u64 table_paddr;
u32 table_offset;
- u8 bir, pbus, pslot, pfunc;
u16 seg = dev->seg;
u8 bus = dev->bus;
u8 slot = PCI_SLOT(dev->devfn);
u8 func = PCI_FUNC(dev->devfn);
- bool maskall = msix->host_maskall;
+ bool maskall = msix->host_maskall, zap_on_error = false;
unsigned int pos = pci_find_cap_offset(seg, bus, slot, func,
PCI_CAP_ID_MSIX);
@@ -820,43 +818,45 @@ static int msix_capability_init(struct p
/* Locate MSI-X table region */
table_offset = pci_conf_read32(dev->sbdf, msix_table_offset_reg(pos));
- bir = (u8)(table_offset & PCI_MSIX_BIRMASK);
- table_offset &= ~PCI_MSIX_BIRMASK;
+ if ( !msix->used_entries &&
+ (!msi ||
+ (is_hardware_domain(current->domain) &&
+ (dev->domain == current->domain || dev->domain == dom_io))) )
+ {
+ unsigned int bir = table_offset & PCI_MSIX_BIRMASK, pbus, pslot, pfunc;
+ int vf;
+ paddr_t pba_paddr;
+ unsigned int pba_offset;
- if ( !dev->info.is_virtfn )
- {
- pbus = bus;
- pslot = slot;
- pfunc = func;
- vf = -1;
- }
- else
- {
- pbus = dev->info.physfn.bus;
- pslot = PCI_SLOT(dev->info.physfn.devfn);
- pfunc = PCI_FUNC(dev->info.physfn.devfn);
- vf = PCI_BDF2(dev->bus, dev->devfn);
- }
-
- table_paddr = read_pci_mem_bar(seg, pbus, pslot, pfunc, bir, vf);
- WARN_ON(msi && msi->table_base != table_paddr);
- if ( !table_paddr )
- {
- if ( !msi || !msi->table_base )
+ if ( !dev->info.is_virtfn )
{
- pci_conf_write16(dev->sbdf, msix_control_reg(pos),
- control & ~PCI_MSIX_FLAGS_ENABLE);
- xfree(entry);
- return -ENXIO;
+ pbus = bus;
+ pslot = slot;
+ pfunc = func;
+ vf = -1;
+ }
+ else
+ {
+ pbus = dev->info.physfn.bus;
+ pslot = PCI_SLOT(dev->info.physfn.devfn);
+ pfunc = PCI_FUNC(dev->info.physfn.devfn);
+ vf = PCI_BDF2(dev->bus, dev->devfn);
}
- table_paddr = msi->table_base;
- }
- table_paddr += table_offset;
- if ( !msix->used_entries )
- {
- u64 pba_paddr;
- u32 pba_offset;
+ table_paddr = read_pci_mem_bar(seg, pbus, pslot, pfunc, bir, vf);
+ WARN_ON(msi && msi->table_base != table_paddr);
+ if ( !table_paddr )
+ {
+ if ( !msi || !msi->table_base )
+ {
+ pci_conf_write16(dev->sbdf, msix_control_reg(pos),
+ control & ~PCI_MSIX_FLAGS_ENABLE);
+ xfree(entry);
+ return -ENXIO;
+ }
+ table_paddr = msi->table_base;
+ }
+ table_paddr += table_offset & ~PCI_MSIX_BIRMASK;
msix->table.first = PFN_DOWN(table_paddr);
msix->table.last = PFN_DOWN(table_paddr +
@@ -875,7 +875,18 @@ static int msix_capability_init(struct p
BITS_TO_LONGS(msix->nr_entries) - 1);
WARN_ON(rangeset_overlaps_range(mmio_ro_ranges, msix->pba.first,
msix->pba.last));
+
+ zap_on_error = true;
+ }
+ else if ( !msix->table.first )
+ {
+ pci_conf_write16(dev->sbdf, msix_control_reg(pos), control);
+ xfree(entry);
+ return -ENODATA;
}
+ else
+ table_paddr = (msix->table.first << PAGE_SHIFT) +
+ (table_offset & ~PCI_MSIX_BIRMASK & ~PAGE_MASK);
if ( entry )
{
@@ -886,8 +897,15 @@ static int msix_capability_init(struct p
if ( idx < 0 )
{
- pci_conf_write16(dev->sbdf, msix_control_reg(pos),
- control & ~PCI_MSIX_FLAGS_ENABLE);
+ if ( zap_on_error )
+ {
+ msix->table.first = 0;
+ msix->pba.first = 0;
+
+ control &= ~PCI_MSIX_FLAGS_ENABLE;
+ }
+
+ pci_conf_write16(dev->sbdf, msix_control_reg(pos), control);
xfree(entry);
return idx;
}
@@ -1076,9 +1094,14 @@ static void _pci_cleanup_msix(struct arc
if ( rangeset_remove_range(mmio_ro_ranges, msix->table.first,
msix->table.last) )
WARN();
+ msix->table.first = 0;
+ msix->table.last = 0;
+
if ( rangeset_remove_range(mmio_ro_ranges, msix->pba.first,
msix->pba.last) )
WARN();
+ msix->pba.first = 0;
+ msix->pba.last = 0;
}
}
|
