[PATCH] intel_iommu: Disable vfio and kvm interrupt assignment when unsafe

[PATCH] intel_iommu: Disable vfio and kvm interrupt assignment when unsafe

[Andy Lutomirski]

We currently report IOMMU_CAP_INTR_REMAP whenever interrupt remapping
is enabled.  Users of that capability expect it to mean that remapping
is secure (i.e. compatibility format interrupts are blocked).  Explicitly
check whether CFIs are blocked and, if not, don't report the capability.

Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Don Zickus <dzickus@redhat.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---

This applies on top of my previous patch:

http://git.kernel.org/tip/af8d102f999a41c0189bd2cce488bac2ee88c29b

This is poorly tested.  My x2apic-opted-out server boots and appears
to work.  My x2apic-supporting server is busy doing production
things, so I can't reboot it to test.  I don't have anything that
uses vfio or kvm device assignment.

 drivers/iommu/intel-iommu.c         |  9 +++++++--
 drivers/iommu/intel_irq_remapping.c | 25 ++++++++++++++-----------
 include/linux/intel-iommu.h         |  7 +++++++
 3 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index c2c07a4..b764117 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4129,8 +4129,13 @@ static int intel_iommu_domain_has_cap(struct iommu_domain *domain,
 
 	if (cap == IOMMU_CAP_CACHE_COHERENCY)
 		return dmar_domain->iommu_snooping;
-	if (cap == IOMMU_CAP_INTR_REMAP)
-		return irq_remapping_enabled;
+	if (cap == IOMMU_CAP_INTR_REMAP) {
+		/*
+		 * This could be per-domain, but unless something's wrong,
+		 * we should have CFIs blocked on all domains.
+		 */
+		return irq_remapping_enabled && irq_remapping_is_secure;
+	}
 
 	return 0;
 }
diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
index eca8832..c7eda74 100644
--- a/drivers/iommu/intel_irq_remapping.c
+++ b/drivers/iommu/intel_irq_remapping.c
@@ -37,6 +37,7 @@ struct hpet_scope {
 static struct ioapic_scope ir_ioapic[MAX_IO_APICS];
 static struct hpet_scope ir_hpet[MAX_HPET_TBS];
 static int ir_ioapic_num, ir_hpet_num;
+bool irq_remapping_is_secure;
 
 static DEFINE_RAW_SPINLOCK(irq_2_ir_lock);
 
@@ -436,10 +437,15 @@ static void iommu_set_irq_remapping(struct intel_iommu *iommu, int mode)
 	 * protected from dangerous (i.e. compatibility) interrupts
 	 * regardless of x2apic status.  Check just to be sure.
 	 */
-	if (sts & DMA_GSTS_CFIS)
+	if (sts & DMA_GSTS_CFIS) {
+		/* This should not happen unless the hardware is broken. */
 		WARN(1, KERN_WARNING
-			"Compatibility-format IRQs enabled despite intr remapping;\n"
-			"you are vulnerable to IRQ injection.\n");
+			"Failed to block compatibility format interrupts.\n");
+		iommu->cfi_blocked = false;
+	} else {
+		/* We are now secure against irq injection attack. */
+		iommu->cfi_blocked = true;
+	}
 
 	raw_spin_unlock_irqrestore(&iommu->register_lock, flags);
 }
@@ -537,18 +543,15 @@ 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");
 		goto error;
 	}
 
