Re: [Xen-devel] Xen security advisory CVE-2011-1898 - VT-d (PCI passthrough) MSI

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2011-05-12 at 14:48 +0100, Ian Jackson wrote:
> The second patch is intended to ensure that when Xen boots with
> "iommu=required" it will also insist that interrupt remapping is
> supported and enabled.  It arranges that booting with that option on
> vulnerable hardware will fail, rather than appearing to succeed but
> actually being vulnerable to guests.
>  Filename: intremap05033.patch
>  SHA1: 1cd26adc5ead0c07b67bf354f03164235d67395c
>  SHA256:
> 7f8c7d95d33bbd5c4f25671b380e70020fda1ba6cb50b67e59131fa8e59c1c66

This patch became 23338:9751bc49639e but it is not clear that it goes
far enough since it appears to rely on TXT or similar since it
implicitly trusts the contents of DMAR (via the call to
platform_supports_intremap()).

I think we should go further and try to be safe by default, even if TXT
is not present. (I don't actually have a VT-d capable machine handy to
properly test this).

8<--------------------------------

# HG changeset patch
# User Ian Campbell <ian.campbell@xxxxxxxxxx>
# Date 1305282251 -3600
# Node ID cfffebdedd0b1f1f3f23f60df269f50f7320226b
# Parent  48d68a57f3e8885366478b418e77b043f73dcb2c
vt-d: [CVE-2011-1898] refuse to boot with VT-d IOMMU enabled without interupt 
remapping

CVE-2011-1898 shows that IOMMU is vulnerable to privilege escalation attacks if
Interrupt Remapping is not available.

23364:9751bc49639e tries to ensure that Interrupt Remapping is always enabled
if iommu=required is passed on the command line. However this relies on being
able to trust the DMAR and therefore requires TXT, as well as the user adding
that particular option.

This patch causes the hypervisor to refuse to boot with VT-d IOMMU enabled
unless Interrupt Remapping is also available. This will prevent unwitting users
of older hardware from running in an insecure mode when they may think they are
secure because they have an IOMMU. It will also prevent a malicious guest from
tricking a system which does support IOMMU into booting without it.

Users with pre-Interrupt Remapping hardware who accept the risks are still able 
to
pass iommu=no-intremap on the command line in order to revert to previous
behaviour.

Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

diff -r 48d68a57f3e8 -r cfffebdedd0b xen/drivers/passthrough/vtd/iommu.c
--- a/xen/drivers/passthrough/vtd/iommu.c       Fri May 13 11:10:13 2011 +0100
+++ b/xen/drivers/passthrough/vtd/iommu.c       Fri May 13 11:24:11 2011 +0100
@@ -1965,32 +1965,17 @@ static int init_vtd_hw(void)
         for ( apic = 0; apic < nr_ioapics; apic++ )
         {
             if ( ioapic_to_iommu(IO_APIC_ID(apic)) == NULL )
-            {
-                iommu_intremap = 0;
-                dprintk(XENLOG_ERR VTDPREFIX,
-                    "ioapic_to_iommu: ioapic 0x%x (id: 0x%x) is NULL! "
-                    "Will not try to enable Interrupt Remapping.\n",
-                    apic, IO_APIC_ID(apic));
-                if ( force_iommu )
-                    panic("intremap remapping failed to enable with 
iommu=required/force in grub\n");
-                break;
-            }
+                panic(VTDPREFIX
+                      "ioapic_to_iommu: ioapic 0x%x (id: 0x%x) is NULL! "
+                      "Unable to enable Interrupt Remapping.\n",
+                      apic, IO_APIC_ID(apic));
         }
-    }
-    if ( iommu_intremap )
-    {
+
         for_each_drhd_unit ( drhd )
         {
             iommu = drhd->iommu;
             if ( enable_intremap(iommu, 0) != 0 )
-            {
-                dprintk(XENLOG_WARNING VTDPREFIX,
-                        "Interrupt Remapping not enabled\n");
-
-                if ( force_iommu && platform_supports_intremap() )
-                    panic("intremap remapping failed to enable with 
iommu=required/force in grub\n");
-                break;
-            }
+                panic(VTDPREFIX "Failed to enable Interrupt Remapping\n");
         }
     }
 
@@ -2066,7 +2051,7 @@ int __init intel_vtd_setup(void)
             iommu_qinval = 0;
 
         if ( iommu_intremap && !ecap_intr_remap(iommu->ecap) )
-            iommu_intremap = 0;
+            panic(VTDPREFIX "IOMMU: hardware does not support Interrupt 
Remapping");
 
         if ( !vtd_ept_page_compatible(iommu) )
             iommu_hap_pt_share = FALSE;
@@ -2081,11 +2066,8 @@ int __init intel_vtd_setup(void)
     }
 
     if ( !iommu_qinval && iommu_intremap )
-    {
-        iommu_intremap = 0;
-        dprintk(XENLOG_WARNING VTDPREFIX, "Interrupt Remapping disabled "
+        panic(XENLOG_WARNING "Interrupt Remapping disabled "
             "since Queued Invalidation isn't supported or enabled.\n");
-    }
 
 #define P(p,s) printk("Intel VT-d %s %senabled.\n", s, (p)? "" : "not ")
     P(iommu_snoop, "Snoop Control");



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel