|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [Xen-devel] Xen security advisory CVE-2011-1898 - VT-d (PCI passthrough) MSI
> From: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx > [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxx] On > Behalf Of Ian Campbell > Sent: Friday, May 13, 2011 3:25 AM > > 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)); > } > - } I'm OK with this, as the user can always specify "no-intremap" if they really want to boot anyway, and the "force" behavior will be the same. > - if ( iommu_intremap ) > - { > + Unless I'm misreading it, this will prevent users from specifying "no-intremap" to disable the use of IR. Why would you keep the 'if ( iommu_intremap )' on the previous code block but remove it here? > 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"); I disagree with these parts as well. These changes would say that if the user wants IOMMU enabled that the system must also support IR. Let me list several issues with such an approach: This may actually weaken security rather than increase it. Let's face it, most users won't buy new HW just because the latest version of their SW breaks on their current HW (were it only so easy ;-) ). So those users will be forced to either turn off IOMMU or not upgrade Xen. In the former case, they will lose the ability to separate the hypervisor from domains (even dom0 is not equally privileged to the hypervisor) as well as having decreased robustness from driver bugs/FW bugs/etc. In the latter case, they will not get any of the other security and feature fixes from newer versions of Xen. MSI support has been on systems before there was even an IOMMU capability. Why does adding IOMMU make these systems suddenly insecure? All of the same uses that existed before IOMMU are still valid even with it--why disallow them (see above as to why forcing user to turn off IOMMU is bad). IOMMU adds security capabilities. IR adds additional security capabilities. IOMMU allows for fully isolating the hypervisor from domains even if the domains control DMA devices. It helps to protect against buggy drivers or device FW by limiting the areas such bugs can affect to just the DMA data buffers. IOMMU, in conjunction with Intel(R) Trusted Execution Technology (TXT), provides DMA protection through the entire launch process and into runtime. This is all true regardless of the presence of IR. IR adds the ability to prevent DoS attacks by untrusted domains with assigned DMA devices, malicious device FW, etc. This is incremental--not all or nothing. The 00-block-msis-on-trap-vectors patch (esp. in conjunction with TXT) prevents all known security exploits of MSI misuse. Might there still be vulnerabilities to MSI--yes, and when they're found they will be fixed. If we find a code injection bug that would have been prevented by XD/NX, do we disallow running on systems without XD/NX capability just because there might be more such bugs--no, we fix the ones we know about and will fix others as they are found. If you really want to fail to run insecurely, here is the patch you need: diff -r f9bb0bbea7c2 xen/arch/x86/boot/head.S --- a/xen/arch/x86/boot/head.S Thu May 12 16:42:54 2011 +0100 +++ b/xen/arch/x86/boot/head.S Mon May 16 14:40:50 2011 -0700 @@ -20,7 +20,7 @@ #define BOOT_PSEUDORM_DS 0x0028 ENTRY(start) - jmp __start + hlt .align 4 /*** MULTIBOOT HEADER ****/ Joe _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |