[PATCH] x86, x2apic: Only WARN on broken BIOSes inside a virtual guest

[PATCH] x86, x2apic: Only WARN on broken BIOSes inside a virtual guest

[Don Zickus]

In commit "41750d3 x86, x2apic: Enable the bios request for x2apic optout"
it was explained how OEMs are trying to opt out of using x2apics.

That commit moved code around and screamed with a WARN if the BIOS
opted out of x2apic mode.  Fair enough.

This code hit our RHEL distro and OEMs started complaining that the
WARN is scaring their customers and asked we tone it down to a
pr_warn().

Before we did that, we thought we should change it upstream too.
Upstream complained that WARN was necessary due to a serious
security threat, namely irq injections.  Hard to argue that.

This left us between a rock and a hard place.  We toned down the
WARN in RHEL to keep our customers happy.  But this leaves us with
a perpetual patch in RHEL and possibly covering up a security threat.

I poked around to understand the nature of the security threat and why
OEMs would want to leave themselves vulnerable.  The only security
threat I could find was this whitepaper talking about Xen and irq
injections:

http://www.invisiblethingslab.com/resources/2011/Software%20Attacks%20on%20Intel%20VT-d.pdf

After talking with folks, the threat of irq injections on virtual guests
made sense.  However, when discussing if this was possible on bare metal
machines, we could not come up with a plausible scenario.

Talking with OEMs, it seems like opting out of x2apic only happens on
bare metal as virtual guests seem to have their own bioses.

Unless irq injections can happen on bare metal too, I am proposing a
tweak in the WARN code.  I am breaking the printk into two pieces.

The first is the loud BIOS disabled x2apic message, but at a printk level
instead of a WARN.  Our OEMs are ok with that.

The second is the security threat of irq injections.  The code now
only WARNs when it detects itself on a virtual machine using x86_hyper.

The hypervisor is configured through setup_arch(), which is run before
the irqs are configured through smp_prepare_boot_cpu(), so this variable
should always be defined correctly.

I tested this on a Dell machine that has x2apic disabled and on an
Intel box that had x2apic enabled.  Everything looked as expected. I
couldn't figure out how to test a virtual guest setup to verify the WARN
works as expected.

Cc: Suresh Siddha <suresh.b.siddha@intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 drivers/iommu/intel_irq_remapping.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
index af8904d..f639906 100644
--- a/drivers/iommu/intel_irq_remapping.c
+++ b/drivers/iommu/intel_irq_remapping.c
@@ -14,6 +14,7 @@
 #include <asm/irq_remapping.h>
 #include <asm/pci-direct.h>
 #include <asm/msidef.h>
+#include <asm/hypervisor.h>
 
 #include "irq_remapping.h"
 