-	if (x2apic_present) {
+	if (x2apic_supported()) {
 		eim = !dmar_x2apic_optout();
 		if (!eim)
 			printk(KERN_WARNING
@@ -616,6 +619,7 @@ static int __init intel_enable_irq_remapping(void)
 	/*
 	 * Setup Interrupt-remapping for all the DRHD's now.
 	 */
+	irq_remapping_is_secure = 1;
 	for_each_drhd_unit(drhd) {
 		struct intel_iommu *iommu = drhd->iommu;
 
@@ -624,6 +628,8 @@ static int __init intel_enable_irq_remapping(void)
 
 		if (intel_setup_irq_remapping(iommu, eim))
 			goto error;
+		if (!iommu->cfi_blocked)
+			irq_remapping_is_secure = 0;
 
 		setup = 1;
 	}
@@ -641,10 +647,7 @@ 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");
-
+	irq_remapping_is_secure = 0;
 	return -1;
 }
 
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 78e2ada..85cb711 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -333,6 +333,7 @@ struct intel_iommu {
 
 #ifdef CONFIG_IRQ_REMAP
 	struct ir_table *ir_table;	/* Interrupt remapping info */
+	bool cfi_blocked;
 #endif
 	int		node;
 };
@@ -365,4 +366,10 @@ extern int qi_submit_sync(struct qi_desc *desc, struct intel_iommu *iommu);
 
 extern int dmar_ir_support(void);
 
+#ifdef CONFIG_IRQ_REMAP
+extern bool irq_remapping_is_secure;  /* Do all iommus block CFI? */
+#else
+#define irq_remapping_is_secure 0
+#endif
+
 #endif
-- 
1.8.1

Re: [PATCH] intel_iommu: Disable vfio and kvm interrupt assignment when unsafe

[Andy Lutomirski]

On Wed, Feb 6, 2013 at 7:08 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> We currently report IOMMU_CAP_INTR_REMAP whenever interrupt remapping
> is enabled.  Users of that capability expect it to mean that remapping
> is secure (i.e. compatibility format interrupts are blocked).  Explicitly
> check whether CFIs are blocked and, if not, don't report the capability.

FWIW, I've wanted a feature IOMMU_CAP_SECURE that means that all DMA
and MSI from the domain is secure (i.e. only does what is explicitly
requested via the iommu api).  The current situation is hard to
understand, as evidenced by the iommu type 1 stuff in vfio.  But I
don't even understand what an iommu group is, and I've read a decent
chunk of the code.  But that's not really relevant to this patch.

--Andy

Re: [PATCH] intel_iommu: Disable vfio and kvm interrupt assignment when unsafe

[Joerg Roedel]

On Wed, Feb 06, 2013 at 07:08:24PM -0800, Andy Lutomirski wrote:
> -	if (x2apic_present)
> -		WARN(1, KERN_WARNING
> -			"Failed to enable irq remapping.  You are vulnerable to irq-injection attacks.\n");
> -
> +	irq_remapping_is_secure = 0;
>  	return -1;
>  }

Why do you remove this warning? It seems unrelated to the rest of the
patch.


	Joerg

Re: [PATCH] intel_iommu: Disable vfio and kvm interrupt assignment when unsafe

[Andy Lutomirski]

On Thu, Feb 7, 2013 at 3:33 AM, Joerg Roedel <joro@8bytes.org> wrote:
> On Wed, Feb 06, 2013 at 07:08:24PM -0800, Andy Lutomirski wrote:
>> -     if (x2apic_present)
>> -             WARN(1, KERN_WARNING
>> -                     "Failed to enable irq remapping.  You are vulnerable to irq-injection attacks.\n");
>> -
>> +     irq_remapping_is_secure = 0;
>>       return -1;
>>  }
>
> Why do you remove this warning? It seems unrelated to the rest of the
> patch.

The idea is that setting irq_remapping_is_secure = 0 makes you (much
less) vulnerable to irq-injection attacks: you're vulnerable to
malicious hardware but not to attack via vfio or kvm, because those
paths are disabled.

I'd have no problem leaving the warning in and letting whoever manages
to trigger it and get annoyed fix it.  FWIW, it's actually likely to
be interesting if the warning hits.

--Andy

Re: [PATCH] intel_iommu: Disable vfio and kvm interrupt assignment when unsafe

[Joerg Roedel]

