From mboxrd@z Thu Jan 1 00:00:00 1970 From: Weidong Han Subject: Re: [PATCH] VT-d: improve RMRR validity checking Date: Fri, 22 Jan 2010 16:47:11 +0800 Message-ID: <4B59660F.4000909@intel.com> References: <4B59098B.6000108@intel.com> <4B590FA4.4000008@jp.fujitsu.com> <4B59132B.40607@intel.com> <4B59188C.50901@jp.fujitsu.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------040009010204000508090103" Return-path: In-Reply-To: <4B59188C.50901@jp.fujitsu.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Noboru Iwamatsu Cc: "xen-devel@lists.xensource.com" , "linux@eikelenboom.it" , "Cihula, Joseph" , "Kay, Allen M" , "keir.fraser@eu.citrix.com" List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --------------040009010204000508090103 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit I implemented a patch and attached. patch description: In order to make Xen more defensive to VT-d related BIOS issue, this patch ignores a DRHD if all devices under its scope are not pci discoverable, and regards a DRHD as invalid and then disable whole VT-d if some devices under its scope are not pci discoverable. But if iommu=force is set, it will enable all DRHDs reported by BIOS, to avoid any security vulnerability with malicious s/s re-enabling "supposed disabled" devices. Pls note that we don't know the devices under the "Include_all" DRHD are existent or not, because the scope of "Include_all" DRHD won't enumerate common pci device, it only enumerates I/OxAPIC and HPET devices. Signed-off-by: Noboru Iwamatsu Signed-off-by: Weidong Han Noboru, pls test the patch on your machine? Joe, could you review the patch? and pls ACK it if it's fine for you. Regards, Weidong Noboru Iwamatsu wrote: > Thanks, > > I understood. > > >> Noboru Iwamatsu wrote: >> >>> Hi Weidong, >>> >>> I'm not sure why the security problem is caused by ignoring >>> the DRHD that has only non-existent devices. >>> >>> Could you explain details or where to read the spec? >>> >> It's requested from security experts. The device that is not pci >> discoverable may be re-enabled by malicious software. If its DRHD is not >> enabled, the re-enabled device is not protected by VT-d. It will cause >> security issue. >> >> >>> As you saying, security is the top-priority. >>> However, when iommu=force is specified, we should enable vt-d >>> if there are some potential issues. >>> Because users want to "force" anyway. >>> >> iommu=force was introduced to enable VT-d anyway for security purpose. I >> plan to still enable those DRHDs that includes non-existed device when >> iommu=force, otherwise ignore them. >> >> Regards, >> Weidong >> >>> Regards, >>> Noboru. >>> >>> >>>> Keir Fraser wrote: >>>> >>>>> If we want to keep iommu=1 as default, then it is unacceptable to >>>>> fail to >>>>> boot on a fairly wide range of modern systems. We have to >>>>> warn-and-disable, >>>>> partially or completely, unless iommu=force is specified. Or we need to >>>>> revert to iommu=0 as the default. >>>>> >>>>> What do you think, Weidong? >>>>> >>>> Yes. I agree to warn-and-disable for these BIOS issues, and consider >>>> security more when iommu=force. Therefore I will implement a patch based >>>> on Nororu's patch. >>>> >>>> Regards, >>>> Weidong >>>> >>>> >>>>> -- Keir >>>>> >>>>> On 21/01/2010 14:17, "Sander Eikelenboom" wrote: >>>>> >>>>> >>>>>> Hello Weidong, >>>>>> >>>>>> The problem is most vendor's just don't fix it and ignore the problem >>>>>> completely. >>>>>> Most often hiding them selves behind: come back when it's a problem >>>>>> with >>>>>> Microsoft Windows, that the only single thing we support (and no other >>>>>> software, so no vmware, no xen, no linux, perhaps even no hypervisor) >>>>>> Well I don't know if the virtual pc in windows 7 supports an iommu >>>>>> now, but it >>>>>> didn't in the past as far as i know, so any complain bounces off, and >>>>>> there it >>>>>> all seems to end for them. >>>>>> >>>>>> Besides that i don't know if they do know what the problems with there >>>>>> implementation in BIOS is when someone reports it. >>>>>> I think some behind the scenes pressure from Intel to vendors might >>>>>> help to >>>>>> solve some of them. >>>>>> (my Q35 chipset, "Intel V-PRO" marketed motherboard (so much for >>>>>> that) also >>>>>> suffers RMRR problem when another graphics card is inserted which >>>>>> switches off >>>>>> the IGD). >>>>>> >>>>>> Although i think in my case your patch will work around that for me. >>>>>> Perhaps a >>>>>> third option is needed, which does all the workarounds possible and >>>>>> warns >>>>>> about potential security problem when requested ? >>>>>> >>>>>> -- >>>>>> Sander >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Thursday, January 21, 2010, 1:46:39 PM, you wrote: >>>>>> >>>>>> >>>>>>> Noboru Iwamatsu wrote: >>>>>>> >>>>>>>> Hi Weidong, >>>>>>>> >>>>>>>> I re-send the DRHD-fix patch. >>>>>>>> >>>>>>>> If DRHD does not have existent devices, ignore it. >>>>>>>> If DRHD has both existent and non-existent devices, consider it >>>>>>>> invalid >>>>>>>> and not register. >>>>>>>> >>>>>>> Although you patch workarounds your buggy BIOS, but we still need to >>>>>>> enable it for security purpose as I mentioned in previous mail. We >>>>>>> needn't workaround / fix all BIOS issues in software. I think >>>>>>> security >>>>>>> is more important for this specific BIOS issue. Did you report the >>>>>>> BIOS >>>>>>> issue to your OEM vendor? maybe it's better to get it fixed in BIOS. >>>>>>> Regards, >>>>>>> Weidong >>>>>>> >>>>>>>> According to this patch and yours, my machine successfully booted >>>>>>>> with vt-d enabled. >>>>>>>> >>>>>>>> Signed-off-by: Noboru Iwamatsu >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> Keir Fraser wrote: >>>>>>>>> >>>>>>>>>> On 21/01/2010 10:19, "Weidong Han" wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>>> Sorry this is typo. >>>>>>>>>>>> I mean: >>>>>>>>>>>> So, I think RMRR that has no-existent device is "invalid" >>>>>>>>>>>> and whole RMRR should be ignored. >>>>>>>>>>>> >>>>>>>>>>> looks reasonable. >>>>>>>>>>> >>>>>>>>>>> Keir, I Acks Noboru's rmrr patch. Or do you want us to merge >>>>>>>>>>> them to one >>>>>>>>>>> patch? >>>>>>>>>>> >>>>>>>>>> Merge them up, re-send with both sign-off and acked-by all in one >>>>>>>>>> email. >>>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> Keir >>>>>>>>>> >>>>>>>>>> >>>>>>>>> Sorry, I disagree with Noboru after thinking it again. If the RMRR >>>>>>>>> has >>>>>>>>> both no-existent device and also has existent devices in its >>>>>>>>> scope, we >>>>>>>>> should not ignore it because the existent devices under its scope >>>>>>>>> will >>>>>>>>> be impacted without the RMRR. so I suggest to print a warning >>>>>>>>> instead of >>>>>>>>> ignore it. Attached a patch for it. >>>>>>>>> >>>>>>>>> Signed-off-by: Weidong Han >>>>>>>>> >>> > > > --------------040009010204000508090103 Content-Type: text/plain; name="drhd-ignore.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="drhd-ignore.patch" diff -r 207fba95a7d5 xen/drivers/passthrough/vtd/dmar.c --- a/xen/drivers/passthrough/vtd/dmar.c Fri Jan 22 13:12:45 2010 +0800 +++ b/xen/drivers/passthrough/vtd/dmar.c Fri Jan 22 22:32:10 2010 +0800 @@ -396,8 +396,49 @@ acpi_parse_one_drhd(struct acpi_dmar_ent if ( ret ) xfree(dmaru); + else if ( force_iommu || dmaru->include_all ) + acpi_register_drhd_unit(dmaru); else - acpi_register_drhd_unit(dmaru); + { + u8 b, d, f; + int i, invalid_cnt = 0; + + for ( i = 0; i < dmaru->scope.devices_cnt; i++ ) + { + b = PCI_BUS(dmaru->scope.devices[i]); + d = PCI_SLOT(dmaru->scope.devices[i]); + f = PCI_FUNC(dmaru->scope.devices[i]); + + if ( pci_device_detect(b, d, f) == 0 ) + { + dprintk(XENLOG_WARNING VTDPREFIX, + " Non-existent device (%x:%x.%x) is reported " + "in this DRHD's scope!\n", b, d, f); + invalid_cnt++; + } + } + + if ( invalid_cnt ) + { + xfree(dmaru); + if ( invalid_cnt == dmaru->scope.devices_cnt ) + { + dprintk(XENLOG_WARNING VTDPREFIX, + " Ignore the DRHD due to all devices under " + "its scope are not PCI discoverable!\n"); + } + else + { + dprintk(XENLOG_WARNING VTDPREFIX, + " The DRHD is invalid due to some devices under " + "its scope are not PCI discoverable!\n"); + ret = -EINVAL; + } + } + else + acpi_register_drhd_unit(dmaru); + } + return ret; } --------------040009010204000508090103 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --------------040009010204000508090103--