Xen devel - May 2013 - [PATCH v2] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed

XSA-36 changed the default vector map mode from global to per-device.  This is
because a global vector map does not prevent one PCI device from impersonating
another and launching a DoS on the system.

However, the per-device vector map logic is broken for devices with multiple
MSI-X vectors, which can either result in a failed ASSERT() or misprogramming
of a guests interrupt remapping tables.  The core problem is not trivial to
fix.

In an effort to get AMD systems back to a non-regressed state, introduce a new
type of vector map called per-device-global.  This uses per-device vector maps
in the IOMMU, but uses a single used_vector map for the core IRQ logic.

This patch is intended to be removed as soon as the per-device logic is fixed
correctly.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

---
This patch specifically does not introduce a command line argument for
this mode to avoid needing to carry it forever more for compatibility reasons.

Unfortunately, the per-device logic is going to be very complicated to fix.
Under the current irq architecture, by the time you can work out you have a
problem in map_domain_pirq(), it is far too late to fix it in a compatible
way.  It would be possible to "fix" the issue by failing the
hypercall, but
is not acceptable IMO.  One logical way to fix the issue would be to reassign
one of the irqs to a different vector, but that requires waiting for another
interrupt, and trashes the PCI device''s used_vector table.

The best solution I can see is to have create_irq() know about which PCI
device the irq belongs to, but I cant find a nice way of making this
information available.

George: This patch should go into xen-4.3 (as well as being backported) as it
is specifically to work around a regression caused by XSA-36

Changes since v1:
 * Correct stupid mistake in commit message, making it confusing to read

diff -r 84e4d183fa8b -r 6671fc79717a xen/arch/x86/irq.c
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -399,7 +399,8 @@ static vmask_t *irq_get_used_vector_mask
 {
     vmask_t *ret = NULL;
 
-    if ( opt_irq_vector_map == OPT_IRQ_VECTOR_MAP_GLOBAL )
+    if ( opt_irq_vector_map == OPT_IRQ_VECTOR_MAP_GLOBAL ||
+         opt_irq_vector_map == OPT_IRQ_VECTOR_MAP_PERDEV_GLOBAL )
     {
         struct irq_desc *desc = irq_to_desc(irq);
 
diff -r 84e4d183fa8b -r 6671fc79717a xen/drivers/passthrough/amd/pci_amd_iommu.c
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -238,6 +238,21 @@ int __init amd_iov_detect(void)
     }
     if ( !amd_iommu_perdev_intremap )
         printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table
is not recommended (see XSA-36)!\n");
+
+    /* Per-device vector map logic is broken for devices with multiple MSI-X
+     * interrupts (and would also be for multiple MSI, if Xen supported it).
+     *
+     * Until this is fixed, use per-device-global vector tables to avoid the
+     * security vulnerability of global maps, and the buggy behaviour of
+     * per-device maps in map_domain_pirq().
+     */
+    if ( opt_irq_vector_map == OPT_IRQ_VECTOR_MAP_PERDEV )
+    {
+        printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is
broken.  "
+               "Using per-device-global maps instead until a fix is
found\n");
+        opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_PERDEV_GLOBAL;
+    }
+
     return scan_pci_devices();
 }
 
diff -r 84e4d183fa8b -r 6671fc79717a xen/include/asm-x86/irq.h
--- a/xen/include/asm-x86/irq.h
+++ b/xen/include/asm-x86/irq.h
@@ -57,6 +57,7 @@ extern bool_t opt_noirqbalance;
 #define OPT_IRQ_VECTOR_MAP_NONE    1 /* None */ 
 #define OPT_IRQ_VECTOR_MAP_GLOBAL  2 /* One global vector map (no vector
sharing) */
 #define OPT_IRQ_VECTOR_MAP_PERDEV  3 /* Per-device vetor map (no vector sharing
w/in a device) */
+#define OPT_IRQ_VECTOR_MAP_PERDEV_GLOBAL 4 /* Remove me when PERDEV logic is
fixed */
 
 extern int opt_irq_vector_map;

>>> On 31.05.13 at 22:04, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> In an effort to get AMD systems back to a non-regressed state, introduce a 
> new
> type of vector map called per-device-global.  This uses per-device vector
maps
> in the IOMMU, but uses a single used_vector map for the core IRQ logic.
So what''s the reason for not simply using OPT_IRQ_VECTOR_MAP_GLOBAL
here?
> This patch is intended to be removed as soon as the per-device logic is
fixed
> correctly.
As a last resort thing this may be acceptable, but I''d much favor to
fix this properly rather than hacking it like this. Hence I''d really
like
to put up for discussion to instead use the patch[1] already posted
as preparatory for the multi-vector MSI support doing away with the
use of the vector for indexing the IRTE (and, in a second patch[2],
the enforcement of OPT_IRQ_VECTOR_MAP_PERDEV).

Also, overriding a command line request in the way you do is a
no-go imo - even if this would cause [theoretical] problems, we
ought to honor the request as long as we can''t tell for sure that
this is going to break the specific system. That''s even more so
since requesting per-device vector maps to be used on VT-d ought
to yield exactly the same effect, yet you don''t override the mode
there.

Furthermore, if only MSI-X devices currently suffer from this, the
scalability effect this has (allowing nor more than about 200
vectors to be in use even on huge systems) would call for limiting
the effect to MSI-X capable devices (or perhaps even to devices
actually using MSI-X).

Jan

[1] http://lists.xen.org/archives/html/xen-devel/2013-04/msg01886.html
[2] http://lists.xen.org/archives/html/xen-devel/2013-04/msg01887.html

On 03/06/13 15:07, Jan Beulich wrote:
>>>> On 31.05.13 at 22:04, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>> In an effort to get AMD systems back to a non-regressed state,
introduce a
>> new
>> type of vector map called per-device-global.  This uses per-device
vector maps
>> in the IOMMU, but uses a single used_vector map for the core IRQ logic.
> So what''s the reason for not simply using
OPT_IRQ_VECTOR_MAP_GLOBAL
> here?
Simply to make it obviously different until the core problem is fixed,
at which point I expect OPT_IRQ_VECTOR_MAP_PERDEV_GLOBAL to disappear.
>
>> This patch is intended to be removed as soon as the per-device logic is
fixed
>> correctly.
> As a last resort thing this may be acceptable, but I''d much favor
to
> fix this properly rather than hacking it like this.
While I agree that a proper fix would be good, what is going to happen
about 4.2 and 4.1 which wont have this new functionality backported? 
Futhermore, unless this new functionalty is going to race into 4.3 at
the last moment, 4.3 will also be in a regressed state.
>  Hence I''d really like
> to put up for discussion to instead use the patch[1] already posted
> as preparatory for the multi-vector MSI support doing away with the
> use of the vector for indexing the IRTE (and, in a second patch[2],
> the enforcement of OPT_IRQ_VECTOR_MAP_PERDEV).
>
> Also, overriding a command line request in the way you do is a
> no-go imo - even if this would cause [theoretical] problems,
Not theoretical.  I have reproduced the issue, albeit with a modified
Xen which deliberately limits the range of vectors considered for a
certain device, to increase the chances of a collision.
>  we
> ought to honor the request as long as we can''t tell for sure that
> this is going to break the specific system. That''s even more so
> since requesting per-device vector maps to be used on VT-d ought
> to yield exactly the same effect, yet you don''t override the mode
> there.
Anyone using these vector maps with VT-d is mad.  I could tweak the
patch to not override the command line but simply warn when global is
chosen.
>
> Furthermore, if only MSI-X devices currently suffer from this, the
> scalability effect this has (allowing nor more than about 200
> vectors to be in use even on huge systems) would call for limiting
> the effect to MSI-X capable devices (or perhaps even to devices
> actually using MSI-X).
>
> Jan
As I said, this reverts to the behaviour before XSA-36, but without the
security issue of a single IOMMU interrupt remapping table.  Before
XSA-36, all AMD systems were limited in vector range because of the
global used_vector map.