On Thu, Feb 07, 2013 at 08:29:42AM -0800, Andy Lutomirski wrote:
> On Thu, Feb 7, 2013 at 3:33 AM, Joerg Roedel <joro@8bytes.org> wrote:
> > On Wed, Feb 06, 2013 at 07:08:24PM -0800, Andy Lutomirski wrote:
> >> -     if (x2apic_present)
> >> -             WARN(1, KERN_WARNING
> >> -                     "Failed to enable irq remapping.  You are vulnerable to irq-injection attacks.\n");
> >> -
> >> +     irq_remapping_is_secure = 0;
> >>       return -1;
> >>  }
> >
> > Why do you remove this warning? It seems unrelated to the rest of the
> > patch.
> 
> The idea is that setting irq_remapping_is_secure = 0 makes you (much
> less) vulnerable to irq-injection attacks: you're vulnerable to
> malicious hardware but not to attack via vfio or kvm, because those
> paths are disabled.
> 
> I'd have no problem leaving the warning in and letting whoever manages
> to trigger it and get annoyed fix it.  FWIW, it's actually likely to
> be interesting if the warning hits.

Hmm, looking into the intel_irq_remapping.c version in the tip tree
makes me wonder even more.

First, I wonder why the warning only hits when an x2apic is present. The
function is not x2apic-specific and the vulnerability also exists in
xapic mode. So that dependency can be removed.

Second, I think that it should be a pr_warn instead of a full WARN. When
IRQ remapping could not be enabled it's most likely because of the BIOS
or the hardware. So a message in the kernel log will do and the
backtrace provides no additional value.

Same is true for the warning in the function iommu_set_irq_remapping():

       if (sts & DMA_GSTS_CFIS)
                WARN(1, KERN_WARNING
                        "Compatibility-format IRQs enabled despite intr remapping;\n"
                        "you are vulnerable to IRQ injection.\n");

>From what I can see this condition depends only on the hardware too. So
a simple pr_warn() provides the same amount of information.


Regards,

	Joerg

Re: [PATCH] intel_iommu: Disable vfio and kvm interrupt assignment when unsafe

[Andy Lutomirski]

On Thu, Feb 7, 2013 at 9:27 AM, Joerg Roedel <joro@8bytes.org> wrote:
> On Thu, Feb 07, 2013 at 08:29:42AM -0800, Andy Lutomirski wrote:
>> On Thu, Feb 7, 2013 at 3:33 AM, Joerg Roedel <joro@8bytes.org> wrote:
>> > On Wed, Feb 06, 2013 at 07:08:24PM -0800, Andy Lutomirski wrote:
>> >> -     if (x2apic_present)
>> >> -             WARN(1, KERN_WARNING
>> >> -                     "Failed to enable irq remapping.  You are vulnerable to irq-injection attacks.\n");
>> >> -
>> >> +     irq_remapping_is_secure = 0;
>> >>       return -1;
>> >>  }
>> >
>> > Why do you remove this warning? It seems unrelated to the rest of the
>> > patch.
>>
>> The idea is that setting irq_remapping_is_secure = 0 makes you (much
>> less) vulnerable to irq-injection attacks: you're vulnerable to
>> malicious hardware but not to attack via vfio or kvm, because those
>> paths are disabled.
>>
>> I'd have no problem leaving the warning in and letting whoever manages
>> to trigger it and get annoyed fix it.  FWIW, it's actually likely to
>> be interesting if the warning hits.
>
> Hmm, looking into the intel_irq_remapping.c version in the tip tree
> makes me wonder even more.

Is this the version I'm based on (intel_irq_remapping: Clean up x2apic
optout security warning mess), or something else?

>
> First, I wonder why the warning only hits when an x2apic is present. The
> function is not x2apic-specific and the vulnerability also exists in
> xapic mode. So that dependency can be removed.
>
> Second, I think that it should be a pr_warn instead of a full WARN. When
> IRQ remapping could not be enabled it's most likely because of the BIOS
> or the hardware. So a message in the kernel log will do and the
> backtrace provides no additional value.
>

Which warning are you referring to?  Unless I'm failing at reading
code this morning, the result of this patch has no such warning.

> Same is true for the warning in the function iommu_set_irq_remapping():
>
>        if (sts & DMA_GSTS_CFIS)
>                 WARN(1, KERN_WARNING
>                         "Compatibility-format IRQs enabled despite intr remapping;\n"
>                         "you are vulnerable to IRQ injection.\n");
>
> From what I can see this condition depends only on the hardware too. So
> a simple pr_warn() provides the same amount of information.
>