@@ -536,10 +537,13 @@ static int __init intel_enable_irq_remapping(void)
 
 	if (x2apic_supported()) {
 		eim = !dmar_x2apic_optout();
-		WARN(!eim, KERN_WARNING
+		if (!eim) {
+			printk(KERN_WARNING
 			   "Your BIOS is broken and requested that x2apic be disabled\n"
-			   "This will leave your machine vulnerable to irq-injection attacks\n"
 			   "Use 'intremap=no_x2apic_optout' to override BIOS request\n");
+			WARN(x86_hyper,
+			   "This will leave your machine vulnerable to irq-injection attacks\n");
+		}
 	}
 
 	for_each_drhd_unit(drhd) {
-- 
1.7.1

Re: [PATCH] x86, x2apic: Only WARN on broken BIOSes inside a virtual guest

[Gleb Natapov]

On Thu, Jan 31, 2013 at 11:40:35AM -0500, Don Zickus wrote:
> In commit "41750d3 x86, x2apic: Enable the bios request for x2apic optout"
> it was explained how OEMs are trying to opt out of using x2apics.
> 
> That commit moved code around and screamed with a WARN if the BIOS
> opted out of x2apic mode.  Fair enough.
> 
> This code hit our RHEL distro and OEMs started complaining that the
> WARN is scaring their customers and asked we tone it down to a
> pr_warn().
> 
> Before we did that, we thought we should change it upstream too.
> Upstream complained that WARN was necessary due to a serious
> security threat, namely irq injections.  Hard to argue that.
> 
> This left us between a rock and a hard place.  We toned down the
> WARN in RHEL to keep our customers happy.  But this leaves us with
> a perpetual patch in RHEL and possibly covering up a security threat.
> 
> I poked around to understand the nature of the security threat and why
> OEMs would want to leave themselves vulnerable.  The only security
> threat I could find was this whitepaper talking about Xen and irq
> injections:
> 
> http://www.invisiblethingslab.com/resources/2011/Software%20Attacks%20on%20Intel%20VT-d.pdf
> 
> After talking with folks, the threat of irq injections on virtual guests
> made sense.  However, when discussing if this was possible on bare metal
> machines, we could not come up with a plausible scenario.
> 
The irq injections is something that a guest with assigned device does
to attack a hypervisor it runs on. Interrupt remapping protects host
from this attack. According to pdf above if x2apic is disabled in a
hypervisor interrupt remapping can be bypassed and leave host vulnerable
to guest attack. This means that situation is exactly opposite: warning
has sense on a bare metal, but not in a guest. I am not sure that there is
a hypervisor that emulates interrupt remapping device though and without
it the warning cannot be triggered in a guest.

--
			Gleb.

Re: [PATCH] x86, x2apic: Only WARN on broken BIOSes inside a virtual guest

[Don Zickus]

On Thu, Jan 31, 2013 at 08:52:00PM +0200, Gleb Natapov wrote:
> > http://www.invisiblethingslab.com/resources/2011/Software%20Attacks%20on%20Intel%20VT-d.pdf
> > 
> > After talking with folks, the threat of irq injections on virtual guests
> > made sense.  However, when discussing if this was possible on bare metal
> > machines, we could not come up with a plausible scenario.
> > 
> The irq injections is something that a guest with assigned device does
> to attack a hypervisor it runs on. Interrupt remapping protects host
> from this attack. According to pdf above if x2apic is disabled in a
> hypervisor interrupt remapping can be bypassed and leave host vulnerable
> to guest attack. This means that situation is exactly opposite: warning
> has sense on a bare metal, but not in a guest. I am not sure that there is
> a hypervisor that emulates interrupt remapping device though and without
> it the warning cannot be triggered in a guest.

Ah, it makes sense.  Not sure how I got it backwards then.  So my patch is
pointless then?  I'll asked for it to be dropped.

>From my previous discussions with folks, is that KVM was protected from
this type of attack.  Is that still true?

Cheers,
Don

Re: [PATCH] x86, x2apic: Only WARN on broken BIOSes inside a virtual guest

[Gleb Natapov]

On Thu, Jan 31, 2013 at 02:34:27PM -0500, Don Zickus wrote:
> On Thu, Jan 31, 2013 at 08:52:00PM +0200, Gleb Natapov wrote:
> > > http://www.invisiblethingslab.com/resources/2011/Software%20Attacks%20on%20Intel%20VT-d.pdf
> > > 
> > > After talking with folks, the threat of irq injections on virtual guests
> > > made sense.  However, when discussing if this was possible on bare metal
> > > machines, we could not come up with a plausible scenario.
> > > 
> > The irq injections is something that a guest with assigned device does
> > to attack a hypervisor it runs on. Interrupt remapping protects host
> > from this attack. According to pdf above if x2apic is disabled in a
> > hypervisor interrupt remapping can be bypassed and leave host vulnerable
> > to guest attack. This means that situation is exactly opposite: warning
> > has sense on a bare metal, but not in a guest. I am not sure that there is
> > a hypervisor that emulates interrupt remapping device though and without
> > it the warning cannot be triggered in a guest.
> 
> Ah, it makes sense.  Not sure how I got it backwards then.  So my patch is
> pointless then?  I'll asked for it to be dropped.
Yes, it is backwards.

> 
> >From my previous discussions with folks, is that KVM was protected from
> this type of attack.  Is that still true?
> 
Copying Alex. He said that to use device assignment without interrupt
remapping customer needs to opt-in explicitly. Not sure what happens
with interrupt remapping but with x2apic disabled.

The problem is not limited to virtualization BTW. Any vfio user may
attack kernel without interrupt remapping so vfio has the same opt-in.
 
--
			Gleb.

Re: [PATCH] x86, x2apic: Only WARN on broken BIOSes inside a virtual guest

[Alex Williamson]

On Thu, 2013-01-31 at 22:00 +0200, Gleb Natapov wrote:
> On Thu, Jan 31, 2013 at 02:34:27PM -0500, Don Zickus wrote:
> > On Thu, Jan 31, 2013 at 08:52:00PM +0200, Gleb Natapov wrote:
> > > > http://www.invisiblethingslab.com/resources/2011/Software%20Attacks%20on%20Intel%20VT-d.pdf
> > > > 
> > > > After talking with folks, the threat of irq injections on virtual guests
> > > > made sense.  However, when discussing if this was possible on bare metal
> > > > machines, we could not come up with a plausible scenario.
> > > > 
> > > The irq injections is something that a guest with assigned device does
> > > to attack a hypervisor it runs on. Interrupt remapping protects host
> > > from this attack. According to pdf above if x2apic is disabled in a
> > > hypervisor interrupt remapping can be bypassed and leave host vulnerable
> > > to guest attack. This means that situation is exactly opposite: warning
> > > has sense on a bare metal, but not in a guest. I am not sure that there is
> > > a hypervisor that emulates interrupt remapping device though and without
> > > it the warning cannot be triggered in a guest.
> > 
> > Ah, it makes sense.  Not sure how I got it backwards then.  So my patch is
> > pointless then?  I'll asked for it to be dropped.
> Yes, it is backwards.
> 
> > 
> > >From my previous discussions with folks, is that KVM was protected from
> > this type of attack.  Is that still true?
> > 
> Copying Alex. He said that to use device assignment without interrupt
> remapping customer needs to opt-in explicitly. Not sure what happens
> with interrupt remapping but with x2apic disabled.

Per the paper above, compatibility format is only vulnerable if EIM
(Extended Interrupt Mode) is clear (x2APIC not enabled) and CFIS in the
global command register is set.  The latter is never set.

> The problem is not limited to virtualization BTW. Any vfio user may
> attack kernel without interrupt remapping so vfio has the same opt-in.

Yep.  Thanks,

Alex

Re: [PATCH] x86, x2apic: Only WARN on broken BIOSes inside a virtual guest

[Andy Lutomirski]

On 01/31/2013 12:52 PM, Alex Williamson wrote:
> On Thu, 2013-01-31 at 22:00 +0200, Gleb Natapov wrote:
>> On Thu, Jan 31, 2013 at 02:34:27PM -0500, Don Zickus wrote:
>>> On Thu, Jan 31, 2013 at 08:52:00PM +0200, Gleb Natapov wrote:
>>>>> http://www.invisiblethingslab.com/resources/2011/Software%20Attacks%20on%20Intel%20VT-d.pdf
>>>>>
>>>>> After talking with folks, the threat of irq injections on virtual guests
>>>>> made sense.  However, when discussing if this was possible on bare metal
>>>>> machines, we could not come up with a plausible scenario.
>>>>>
>>>> The irq injections is something that a guest with assigned device does
>>>> to attack a hypervisor it runs on. Interrupt remapping protects host
>>>> from this attack. According to pdf above if x2apic is disabled in a
>>>> hypervisor interrupt remapping can be bypassed and leave host vulnerable
>>>> to guest attack. This means that situation is exactly opposite: warning
>>>> has sense on a bare metal, but not in a guest. I am not sure that there is
>>>> a hypervisor that emulates interrupt remapping device though and without
>>>> it the warning cannot be triggered in a guest.
>>>
>>> Ah, it makes sense.  Not sure how I got it backwards then.  So my patch is
>>> pointless then?  I'll asked for it to be dropped.
>> Yes, it is backwards.
>>
>>>
>>> >From my previous discussions with folks, is that KVM was protected from
>>> this type of attack.  Is that still true?
>>>
>> Copying Alex. He said that to use device assignment without interrupt
>> remapping customer needs to opt-in explicitly. Not sure what happens
>> with interrupt remapping but with x2apic disabled.
> 
> Per the paper above, compatibility format is only vulnerable if EIM
> (Extended Interrupt Mode) is clear (x2APIC not enabled) and CFIS in the
> global command register is set.  The latter is never set.

The existing code is either confusing or entirely wrong.  Here's what
my Dell server says:

[    0.192168] ------------[ cut here ]------------
[    0.197320] WARNING: at drivers/iommu/intel_irq_remapping.c:542
intel_enable_
irq_remapping+0x7b/0x27e()
[    0.207799] Hardware name: PowerEdge R620
[    0.212268] Your BIOS is broken and requested that x2apic be disabled
[    0.212268] This will leave your machine vulnerable to irq-injection
attacks
[    0.212268] Use 'intremap=no_x2apic_optout' to override BIOS request
[    0.234378] Modules linked in:
[    0.237792] Pid: 1, comm: swapper/0 Not tainted 3.5.7-ama+ #6
[    0.244199] Call Trace:
[    0.246924]  [<ffffffff81afb7e6>] ? intel_enable_irq_remapping+0x7b/0x27e
[    0.254497]  [<ffffffff8104529f>] warn_slowpath_common+0x7f/0xc0
[    0.261196]  [<ffffffff81045396>] warn_slowpath_fmt+0x46/0x50
[    0.267604]  [<ffffffff81afb7e6>] intel_enable_irq_remapping+0x7b/0x27e
[    0.274982]  [<ffffffff81afbb23>] irq_remapping_enable+0x20/0x22
[    0.281682]  [<ffffffff81ad4581>] enable_IR+0x39/0x41
[    0.287315]  [<ffffffff81ad48a4>] enable_IR_x2apic+0x88/0x1cc
[    0.293725]  [<ffffffff814f9685>] ? set_cpu_sibling_map+0x416/0x433
[    0.300715]  [<ffffffff81ad66d3>] default_setup_apic_routing+0x12/0x78
[    0.307997]  [<ffffffff81ad26bd>] native_smp_prepare_cpus+0x430/0x476
[    0.315183]  [<ffffffff81ac7bf2>] kernel_init+0x8d/0x1c0
[    0.321106]  [<ffffffff81513cd4>] kernel_thread_helper+0x4/0x10
[    0.327710]  [<ffffffff81ac7b65>] ? start_kernel+0x346/0x346
[    0.334020]  [<ffffffff81513cd0>] ? gs_change+0xb/0xb
[    0.339654] ---[ end trace 6a8759eb4c55eb5c ]---
[    0.345193] Enabled IRQ remapping in xapic mode
[    0.350241] x2apic not enabled, IRQ remapping is in xapic mode

(This is Linux 3.5, but I don't think anything has changed.)

Note:

1. I've been warned that x2apic is off, so I'm vulnerable.
2. IRQ remapping *is* enabled.

So disabling the x2apic opt-out is a red herring, *except* that,
according to the VT-d spec (Section 5.3.2.1, page 42):

If Extended Interrupt Mode is enabled (EIME field in Interrupt
Remapping Table Address register is Set), or if the Compatibility format
interrupts are disabled (CFIS field in the Global Status register is
Clear), the Compatibility format interrupts are blocked.


EIME appears to be set in x2apic mode but not in xapic mode.  I'll send
a patch to clean up the warnings and explicitly clear CFI once I
test-boot it.

--Andy

[PATCH] intel_irq_remapping: Clean up x2apic optout security warning mess

[Andy Lutomirski]

Current kernels print this on my Dell server:

   ------------[ cut here ]------------
   WARNING: at drivers/iommu/intel_irq_remapping.c:542
   intel_enable_irq_remapping+0x7b/0x27e()
   Hardware name: PowerEdge R620
   Your BIOS is broken and requested that x2apic be disabled
   This will leave your machine vulnerable to irq-injection attacks
   Use 'intremap=no_x2apic_optout' to override BIOS request
   [...]
   Enabled IRQ remapping in xapic mode
   x2apic not enabled, IRQ remapping is in xapic mode

This is inconsistent with itself -- interrupt remapping is *on*.

Fix the mess by making the warnings say what they mean and my making
sure that compatibility format interrupts (the dangerous ones) are
disabled if x2apic is present regardless of BIOS settings.

With this patch applied, the output is:

  Your BIOS is broken and requested that x2apic be disabled.
  This will slightly decrease performance.
  Use 'intremap=no_x2apic_optout' to override BIOS request.
  Enabled IRQ remapping in xapic mode
  x2apic not enabled, IRQ remapping is in xapic mode

This should make us as or more secure than we are now and replace
a rather scary warning with a much less scary warning on silly
but functional systems.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 drivers/iommu/intel_irq_remapping.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
index af8904d..eca8832 100644
--- a/drivers/iommu/intel_irq_remapping.c
+++ b/drivers/iommu/intel_irq_remapping.c
@@ -425,11 +425,22 @@ static void iommu_set_irq_remapping(struct intel_iommu *iommu, int mode)
 
 	/* Enable interrupt-remapping */
 	iommu->gcmd |= DMA_GCMD_IRE;
+	iommu->gcmd &= ~DMA_GCMD_CFI;  /* Block compatibility-format MSIs */
 	writel(iommu->gcmd, iommu->reg + DMAR_GCMD_REG);
 
 	IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG,
 		      readl, (sts & DMA_GSTS_IRES), sts);
 
+	/*
+	 * With CFI clear in the Global Command register, we should be
+	 * protected from dangerous (i.e. compatibility) interrupts
+	 * regardless of x2apic status.  Check just to be sure.
+	 */
+	if (sts & DMA_GSTS_CFIS)
+		WARN(1, KERN_WARNING
+			"Compatibility-format IRQs enabled despite intr remapping;\n"
+			"you are vulnerable to IRQ injection.\n");
+
 	raw_spin_unlock_irqrestore(&iommu->register_lock, flags);
 }
 
@@ -526,20 +537,24 @@ static int __init intel_irq_remapping_supported(void)
 static int __init intel_enable_irq_remapping(void)
 {
 	struct dmar_drhd_unit *drhd;
+	bool x2apic_present;
 	int setup = 0;
 	int eim = 0;
 
+	x2apic_present = x2apic_supported();
+
 	if (parse_ioapics_under_ir() != 1) {
 		printk(KERN_INFO "Not enable interrupt remapping\n");
-		return -1;
+		goto error;
 	}
 
-	if (x2apic_supported()) {
+	if (x2apic_present) {
 		eim = !dmar_x2apic_optout();
-		WARN(!eim, KERN_WARNING
-			   "Your BIOS is broken and requested that x2apic be disabled\n"
-			   "This will leave your machine vulnerable to irq-injection attacks\n"
-			   "Use 'intremap=no_x2apic_optout' to override BIOS request\n");
+		if (!eim)
+			printk(KERN_WARNING
+				"Your BIOS is broken and requested that x2apic be disabled.\n"
+				"This will slightly decrease performance.\n"
+				"Use 'intremap=no_x2apic_optout' to override BIOS request.\n");
 	}
 
 	for_each_drhd_unit(drhd) {
@@ -578,7 +593,7 @@ static int __init intel_enable_irq_remapping(void)
 		if (eim && !ecap_eim_support(iommu->ecap)) {
 			printk(KERN_INFO "DRHD %Lx: EIM not supported by DRHD, "
 			       " ecap %Lx\n", drhd->reg_base_addr, iommu->ecap);
-			return -1;
+			goto error;
 		}
 	}
 
@@ -594,7 +609,7 @@ static int __init intel_enable_irq_remapping(void)
 			printk(KERN_ERR "DRHD %Lx: failed to enable queued, "
 			       " invalidation, ecap %Lx, ret %d\n",
 			       drhd->reg_base_addr, iommu->ecap, ret);
-			return -1;
+			goto error;
 		}
 	}
 
@@ -625,6 +640,11 @@ error:
 	/*
 	 * handle error condition gracefully here!
 	 */
+
+	if (x2apic_present)
+		WARN(1, KERN_WARNING
+			"Failed to enable irq remapping.  You are vulnerable to irq-injection attacks.\n");
+
 	return -1;
 }
 
-- 
1.8.1

[tip:x86/apic] x86/intel/irq_remapping: Clean up x2apic opt-out security warning mess

[tip-bot for Andy Lutomirski]

Commit-ID:  af8d102f999a41c0189bd2cce488bac2ee88c29b
Gitweb:     http://git.kernel.org/tip/af8d102f999a41c0189bd2cce488bac2ee88c29b
Author:     Andy Lutomirski <luto@amacapital.net>
AuthorDate: Fri, 1 Feb 2013 14:57:43 -0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sun, 3 Feb 2013 12:13:48 +0100

x86/intel/irq_remapping: Clean up x2apic opt-out security warning mess

Current kernels print this on my Dell server:

   ------------[ cut here ]------------
   WARNING: at drivers/iommu/intel_irq_remapping.c:542
   intel_enable_irq_remapping+0x7b/0x27e()
   Hardware name: PowerEdge R620
   Your BIOS is broken and requested that x2apic be disabled
   This will leave your machine vulnerable to irq-injection attacks
   Use 'intremap=no_x2apic_optout' to override BIOS request
   [...]
   Enabled IRQ remapping in xapic mode
   x2apic not enabled, IRQ remapping is in xapic mode

This is inconsistent with itself -- interrupt remapping is *on*.

Fix the mess by making the warnings say what they mean and my
making sure that compatibility format interrupts (the dangerous
ones) are disabled if x2apic is present regardless of BIOS
settings.

