| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CALCETrVnQqiozr8Kj5bPHF+KEauma3q-2AQx27fkcj630duSOg@mail.gmail.com>
Date: Mon, 4 Feb 2013 11:47:53 -0800
From: Andy Lutomirski <luto@...capital.net>
To: Alex Williamson <alex.williamson@...hat.com>
Cc: Gleb Natapov <gleb@...hat.com>, Don Zickus <dzickus@...hat.com>,
x86@...nel.org, LKML <linux-kernel@...r.kernel.org>,
Suresh Siddha <suresh.b.siddha@...el.com>,
"H. Peter Anvin" <hpa@...or.com>,
Prarit Bhargava <prarit@...hat.com>
Subject: Re: [PATCH] intel_irq_remapping: Clean up x2apic optout security
warning mess
On Mon, Feb 4, 2013 at 11:39 AM, Alex Williamson
<alex.williamson@...hat.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@...hat.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@...capital.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists