* [PATCH] x86, x2apic: Only WARN on broken BIOSes inside a virtual guest @ 2013-01-31 16:40 Don Zickus 2013-01-31 18:52 ` Gleb Natapov 0 siblings, 1 reply; 13+ messages in thread From: Don Zickus @ 2013-01-31 16:40 UTC (permalink / raw) To: x86; +Cc: LKML, Don Zickus, Suresh Siddha, H. Peter Anvin, Prarit Bhargava 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] x86, x2apic: Only WARN on broken BIOSes inside a virtual guest 2013-01-31 16:40 [PATCH] x86, x2apic: Only WARN on broken BIOSes inside a virtual guest Don Zickus @ 2013-01-31 18:52 ` Gleb Natapov 2013-01-31 19:34 ` Don Zickus 0 siblings, 1 reply; 13+ messages in thread From: Gleb Natapov @ 2013-01-31 18:52 UTC (permalink / raw) To: Don Zickus; +Cc: x86, LKML, Suresh Siddha, H. Peter Anvin, Prarit Bhargava 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86, x2apic: Only WARN on broken BIOSes inside a virtual guest 2013-01-31 18:52 ` Gleb Natapov @ 2013-01-31 19:34 ` Don Zickus 2013-01-31 20:00 ` Gleb Natapov 0 siblings, 1 reply; 13+ messages in thread From: Don Zickus @ 2013-01-31 19:34 UTC (permalink / raw) To: Gleb Natapov; +Cc: x86, LKML, Suresh Siddha, H. Peter Anvin, Prarit Bhargava 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86, x2apic: Only WARN on broken BIOSes inside a virtual guest 2013-01-31 19:34 ` Don Zickus @ 2013-01-31 20:00 ` Gleb Natapov 2013-01-31 20:52 ` Alex Williamson 0 siblings, 1 reply; 13+ messages in thread From: Gleb Natapov @ 2013-01-31 20:00 UTC (permalink / raw) To: Don Zickus Cc: x86, LKML, Suresh Siddha, H. Peter Anvin, Prarit Bhargava, alex.williamson 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86, x2apic: Only WARN on broken BIOSes inside a virtual guest 2013-01-31 20:00 ` Gleb Natapov @ 2013-01-31 20:52 ` Alex Williamson 2013-02-01 22:00 ` Andy Lutomirski 0 siblings, 1 reply; 13+ messages in thread From: Alex Williamson @ 2013-01-31 20:52 UTC (permalink / raw) To: Gleb Natapov Cc: Don Zickus, x86, LKML, Suresh Siddha, H. Peter Anvin, Prarit Bhargava 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86, x2apic: Only WARN on broken BIOSes inside a virtual guest 2013-01-31 20:52 ` Alex Williamson @ 2013-02-01 22:00 ` Andy Lutomirski 2013-02-01 22:57 ` [PATCH] intel_irq_remapping: Clean up x2apic optout security warning mess Andy Lutomirski 0 siblings, 1 reply; 13+ messages in thread From: Andy Lutomirski @ 2013-02-01 22:00 UTC (permalink / raw) To: Alex Williamson Cc: Gleb Natapov, Don Zickus, x86, LKML, Suresh Siddha, H. Peter Anvin, Prarit Bhargava 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] intel_irq_remapping: Clean up x2apic optout security warning mess 2013-02-01 22:00 ` Andy Lutomirski @ 2013-02-01 22:57 ` Andy Lutomirski 2013-02-03 19:29 ` [tip:x86/apic] x86/intel/irq_remapping: Clean up x2apic opt-out " tip-bot for Andy Lutomirski ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Andy Lutomirski @ 2013-02-01 22:57 UTC (permalink / raw) To: Gleb Natapov, Don Zickus, Alex Williamson Cc: x86, LKML, Suresh Siddha, H. Peter Anvin, Prarit Bhargava, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [tip:x86/apic] x86/intel/irq_remapping: Clean up x2apic opt-out security warning mess 2013-02-01 22:57 ` [PATCH] intel_irq_remapping: Clean up x2apic optout security warning mess Andy Lutomirski @ 2013-02-03 19:29 ` tip-bot for Andy Lutomirski 2013-02-04 18:20 ` [PATCH] intel_irq_remapping: Clean up x2apic optout " Don Zickus 2013-02-04 19:04 ` Alex Williamson 2 siblings, 0 replies; 13+ messages in thread From: tip-bot for Andy Lutomirski @ 2013-02-03 19:29 UTC (permalink / raw) To: linux-tip-commits Cc: linux-kernel, hpa, mingo, alex.williamson, luto, gleb, suresh.b.siddha, tglx, prarit, dzickus 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; } ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] intel_irq_remapping: Clean up x2apic optout security warning mess 2013-02-01 22:57 ` [PATCH] intel_irq_remapping: Clean up x2apic optout security warning mess Andy Lutomirski 2013-02-03 19:29 ` [tip:x86/apic] x86/intel/irq_remapping: Clean up x2apic opt-out " tip-bot for Andy Lutomirski @ 2013-02-04 18:20 ` Don Zickus 2013-02-04 19:04 ` Alex Williamson 2 siblings, 0 replies; 13+ messages in thread From: Don Zickus @ 2013-02-04 18:20 UTC (permalink / raw) To: Andy Lutomirski Cc: Gleb Natapov, Alex Williamson, x86, LKML, Suresh Siddha, H. Peter Anvin, Prarit Bhargava 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] intel_irq_remapping: Clean up x2apic optout security warning mess 2013-02-01 22:57 ` [PATCH] intel_irq_remapping: Clean up x2apic optout security warning mess Andy Lutomirski 2013-02-03 19:29 ` [tip:x86/apic] x86/intel/irq_remapping: Clean up x2apic opt-out " tip-bot for Andy Lutomirski 2013-02-04 18:20 ` [PATCH] intel_irq_remapping: Clean up x2apic optout " Don Zickus @ 2013-02-04 19:04 ` Alex Williamson 2013-02-04 19:19 ` Andy Lutomirski 2 siblings, 1 reply; 13+ messages in thread From: Alex Williamson @ 2013-02-04 19:04 UTC (permalink / raw) To: Andy Lutomirski Cc: Gleb Natapov, Don Zickus, x86, LKML, Suresh Siddha, H. Peter Anvin, Prarit Bhargava 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] intel_irq_remapping: Clean up x2apic optout security warning mess 2013-02-04 19:04 ` Alex Williamson @ 2013-02-04 19:19 ` Andy Lutomirski 2013-02-04 19:39 ` Alex Williamson 0 siblings, 1 reply; 13+ messages in thread From: Andy Lutomirski @ 2013-02-04 19:19 UTC (permalink / raw) To: Alex Williamson Cc: Gleb Natapov, Don Zickus, x86, LKML, Suresh Siddha, H. Peter Anvin, Prarit Bhargava 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] intel_irq_remapping: Clean up x2apic optout security warning mess 2013-02-04 19:19 ` Andy Lutomirski @ 2013-02-04 19:39 ` Alex Williamson 2013-02-04 19:47 ` Andy Lutomirski 0 siblings, 1 reply; 13+ messages in thread From: Alex Williamson @ 2013-02-04 19:39 UTC (permalink / raw) To: Andy Lutomirski Cc: Gleb Natapov, Don Zickus, x86, LKML, Suresh Siddha, H. Peter Anvin, Prarit Bhargava 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] intel_irq_remapping: Clean up x2apic optout security warning mess 2013-02-04 19:39 ` Alex Williamson @ 2013-02-04 19:47 ` Andy Lutomirski 0 siblings, 0 replies; 13+ messages in thread From: Andy Lutomirski @ 2013-02-04 19:47 UTC (permalink / raw) To: Alex Williamson Cc: Gleb Natapov, Don Zickus, x86, LKML, Suresh Siddha, H. Peter Anvin, Prarit Bhargava 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-02-04 19:48 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-01-31 16:40 [PATCH] x86, x2apic: Only WARN on broken BIOSes inside a virtual guest Don Zickus 2013-01-31 18:52 ` Gleb Natapov 2013-01-31 19:34 ` Don Zickus 2013-01-31 20:00 ` Gleb Natapov 2013-01-31 20:52 ` Alex Williamson 2013-02-01 22:00 ` Andy Lutomirski 2013-02-01 22:57 ` [PATCH] intel_irq_remapping: Clean up x2apic optout security warning mess Andy Lutomirski 2013-02-03 19:29 ` [tip:x86/apic] x86/intel/irq_remapping: Clean up x2apic opt-out " tip-bot for Andy Lutomirski 2013-02-04 18:20 ` [PATCH] intel_irq_remapping: Clean up x2apic optout " Don Zickus 2013-02-04 19:04 ` Alex Williamson 2013-02-04 19:19 ` Andy Lutomirski 2013-02-04 19:39 ` Alex Williamson 2013-02-04 19:47 ` Andy Lutomirski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).