With this patch applied, the output is:

  Your BIOS is broken and requested that x2apic be disabled.
  This will slightly decrease performance.
  Use 'intremap=no_x2apic_optout' to override BIOS request.
  Enabled IRQ remapping in xapic mode
  x2apic not enabled, IRQ remapping is in xapic mode

This should make us as or more secure than we are now and
replace a rather scary warning with a much less scary warning on
silly but functional systems.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
Cc: Suresh Siddha <suresh.b.siddha@intel.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Gleb Natapov <gleb@redhat.com>
Cc: Don Zickus <dzickus@redhat.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Link: http://lkml.kernel.org/r/2011b943a886fd7c46079eb10bc24fc130587503.1359759303.git.luto@amacapital.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/iommu/intel_irq_remapping.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
index 7ca4947..f3b8f23 100644
--- a/drivers/iommu/intel_irq_remapping.c
+++ b/drivers/iommu/intel_irq_remapping.c
@@ -429,11 +429,22 @@ static void iommu_set_irq_remapping(struct intel_iommu *iommu, int mode)
 
 	/* Enable interrupt-remapping */
 	iommu->gcmd |= DMA_GCMD_IRE;
+	iommu->gcmd &= ~DMA_GCMD_CFI;  /* Block compatibility-format MSIs */
 	writel(iommu->gcmd, iommu->reg + DMAR_GCMD_REG);
 
 	IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG,
 		      readl, (sts & DMA_GSTS_IRES), sts);
 
+	/*
+	 * With CFI clear in the Global Command register, we should be
+	 * protected from dangerous (i.e. compatibility) interrupts
+	 * regardless of x2apic status.  Check just to be sure.
+	 */
+	if (sts & DMA_GSTS_CFIS)
+		WARN(1, KERN_WARNING
+			"Compatibility-format IRQs enabled despite intr remapping;\n"
+			"you are vulnerable to IRQ injection.\n");
+
 	raw_spin_unlock_irqrestore(&iommu->register_lock, flags);
 }
 
@@ -530,20 +541,24 @@ static int __init intel_irq_remapping_supported(void)
 static int __init intel_enable_irq_remapping(void)
 {
 	struct dmar_drhd_unit *drhd;
+	bool x2apic_present;
 	int setup = 0;
 	int eim = 0;
 
+	x2apic_present = x2apic_supported();
+
 	if (parse_ioapics_under_ir() != 1) {
 		printk(KERN_INFO "Not enable interrupt remapping\n");
-		return -1;
+		goto error;
 	}
 
-	if (x2apic_supported()) {
+	if (x2apic_present) {
 		eim = !dmar_x2apic_optout();
-		WARN(!eim, KERN_WARNING
-			   "Your BIOS is broken and requested that x2apic be disabled\n"
-			   "This will leave your machine vulnerable to irq-injection attacks\n"
-			   "Use 'intremap=no_x2apic_optout' to override BIOS request\n");
+		if (!eim)
+			printk(KERN_WARNING
+				"Your BIOS is broken and requested that x2apic be disabled.\n"
+				"This will slightly decrease performance.\n"
+				"Use 'intremap=no_x2apic_optout' to override BIOS request.\n");
 	}
 
 	for_each_drhd_unit(drhd) {
@@ -582,7 +597,7 @@ static int __init intel_enable_irq_remapping(void)
 		if (eim && !ecap_eim_support(iommu->ecap)) {
 			printk(KERN_INFO "DRHD %Lx: EIM not supported by DRHD, "
 			       " ecap %Lx\n", drhd->reg_base_addr, iommu->ecap);
-			return -1;
+			goto error;
 		}
 	}
 
@@ -598,7 +613,7 @@ static int __init intel_enable_irq_remapping(void)
 			printk(KERN_ERR "DRHD %Lx: failed to enable queued, "
 			       " invalidation, ecap %Lx, ret %d\n",
 			       drhd->reg_base_addr, iommu->ecap, ret);
-			return -1;
+			goto error;
 		}
 	}
 
@@ -637,6 +652,11 @@ error:
 	/*
 	 * handle error condition gracefully here!
 	 */
+
+	if (x2apic_present)
+		WARN(1, KERN_WARNING
+			"Failed to enable irq remapping.  You are vulnerable to irq-injection attacks.\n");
+
 	return -1;
 }
 

Re: [PATCH] intel_irq_remapping: Clean up x2apic optout security warning mess

[Don Zickus]

On Fri, Feb 01, 2013 at 02:57:43PM -0800, Andy Lutomirski wrote:
> Current kernels print this on my Dell server:
> 
>    ------------[ cut here ]------------
>    WARNING: at drivers/iommu/intel_irq_remapping.c:542
>    intel_enable_irq_remapping+0x7b/0x27e()
>    Hardware name: PowerEdge R620
>    Your BIOS is broken and requested that x2apic be disabled
>    This will leave your machine vulnerable to irq-injection attacks
>    Use 'intremap=no_x2apic_optout' to override BIOS request
>    [...]
>    Enabled IRQ remapping in xapic mode
>    x2apic not enabled, IRQ remapping is in xapic mode
> 
> This is inconsistent with itself -- interrupt remapping is *on*.
> 
> Fix the mess by making the warnings say what they mean and my making
> sure that compatibility format interrupts (the dangerous ones) are
> disabled if x2apic is present regardless of BIOS settings.

Yeah, this fixes the problem on our end too.  Thank Andy!

Cheers,
Don

Re: [PATCH] intel_irq_remapping: Clean up x2apic optout security warning mess

[Alex Williamson]