~Andrew
>
> [1] http://lists.xen.org/archives/html/xen-devel/2013-04/msg01886.html
> [2] http://lists.xen.org/archives/html/xen-devel/2013-04/msg01887.html
>

>>> On 03.06.13 at 16:35, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 03/06/13 15:07, Jan Beulich wrote:
>>>>> On 31.05.13 at 22:04, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>>> In an effort to get AMD systems back to a non-regressed state,
introduce a
>>> new
>>> type of vector map called per-device-global.  This uses per-device
vector maps
>>> in the IOMMU, but uses a single used_vector map for the core IRQ
logic.
>> So what''s the reason for not simply using
OPT_IRQ_VECTOR_MAP_GLOBAL
>> here?
> 
> Simply to make it obviously different until the core problem is fixed,
> at which point I expect OPT_IRQ_VECTOR_MAP_PERDEV_GLOBAL to disappear.
That''s not a really good excuse...
>>> This patch is intended to be removed as soon as the per-device
logic is fixed
>>> correctly.
>> As a last resort thing this may be acceptable, but I''d much
favor to
>> fix this properly rather than hacking it like this.
> 
> While I agree that a proper fix would be good, what is going to happen
> about 4.2 and 4.1 which wont have this new functionality backported? 
> Futhermore, unless this new functionalty is going to race into 4.3 at
> the last moment, 4.3 will also be in a regressed state.
The new functionality (multi-vector MSI) doesn''t necessarily need
to be backported, but if the prereq change turns out to fix a bug,
I don''t see a reason not to try to backport that one.

As to getting the patch in for 4.3 - George, would you revisit your
opinion on the part of the multi-vector MSI series that originally
I had hoped to get into 4.3 anyway?
>>  Hence I''d really like
>> to put up for discussion to instead use the patch[1] already posted
>> as preparatory for the multi-vector MSI support doing away with the
>> use of the vector for indexing the IRTE (and, in a second patch[2],
>> the enforcement of OPT_IRQ_VECTOR_MAP_PERDEV).
>>
>> Also, overriding a command line request in the way you do is a
>> no-go imo - even if this would cause [theoretical] problems,
> 
> Not theoretical.  I have reproduced the issue, albeit with a modified
> Xen which deliberately limits the range of vectors considered for a
> certain device, to increase the chances of a collision.
You misunderstood my use of "theoretical": On a system with only
MSI devices, no problem is to be expected afaict. Yet your change
would affect those too.
>>  we
>> ought to honor the request as long as we can''t tell for sure
that
>> this is going to break the specific system. That''s even more
so
>> since requesting per-device vector maps to be used on VT-d ought
>> to yield exactly the same effect, yet you don''t override the
mode
>> there.
> 
> Anyone using these vector maps with VT-d is mad.  I could tweak the
> patch to not override the command line but simply warn when global is
> chosen.
Let''s take a step back: What do we need those vector maps for in
the first place, other than the disambiguation of AMD IOMMU
IRTEs? If the answer is "nothing", then why was a command line
option controlling this added in the first place? And in that case
ripping them out the moment the patches mentioned above go in
would seem like the right thing to do. George, I think you added all
that - do you have any thoughts here?
>> Furthermore, if only MSI-X devices currently suffer from this, the
>> scalability effect this has (allowing nor more than about 200
>> vectors to be in use even on huge systems) would call for limiting
>> the effect to MSI-X capable devices (or perhaps even to devices
>> actually using MSI-X).
> 
> As I said, this reverts to the behaviour before XSA-36, but without the
> security issue of a single IOMMU interrupt remapping table.  Before
> XSA-36, all AMD systems were limited in vector range because of the
> global used_vector map.
Right, so you''d trade one regression for another (less severe, but
anyway).

Jan