What's the general rule here?  If this warning hits, then my
understanding of the Intel VT-d spec is wrong, and I don't think that
firmware can cause it.  A buggy hypervisor could, I suppose.

--Andy

Re: [PATCH] intel_iommu: Disable vfio and kvm interrupt assignment when unsafe

[Joerg Roedel]

On Thu, Feb 07, 2013 at 09:53:45AM -0800, Andy Lutomirski wrote:
> On Thu, Feb 7, 2013 at 9:27 AM, Joerg Roedel <joro@8bytes.org> wrote:
> > Hmm, looking into the intel_irq_remapping.c version in the tip tree
> > makes me wonder even more.
> 
> Is this the version I'm based on (intel_irq_remapping: Clean up x2apic
> optout security warning mess), or something else?

The current tip/master branch with your previous patches included. Btw.
commit af8d102f99 (which introduces the CFIS check) is also bogus. It
claims to cleanup warning messages but does more, like explicitly
clearing DMA_GCMD_CFI in the global command register. This should have
been a seperate patch with a seperate commit-msg.

Now to the two warnings. First of:

        if (sts & DMA_GSTS_CFIS)
                WARN(1, KERN_WARNING
                        "Compatibility-format IRQs enabled despite intr remapping;\n"
                        "you are vulnerable to IRQ injection.\n");

This one can be removed completly as it will never trigger. The CFIS bit
you check here (according to the VT-d spec) has the same value as what
you write into GCMD.CFI (which you set to 0 earlier in the function).

The other warning:

        if (x2apic_present)
                WARN(1, KERN_WARNING
                        "Failed to enable irq remapping.  You are vulnerable to irq-injection attacks.\n");

Here you should remove the x2apic_present check as this is the
error-path for xapic and x2apic. The vulnerability exists with xapic
too.

> What's the general rule here?  If this warning hits, then my
> understanding of the Intel VT-d spec is wrong, and I don't think that
> firmware can cause it.  A buggy hypervisor could, I suppose.

A buggy hypervisor can not cause the CFIS-check warning. In my
understanding only broken hardware can cause it, but that would be known
already in the form of a hardware erratum. As I said above, after some
reading in the VT-d spec I think the warning can go away (Even clearing
GCMD.CFI can be removed as the value is never set in the VT-d driver, so
the bit is always written as 0 by Linux).

With this in mind, I also think that the patch this thread is about does
not make much sense. It just adds more code to handle a case that could
never happen.

Regards,

	Joerg

Re: [PATCH] intel_iommu: Disable vfio and kvm interrupt assignment when unsafe

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

On Thu, 2013-02-07 at 18:27 +0100, Joerg Roedel wrote:
> 
> Second, I think that it should be a pr_warn instead of a full WARN. When
> IRQ remapping could not be enabled it's most likely because of the BIOS
> or the hardware. So a message in the kernel log will do and the
> backtrace provides no additional value.

Backtraces add visibility and have proven to be extremely useful in the
past for getting people to actually *fix* broken BIOSes.

When kerneloops.org was running, it also gave very good statistics which
helped to apply pressure.

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH] intel_iommu: Disable vfio and kvm interrupt assignment when unsafe

[Joerg Roedel]

On Thu, Feb 07, 2013 at 09:02:38PM +0000, David Woodhouse wrote:
> Backtraces add visibility and have proven to be extremely useful in the
> past for getting people to actually *fix* broken BIOSes.
> 
> When kerneloops.org was running, it also gave very good statistics which
> helped to apply pressure.

That is true in general, but does not apply to the two warnings in
question here. One warning checks for a hypothetical hardware problem
and the other warning could happen for several reasons, not only a
firmware bug.

It would make sense to put a warning in the respective places where a
firmware problem is detected, though. The parse_ioapics_under_ir()
function is a candidate where it would make sense, for example. But in
the error path of the intel_enable_irq_remapping() function a pr_warn
would do the same job.


	Joerg