On Fri, 2013-02-01 at 14:57 -0800, Andy Lutomirski wrote:
> Current kernels print this on my Dell server:
> 
>    ------------[ cut here ]------------
>    WARNING: at drivers/iommu/intel_irq_remapping.c:542
>    intel_enable_irq_remapping+0x7b/0x27e()
>    Hardware name: PowerEdge R620
>    Your BIOS is broken and requested that x2apic be disabled
>    This will leave your machine vulnerable to irq-injection attacks
>    Use 'intremap=no_x2apic_optout' to override BIOS request
>    [...]
>    Enabled IRQ remapping in xapic mode
>    x2apic not enabled, IRQ remapping is in xapic mode
> 
> This is inconsistent with itself -- interrupt remapping is *on*.
> 
> Fix the mess by making the warnings say what they mean and my making
> sure that compatibility format interrupts (the dangerous ones) are
> disabled if x2apic is present regardless of BIOS settings.
> 
> With this patch applied, the output is:
> 
>   Your BIOS is broken and requested that x2apic be disabled.
>   This will slightly decrease performance.
>   Use 'intremap=no_x2apic_optout' to override BIOS request.
>   Enabled IRQ remapping in xapic mode
>   x2apic not enabled, IRQ remapping is in xapic mode
> 
> This should make us as or more secure than we are now and replace
> a rather scary warning with a much less scary warning on silly
> but functional systems.
> 
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  drivers/iommu/intel_irq_remapping.c | 36 ++++++++++++++++++++++++++++--------
>  1 file changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
> index af8904d..eca8832 100644
> --- a/drivers/iommu/intel_irq_remapping.c
> +++ b/drivers/iommu/intel_irq_remapping.c
> @@ -425,11 +425,22 @@ static void iommu_set_irq_remapping(struct intel_iommu *iommu, int mode)
>  
>  	/* Enable interrupt-remapping */
>  	iommu->gcmd |= DMA_GCMD_IRE;
> +	iommu->gcmd &= ~DMA_GCMD_CFI;  /* Block compatibility-format MSIs */
>  	writel(iommu->gcmd, iommu->reg + DMAR_GCMD_REG);
>  
>  	IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG,
>  		      readl, (sts & DMA_GSTS_IRES), sts);
>  
> +	/*
> +	 * With CFI clear in the Global Command register, we should be
> +	 * protected from dangerous (i.e. compatibility) interrupts
> +	 * regardless of x2apic status.  Check just to be sure.
> +	 */
> +	if (sts & DMA_GSTS_CFIS)
> +		WARN(1, KERN_WARNING
> +			"Compatibility-format IRQs enabled despite intr remapping;\n"
> +			"you are vulnerable to IRQ injection.\n");
> +
>  	raw_spin_unlock_irqrestore(&iommu->register_lock, flags);
>  }
>  
> @@ -526,20 +537,24 @@ static int __init intel_irq_remapping_supported(void)
>  static int __init intel_enable_irq_remapping(void)
>  {
>  	struct dmar_drhd_unit *drhd;
> +	bool x2apic_present;
>  	int setup = 0;
>  	int eim = 0;
>  
> +	x2apic_present = x2apic_supported();
> +
>  	if (parse_ioapics_under_ir() != 1) {
>  		printk(KERN_INFO "Not enable interrupt remapping\n");
> -		return -1;
> +		goto error;
>  	}
>  
> -	if (x2apic_supported()) {
> +	if (x2apic_present) {
>  		eim = !dmar_x2apic_optout();
> -		WARN(!eim, KERN_WARNING
> -			   "Your BIOS is broken and requested that x2apic be disabled\n"
> -			   "This will leave your machine vulnerable to irq-injection attacks\n"
> -			   "Use 'intremap=no_x2apic_optout' to override BIOS request\n");
> +		if (!eim)
> +			printk(KERN_WARNING
> +				"Your BIOS is broken and requested that x2apic be disabled.\n"
> +				"This will slightly decrease performance.\n"
> +				"Use 'intremap=no_x2apic_optout' to override BIOS request.\n");
>  	}
>  
>  	for_each_drhd_unit(drhd) {
> @@ -578,7 +593,7 @@ static int __init intel_enable_irq_remapping(void)
>  		if (eim && !ecap_eim_support(iommu->ecap)) {
>  			printk(KERN_INFO "DRHD %Lx: EIM not supported by DRHD, "
>  			       " ecap %Lx\n", drhd->reg_base_addr, iommu->ecap);
> -			return -1;
> +			goto error;
>  		}
>  	}
>  
> @@ -594,7 +609,7 @@ static int __init intel_enable_irq_remapping(void)
>  			printk(KERN_ERR "DRHD %Lx: failed to enable queued, "
>  			       " invalidation, ecap %Lx, ret %d\n",
>  			       drhd->reg_base_addr, iommu->ecap, ret);
> -			return -1;
> +			goto error;
>  		}
>  	}
>  
> @@ -625,6 +640,11 @@ error:
>  	/*
>  	 * handle error condition gracefully here!
>  	 */
> +
> +	if (x2apic_present)
> +		WARN(1, KERN_WARNING
> +			"Failed to enable irq remapping.  You are vulnerable to irq-injection attacks.\n");

How so?  Without interrupt remapping, neither KVM assignment or vfio
work unless the user explicitly opts-in to insecure interrupts via
module options.  Are there other attack vectors?  Also, I really like
the code above that forces CFI clear, but I don't think that warning is
correct yet.  According to the paper the vulnerability is enabled if
both x2apic is not enabled and CFI is enabled.  When x2apic is enabled,
does it matter if CFI is enabled?  My understand is no.  Should a
warning only occur if the interrupt remapper is enabled and x2apic is
not enabled and we're not able to clear CFI?  Thanks,

Alex

Re: [PATCH] intel_irq_remapping: Clean up x2apic optout security warning mess

[Andy Lutomirski]