On 03/06/13 16:01, Jan Beulich wrote:
>>>> On 03.06.13 at 16:35, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>> On 03/06/13 15:07, Jan Beulich wrote:
>>>>>> On 31.05.13 at 22:04, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>>>> In an effort to get AMD systems back to a non-regressed state,
introduce a
>>>> new
>>>> type of vector map called per-device-global.  This uses
per-device vector maps
>>>> in the IOMMU, but uses a single used_vector map for the core
IRQ logic.
>>> So what''s the reason for not simply using
OPT_IRQ_VECTOR_MAP_GLOBAL
>>> here?
>> Simply to make it obviously different until the core problem is fixed,
>> at which point I expect OPT_IRQ_VECTOR_MAP_PERDEV_GLOBAL to disappear.
> That''s not a really good excuse...
>
>>>> This patch is intended to be removed as soon as the per-device
logic is fixed
>>>> correctly.
>>> As a last resort thing this may be acceptable, but I''d
much favor to
>>> fix this properly rather than hacking it like this.
>> While I agree that a proper fix would be good, what is going to happen
>> about 4.2 and 4.1 which wont have this new functionality backported? 
>> Futhermore, unless this new functionalty is going to race into 4.3 at
>> the last moment, 4.3 will also be in a regressed state.
> The new functionality (multi-vector MSI) doesn''t necessarily need
> to be backported, but if the prereq change turns out to fix a bug,
> I don''t see a reason not to try to backport that one.
>
> As to getting the patch in for 4.3 - George, would you revisit your
> opinion on the part of the multi-vector MSI series that originally
> I had hoped to get into 4.3 anyway?
>
>>>  Hence I''d really like
>>> to put up for discussion to instead use the patch[1] already posted
>>> as preparatory for the multi-vector MSI support doing away with the
>>> use of the vector for indexing the IRTE (and, in a second patch[2],
>>> the enforcement of OPT_IRQ_VECTOR_MAP_PERDEV).
>>>
>>> Also, overriding a command line request in the way you do is a
>>> no-go imo - even if this would cause [theoretical] problems,
>> Not theoretical.  I have reproduced the issue, albeit with a modified
>> Xen which deliberately limits the range of vectors considered for a
>> certain device, to increase the chances of a collision.
> You misunderstood my use of "theoretical": On a system with only
> MSI devices, no problem is to be expected afaict. Yet your change
> would affect those too.
Ah I see.
>
>>>  we
>>> ought to honor the request as long as we can''t tell for
sure that
>>> this is going to break the specific system. That''s even
more so
>>> since requesting per-device vector maps to be used on VT-d ought
>>> to yield exactly the same effect, yet you don''t override
the mode
>>> there.
>> Anyone using these vector maps with VT-d is mad.  I could tweak the
>> patch to not override the command line but simply warn when global is
>> chosen.
> Let''s take a step back: What do we need those vector maps for in
> the first place, other than the disambiguation of AMD IOMMU
> IRTEs? If the answer is "nothing", then why was a command line
> option controlling this added in the first place? And in that case
> ripping them out the moment the patches mentioned above go in
> would seem like the right thing to do. George, I think you added all
> that - do you have any thoughts here?
As I remember, the original bug was that when migrating an interrupts in
Xen from one pcpu to another and choosing the same vector, the cleanup
code zapped the IRTE, causing loss of interrupts.  The used_vector logic
was added to prevent the interrupt migration code from choosing the same
vector on a different pcpu.

I cant precisely comment about the introduction of the command line
option.  With hindsight, I suspect it might have been a lack of
understanding the extent of the problem.  I was certainly quite new to
interrupt remapping at the time and did feel a little out of my depth.
>
>>> Furthermore, if only MSI-X devices currently suffer from this, the
>>> scalability effect this has (allowing nor more than about 200
>>> vectors to be in use even on huge systems) would call for limiting
>>> the effect to MSI-X capable devices (or perhaps even to devices
>>> actually using MSI-X).
>> As I said, this reverts to the behaviour before XSA-36, but without the
>> security issue of a single IOMMU interrupt remapping table.  Before
>> XSA-36, all AMD systems were limited in vector range because of the
>> global used_vector map.
> Right, so you''d trade one regression for another (less severe, but
> anyway).
>
> Jan
>
Absolutely, especially when it comes to trying to fix a regression we
have pushed out in a security fix.

Ideally a proper fix to MSI-X issue can be found, but failing a timely
fix, reverting to the pre XSA-36 behaviour but without the security
issue is a good solution.

~Andrew

>>> On 03.06.13 at 17:17, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 03/06/13 16:01, Jan Beulich wrote:
>>>>> On 03.06.13 at 16:35, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>>> As I said, this reverts to the behaviour before XSA-36, but without
the
>>> security issue of a single IOMMU interrupt remapping table.  Before
>>> XSA-36, all AMD systems were limited in vector range because of the
>>> global used_vector map.
>> Right, so you''d trade one regression for another (less severe,
but
>> anyway).
> 
> Absolutely, especially when it comes to trying to fix a regression we
> have pushed out in a security fix.
> 
> Ideally a proper fix to MSI-X issue can be found, but failing a timely
> fix, reverting to the pre XSA-36 behaviour but without the security
> issue is a good solution.
Just to repeat - "can be found" is the wrong term, as we already
have a patch pending that - from all I can tell - would take care of
the problem (and you not stating anything to the contrary makes
me assume you agree).

Jan

On 03/06/13 16:28, Jan Beulich wrote:
>>>> On 03.06.13 at 17:17, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>> On 03/06/13 16:01, Jan Beulich wrote:
>>>>>> On 03.06.13 at 16:35, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>>>> As I said, this reverts to the behaviour before XSA-36, but
without the
>>>> security issue of a single IOMMU interrupt remapping table. 
Before
>>>> XSA-36, all AMD systems were limited in vector range because of
the
>>>> global used_vector map.
>>> Right, so you''d trade one regression for another (less
severe, but
>>> anyway).
>> Absolutely, especially when it comes to trying to fix a regression we
>> have pushed out in a security fix.
>>
>> Ideally a proper fix to MSI-X issue can be found, but failing a timely
>> fix, reverting to the pre XSA-36 behaviour but without the security
>> issue is a good solution.
> Just to repeat - "can be found" is the wrong term, as we already
> have a patch pending that - from all I can tell - would take care of
> the problem (and you not stating anything to the contrary makes
> me assume you agree).
>
> Jan
I would agree that it looks to fix the underlying issue.

It is however quite a large change to a subtle part of the code, which
makes me hesitant about declaring it a good backport candidate for
previous versions.

~Andrew
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On 06/03/2013 03:07 PM, Jan Beulich wrote:
>>>> On 31.05.13 at 22:04, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>> In an effort to get AMD systems back to a non-regressed state,
introduce a
>> new
>> type of vector map called per-device-global.  This uses per-device
vector maps
>> in the IOMMU, but uses a single used_vector map for the core IRQ logic.
>
> So what''s the reason for not simply using
OPT_IRQ_VECTOR_MAP_GLOBAL
> here?
>
>> This patch is intended to be removed as soon as the per-device logic is
fixed
>> correctly.
>
> As a last resort thing this may be acceptable, but I''d much favor
to
> fix this properly rather than hacking it like this. Hence I''d
really like
> to put up for discussion to instead use the patch[1] already posted
> as preparatory for the multi-vector MSI support doing away with the
> use of the vector for indexing the IRTE (and, in a second patch[2],
> the enforcement of OPT_IRQ_VECTOR_MAP_PERDEV).
Unfortunately this is the time of the release to do simple hacks.  We 
can obviously back this out when we get hte multi-vector MSI suport in.

> Also, overriding a command line request in the way you do is a
> no-go imo - even if this would cause [theoretical] problems, we
> ought to honor the request as long as we can''t tell for sure that
> this is going to break the specific system. That''s even more so
> since requesting per-device vector maps to be used on VT-d ought
> to yield exactly the same effect, yet you don''t override the mode
> there.
I agree -- we need to have a sensible default, but we also need to have 
a way to override behavior if it turns out to cause a problem.

  -George