On Mon, Feb 4, 2013 at 11:04 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Fri, 2013-02-01 at 14:57 -0800, Andy Lutomirski wrote:
>> Current kernels print this on my Dell server:
>>
>>    ------------[ cut here ]------------
>>    WARNING: at drivers/iommu/intel_irq_remapping.c:542
>>    intel_enable_irq_remapping+0x7b/0x27e()
>>    Hardware name: PowerEdge R620
>>    Your BIOS is broken and requested that x2apic be disabled
>>    This will leave your machine vulnerable to irq-injection attacks
>>    Use 'intremap=no_x2apic_optout' to override BIOS request
>>    [...]
>>    Enabled IRQ remapping in xapic mode
>>    x2apic not enabled, IRQ remapping is in xapic mode
>>
>> This is inconsistent with itself -- interrupt remapping is *on*.
>>
>> Fix the mess by making the warnings say what they mean and my making
>> sure that compatibility format interrupts (the dangerous ones) are
>> disabled if x2apic is present regardless of BIOS settings.
>>
>> With this patch applied, the output is:
>>
>>   Your BIOS is broken and requested that x2apic be disabled.
>>   This will slightly decrease performance.
>>   Use 'intremap=no_x2apic_optout' to override BIOS request.
>>   Enabled IRQ remapping in xapic mode
>>   x2apic not enabled, IRQ remapping is in xapic mode
>>
>> This should make us as or more secure than we are now and replace
>> a rather scary warning with a much less scary warning on silly
>> but functional systems.
>>
>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>> ---
>>  drivers/iommu/intel_irq_remapping.c | 36 ++++++++++++++++++++++++++++--------
>>  1 file changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
>> index af8904d..eca8832 100644
>> --- a/drivers/iommu/intel_irq_remapping.c
>> +++ b/drivers/iommu/intel_irq_remapping.c
>> @@ -425,11 +425,22 @@ static void iommu_set_irq_remapping(struct intel_iommu *iommu, int mode)
>>
>>       /* Enable interrupt-remapping */
>>       iommu->gcmd |= DMA_GCMD_IRE;
>> +     iommu->gcmd &= ~DMA_GCMD_CFI;  /* Block compatibility-format MSIs */
>>       writel(iommu->gcmd, iommu->reg + DMAR_GCMD_REG);
>>
>>       IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG,
>>                     readl, (sts & DMA_GSTS_IRES), sts);
>>
>> +     /*
>> +      * With CFI clear in the Global Command register, we should be
>> +      * protected from dangerous (i.e. compatibility) interrupts
>> +      * regardless of x2apic status.  Check just to be sure.
>> +      */
>> +     if (sts & DMA_GSTS_CFIS)
>> +             WARN(1, KERN_WARNING
>> +                     "Compatibility-format IRQs enabled despite intr remapping;\n"
>> +                     "you are vulnerable to IRQ injection.\n");
>> +
>>       raw_spin_unlock_irqrestore(&iommu->register_lock, flags);
>>  }
>>
>> @@ -526,20 +537,24 @@ static int __init intel_irq_remapping_supported(void)
>>  static int __init intel_enable_irq_remapping(void)
>>  {
>>       struct dmar_drhd_unit *drhd;
>> +     bool x2apic_present;
>>       int setup = 0;
>>       int eim = 0;
>>
>> +     x2apic_present = x2apic_supported();
>> +
>>       if (parse_ioapics_under_ir() != 1) {
>>               printk(KERN_INFO "Not enable interrupt remapping\n");
>> -             return -1;
>> +             goto error;
>>       }
>>
>> -     if (x2apic_supported()) {
>> +     if (x2apic_present) {
>>               eim = !dmar_x2apic_optout();
>> -             WARN(!eim, KERN_WARNING
>> -                        "Your BIOS is broken and requested that x2apic be disabled\n"
>> -                        "This will leave your machine vulnerable to irq-injection attacks\n"
>> -                        "Use 'intremap=no_x2apic_optout' to override BIOS request\n");
>> +             if (!eim)
>> +                     printk(KERN_WARNING
>> +                             "Your BIOS is broken and requested that x2apic be disabled.\n"
>> +                             "This will slightly decrease performance.\n"
>> +                             "Use 'intremap=no_x2apic_optout' to override BIOS request.\n");
>>       }
>>
>>       for_each_drhd_unit(drhd) {
>> @@ -578,7 +593,7 @@ static int __init intel_enable_irq_remapping(void)
>>               if (eim && !ecap_eim_support(iommu->ecap)) {
>>                       printk(KERN_INFO "DRHD %Lx: EIM not supported by DRHD, "
>>                              " ecap %Lx\n", drhd->reg_base_addr, iommu->ecap);
>> -                     return -1;
>> +                     goto error;
>>               }
>>       }
>>
>> @@ -594,7 +609,7 @@ static int __init intel_enable_irq_remapping(void)
>>                       printk(KERN_ERR "DRHD %Lx: failed to enable queued, "
>>                              " invalidation, ecap %Lx, ret %d\n",
>>                              drhd->reg_base_addr, iommu->ecap, ret);
>> -                     return -1;
>> +                     goto error;
>>               }
>>       }
>>
>> @@ -625,6 +640,11 @@ error:
>>       /*
>>        * handle error condition gracefully here!
>>        */
>> +
>> +     if (x2apic_present)
>> +             WARN(1, KERN_WARNING
>> +                     "Failed to enable irq remapping.  You are vulnerable to irq-injection attacks.\n");
>
> How so?  Without interrupt remapping, neither KVM assignment or vfio
> work unless the user explicitly opts-in to insecure interrupts via
> module options.  Are there other attack vectors?  Also, I really like
> the code above that forces CFI clear, but I don't think that warning is
> correct yet.  According to the paper the vulnerability is enabled if
> both x2apic is not enabled and CFI is enabled.  When x2apic is enabled,
> does it matter if CFI is enabled?  My understand is no.  Should a
> warning only occur if the interrupt remapper is enabled and x2apic is
> not enabled and we're not able to clear CFI?  Thanks,

That's x2apic *present*, not enabled.  The idea is that, on a system
that has x2apic (which AFAIK is the same set of systems that support
interrupt remapping), then you expect interrupt remapping to work,
whether in x2apic mode or xapic mode.  This warning should never
trigger on any hardware I know of (which, admittedly, doesn't mean
much).

I have no objection to rewording the warning, if you have any better
ideas.  I guess the vfio code (and hence the kvm code?) is more
careful than I realized.

--Andy

Re: [PATCH] intel_irq_remapping: Clean up x2apic optout security warning mess

[Alex Williamson]

On Mon, 2013-02-04 at 11:19 -0800, Andy Lutomirski wrote:
> On Mon, Feb 4, 2013 at 11:04 AM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > On Fri, 2013-02-01 at 14:57 -0800, Andy Lutomirski wrote:
> >> Current kernels print this on my Dell server:
> >>
> >>    ------------[ cut here ]------------
> >>    WARNING: at drivers/iommu/intel_irq_remapping.c:542
> >>    intel_enable_irq_remapping+0x7b/0x27e()
> >>    Hardware name: PowerEdge R620
> >>    Your BIOS is broken and requested that x2apic be disabled
> >>    This will leave your machine vulnerable to irq-injection attacks
> >>    Use 'intremap=no_x2apic_optout' to override BIOS request
> >>    [...]
> >>    Enabled IRQ remapping in xapic mode
> >>    x2apic not enabled, IRQ remapping is in xapic mode
> >>
> >> This is inconsistent with itself -- interrupt remapping is *on*.
> >>
> >> Fix the mess by making the warnings say what they mean and my making
> >> sure that compatibility format interrupts (the dangerous ones) are
> >> disabled if x2apic is present regardless of BIOS settings.
> >>
> >> With this patch applied, the output is:
> >>
> >>   Your BIOS is broken and requested that x2apic be disabled.
> >>   This will slightly decrease performance.
> >>   Use 'intremap=no_x2apic_optout' to override BIOS request.
> >>   Enabled IRQ remapping in xapic mode
> >>   x2apic not enabled, IRQ remapping is in xapic mode
> >>
> >> This should make us as or more secure than we are now and replace
> >> a rather scary warning with a much less scary warning on silly
> >> but functional systems.
> >>
> >> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> >> ---
> >>  drivers/iommu/intel_irq_remapping.c | 36 ++++++++++++++++++++++++++++--------
> >>  1 file changed, 28 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
> >> index af8904d..eca8832 100644
> >> --- a/drivers/iommu/intel_irq_remapping.c
> >> +++ b/drivers/iommu/intel_irq_remapping.c
> >> @@ -425,11 +425,22 @@ static void iommu_set_irq_remapping(struct intel_iommu *iommu, int mode)
> >>
> >>       /* Enable interrupt-remapping */
> >>       iommu->gcmd |= DMA_GCMD_IRE;
> >> +     iommu->gcmd &= ~DMA_GCMD_CFI;  /* Block compatibility-format MSIs */
> >>       writel(iommu->gcmd, iommu->reg + DMAR_GCMD_REG);
> >>
> >>       IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG,
> >>                     readl, (sts & DMA_GSTS_IRES), sts);
> >>
> >> +     /*
> >> +      * With CFI clear in the Global Command register, we should be
> >> +      * protected from dangerous (i.e. compatibility) interrupts
> >> +      * regardless of x2apic status.  Check just to be sure.
> >> +      */
> >> +     if (sts & DMA_GSTS_CFIS)
> >> +             WARN(1, KERN_WARNING
> >> +                     "Compatibility-format IRQs enabled despite intr remapping;\n"
> >> +                     "you are vulnerable to IRQ injection.\n");
> >> +
> >>       raw_spin_unlock_irqrestore(&iommu->register_lock, flags);
> >>  }
> >>
> >> @@ -526,20 +537,24 @@ static int __init intel_irq_remapping_supported(void)
> >>  static int __init intel_enable_irq_remapping(void)
> >>  {
> >>       struct dmar_drhd_unit *drhd;
> >> +     bool x2apic_present;
> >>       int setup = 0;
> >>       int eim = 0;
> >>
> >> +     x2apic_present = x2apic_supported();
> >> +
> >>       if (parse_ioapics_under_ir() != 1) {
> >>               printk(KERN_INFO "Not enable interrupt remapping\n");
> >> -             return -1;
> >> +             goto error;
> >>       }
> >>
> >> -     if (x2apic_supported()) {
> >> +     if (x2apic_present) {
> >>               eim = !dmar_x2apic_optout();
> >> -             WARN(!eim, KERN_WARNING
> >> -                        "Your BIOS is broken and requested that x2apic be disabled\n"
> >> -                        "This will leave your machine vulnerable to irq-injection attacks\n"
> >> -                        "Use 'intremap=no_x2apic_optout' to override BIOS request\n");
> >> +             if (!eim)
> >> +                     printk(KERN_WARNING
> >> +                             "Your BIOS is broken and requested that x2apic be disabled.\n"
> >> +                             "This will slightly decrease performance.\n"
> >> +                             "Use 'intremap=no_x2apic_optout' to override BIOS request.\n");
> >>       }
> >>
> >>       for_each_drhd_unit(drhd) {
> >> @@ -578,7 +593,7 @@ static int __init intel_enable_irq_remapping(void)
> >>               if (eim && !ecap_eim_support(iommu->ecap)) {
> >>                       printk(KERN_INFO "DRHD %Lx: EIM not supported by DRHD, "
> >>                              " ecap %Lx\n", drhd->reg_base_addr, iommu->ecap);
> >> -                     return -1;
> >> +                     goto error;
> >>               }
> >>       }
> >>
> >> @@ -594,7 +609,7 @@ static int __init intel_enable_irq_remapping(void)
> >>                       printk(KERN_ERR "DRHD %Lx: failed to enable queued, "
> >>                              " invalidation, ecap %Lx, ret %d\n",
> >>                              drhd->reg_base_addr, iommu->ecap, ret);
> >> -                     return -1;
> >> +                     goto error;
> >>               }
> >>       }
> >>
> >> @@ -625,6 +640,11 @@ error:
> >>       /*
> >>        * handle error condition gracefully here!
> >>        */
> >> +
> >> +     if (x2apic_present)
> >> +             WARN(1, KERN_WARNING
> >> +                     "Failed to enable irq remapping.  You are vulnerable to irq-injection attacks.\n");
> >
> > How so?  Without interrupt remapping, neither KVM assignment or vfio
> > work unless the user explicitly opts-in to insecure interrupts via
> > module options.  Are there other attack vectors?  Also, I really like
> > the code above that forces CFI clear, but I don't think that warning is
> > correct yet.  According to the paper the vulnerability is enabled if
> > both x2apic is not enabled and CFI is enabled.  When x2apic is enabled,
> > does it matter if CFI is enabled?  My understand is no.  Should a
> > warning only occur if the interrupt remapper is enabled and x2apic is
> > not enabled and we're not able to clear CFI?  Thanks,
> 
> That's x2apic *present*, not enabled.  The idea is that, on a system
> that has x2apic (which AFAIK is the same set of systems that support
> interrupt remapping)

I don't know that those are entirely overlapping sets.

> , then you expect interrupt remapping to work,
> whether in x2apic mode or xapic mode.  This warning should never
> trigger on any hardware I know of (which, admittedly, doesn't mean
> much).

Sure, you expect it to work, otherwise the interrupt remapping
supported() callback should have failed.  If we get here, regardless of
x2apic, we might want to print something about failing to enable it.

> I have no objection to rewording the warning, if you have any better
> ideas.  I guess the vfio code (and hence the kvm code?) is more
> careful than I realized.

As above, I think we're only vulnerable if we enable irq remapping,
disable x2apic, and can't disable CFI.  Even then, I'm not sure what
good it is to print scary warnings about vulnerabilities when the
majority of the user base isn't actually doing anything that makes them
vulnerable.

Should we even put the user in a state where they can be vulnerable?
Rather than print a warning we could disable interrupt remapping if
either x2apic isn't enabled or CFI cannot be disabled.  This could be
noted with a pr_info/pr_debug warning and enabled with an iommu= option.
Then, like doing device assignment w/o interrupt remapping, the user has
to opt-in for interrupt remapping that is vulnerable to MSI injection.
Thanks,

Alex

Re: [PATCH] intel_irq_remapping: Clean up x2apic optout security warning mess

[Andy Lutomirski]

On Mon, Feb 4, 2013 at 11:39 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Mon, 2013-02-04 at 11:19 -0800, Andy Lutomirski wrote:
>> On Mon, Feb 4, 2013 at 11:04 AM, Alex Williamson
>> <alex.williamson@redhat.com> wrote:
>> > On Fri, 2013-02-01 at 14:57 -0800, Andy Lutomirski wrote:
>> >> Current kernels print this on my Dell server:
>> >>
>> >>    ------------[ cut here ]------------
>> >>    WARNING: at drivers/iommu/intel_irq_remapping.c:542
>> >>    intel_enable_irq_remapping+0x7b/0x27e()
>> >>    Hardware name: PowerEdge R620
>> >>    Your BIOS is broken and requested that x2apic be disabled
>> >>    This will leave your machine vulnerable to irq-injection attacks
>> >>    Use 'intremap=no_x2apic_optout' to override BIOS request
>> >>    [...]
>> >>    Enabled IRQ remapping in xapic mode
>> >>    x2apic not enabled, IRQ remapping is in xapic mode
>> >>
>> >> This is inconsistent with itself -- interrupt remapping is *on*.
>> >>
>> >> Fix the mess by making the warnings say what they mean and my making
>> >> sure that compatibility format interrupts (the dangerous ones) are
>> >> disabled if x2apic is present regardless of BIOS settings.
>> >>
>> >> With this patch applied, the output is:
>> >>
>> >>   Your BIOS is broken and requested that x2apic be disabled.
>> >>   This will slightly decrease performance.
>> >>   Use 'intremap=no_x2apic_optout' to override BIOS request.
>> >>   Enabled IRQ remapping in xapic mode
>> >>   x2apic not enabled, IRQ remapping is in xapic mode
>> >>
>> >> This should make us as or more secure than we are now and replace
>> >> a rather scary warning with a much less scary warning on silly
>> >> but functional systems.
>> >>
>> >> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>> >> ---
>> >>  drivers/iommu/intel_irq_remapping.c | 36 ++++++++++++++++++++++++++++--------
>> >>  1 file changed, 28 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
>> >> index af8904d..eca8832 100644
>> >> --- a/drivers/iommu/intel_irq_remapping.c
>> >> +++ b/drivers/iommu/intel_irq_remapping.c
>> >> @@ -425,11 +425,22 @@ static void iommu_set_irq_remapping(struct intel_iommu *iommu, int mode)
>> >>
>> >>       /* Enable interrupt-remapping */
>> >>       iommu->gcmd |= DMA_GCMD_IRE;
>> >> +     iommu->gcmd &= ~DMA_GCMD_CFI;  /* Block compatibility-format MSIs */
>> >>       writel(iommu->gcmd, iommu->reg + DMAR_GCMD_REG);
>> >>
>> >>       IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG,
>> >>                     readl, (sts & DMA_GSTS_IRES), sts);
>> >>
>> >> +     /*
>> >> +      * With CFI clear in the Global Command register, we should be
>> >> +      * protected from dangerous (i.e. compatibility) interrupts
>> >> +      * regardless of x2apic status.  Check just to be sure.
>> >> +      */
>> >> +     if (sts & DMA_GSTS_CFIS)
>> >> +             WARN(1, KERN_WARNING
>> >> +                     "Compatibility-format IRQs enabled despite intr remapping;\n"
>> >> +                     "you are vulnerable to IRQ injection.\n");
>> >> +
>> >>       raw_spin_unlock_irqrestore(&iommu->register_lock, flags);
>> >>  }
>> >>
>> >> @@ -526,20 +537,24 @@ static int __init intel_irq_remapping_supported(void)
>> >>  static int __init intel_enable_irq_remapping(void)
>> >>  {
>> >>       struct dmar_drhd_unit *drhd;
>> >> +     bool x2apic_present;
>> >>       int setup = 0;
>> >>       int eim = 0;
>> >>
>> >> +     x2apic_present = x2apic_supported();
>> >> +
>> >>       if (parse_ioapics_under_ir() != 1) {
>> >>               printk(KERN_INFO "Not enable interrupt remapping\n");
>> >> -             return -1;
>> >> +             goto error;
>> >>       }
>> >>
>> >> -     if (x2apic_supported()) {
>> >> +     if (x2apic_present) {
>> >>               eim = !dmar_x2apic_optout();
>> >> -             WARN(!eim, KERN_WARNING
>> >> -                        "Your BIOS is broken and requested that x2apic be disabled\n"
>> >> -                        "This will leave your machine vulnerable to irq-injection attacks\n"
>> >> -                        "Use 'intremap=no_x2apic_optout' to override BIOS request\n");
>> >> +             if (!eim)
>> >> +                     printk(KERN_WARNING
>> >> +                             "Your BIOS is broken and requested that x2apic be disabled.\n"
>> >> +                             "This will slightly decrease performance.\n"
>> >> +                             "Use 'intremap=no_x2apic_optout' to override BIOS request.\n");
>> >>       }
>> >>
>> >>       for_each_drhd_unit(drhd) {
>> >> @@ -578,7 +593,7 @@ static int __init intel_enable_irq_remapping(void)
>> >>               if (eim && !ecap_eim_support(iommu->ecap)) {
>> >>                       printk(KERN_INFO "DRHD %Lx: EIM not supported by DRHD, "
>> >>                              " ecap %Lx\n", drhd->reg_base_addr, iommu->ecap);
>> >> -                     return -1;
>> >> +                     goto error;
>> >>               }
>> >>       }
>> >>
>> >> @@ -594,7 +609,7 @@ static int __init intel_enable_irq_remapping(void)
>> >>                       printk(KERN_ERR "DRHD %Lx: failed to enable queued, "
>> >>                              " invalidation, ecap %Lx, ret %d\n",
>> >>                              drhd->reg_base_addr, iommu->ecap, ret);
>> >> -                     return -1;
>> >> +                     goto error;
>> >>               }
>> >>       }
>> >>
>> >> @@ -625,6 +640,11 @@ error:
>> >>       /*
>> >>        * handle error condition gracefully here!
>> >>        */
>> >> +
>> >> +     if (x2apic_present)
>> >> +             WARN(1, KERN_WARNING
>> >> +                     "Failed to enable irq remapping.  You are vulnerable to irq-injection attacks.\n");
>> >
>> > How so?  Without interrupt remapping, neither KVM assignment or vfio
>> > work unless the user explicitly opts-in to insecure interrupts via
>> > module options.  Are there other attack vectors?  Also, I really like
>> > the code above that forces CFI clear, but I don't think that warning is
>> > correct yet.  According to the paper the vulnerability is enabled if
>> > both x2apic is not enabled and CFI is enabled.  When x2apic is enabled,
>> > does it matter if CFI is enabled?  My understand is no.  Should a
>> > warning only occur if the interrupt remapper is enabled and x2apic is
>> > not enabled and we're not able to clear CFI?  Thanks,
>>
>> That's x2apic *present*, not enabled.  The idea is that, on a system
>> that has x2apic (which AFAIK is the same set of systems that support
>> interrupt remapping)
>
> I don't know that those are entirely overlapping sets.
>
>> , then you expect interrupt remapping to work,
>> whether in x2apic mode or xapic mode.  This warning should never
>> trigger on any hardware I know of (which, admittedly, doesn't mean
>> much).
>
> Sure, you expect it to work, otherwise the interrupt remapping
> supported() callback should have failed.  If we get here, regardless of
> x2apic, we might want to print something about failing to enable it.

Fair enough.

>
>> I have no objection to rewording the warning, if you have any better
>> ideas.  I guess the vfio code (and hence the kvm code?) is more
>> careful than I realized.
>
> As above, I think we're only vulnerable if we enable irq remapping,
> disable x2apic, and can't disable CFI.  Even then, I'm not sure what
> good it is to print scary warnings about vulnerabilities when the
> majority of the user base isn't actually doing anything that makes them
> vulnerable.
>
> Should we even put the user in a state where they can be vulnerable?
> Rather than print a warning we could disable interrupt remapping if
> either x2apic isn't enabled or CFI cannot be disabled.  This could be
> noted with a pr_info/pr_debug warning and enabled with an iommu= option.
> Then, like doing device assignment w/o interrupt remapping, the user has
> to opt-in for interrupt remapping that is vulnerable to MSI injection.
> Thanks,

Hmm.  It may be too late to cleanly disable interrupt remapping at
this point without writing a bunch of code to back out whatever setup
we did.  I'd rather set a flag and then pretend that interrupt
remapping is off for purposes of iommu caps.

I'll send a followup patch later today.

--Andy