From mboxrd@z Thu Jan 1 00:00:00 1970 From: Noboru Iwamatsu Subject: Re: [PATCH] VT-d: improve RMRR validity checking Date: Mon, 25 Jan 2010 16:06:46 +0900 Message-ID: <4B5D4306.5060307@jp.fujitsu.com> References: <4B59098B.6000108@intel.com> <4B590FA4.4000008@jp.fujitsu.com> <4B59132B.40607@intel.com> <4B59188C.50901@jp.fujitsu.com> <4B59660F.4000909@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------070701090808060906000805" Return-path: In-Reply-To: <4B59660F.4000909@intel.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: weidong.han@intel.com, keir.fraser@eu.citrix.com Cc: linux@eikelenboom.it, joseph.cihula@intel.com, allen.m.kay@intel.com, xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --------------070701090808060906000805 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Weidong, I read the patch and the following thread. I understood what you mean, but I think it's better to limit the scope of "force_iommu". And I believe RMRR should be checked as same as DRHD. What I thought about DRHD is: If all devices under the scope of the DRHD are non-existent, this DRHD is invalid but safely ignorable, so ignore it. If some devices under the scope of the DRHD are non-existent, this DRHD is invalid, so disable VT-d unless "iommu=force" option is specified. When "iommu=force" option is specified, even the invalid DRHD will be registered, because DRHD that has some existent devices must not be ignored due to security reasons. About the RMRR: If all devices under the scope of the RMRR are non-existent, this RMMR is invalid but ignorable, so ignore it. If some devices under the scope of the RMRR are non-existent, this RMRR is invalid, so disable VT-d unless "iommu=force" option is specified. When "iommu=force" option is specified, the invalid RMRR is ignored (it's safe). I attach the patch. What do you think? Regards, Noboru. > 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 >> >> > --------------070701090808060906000805 Content-Type: text/plain; name="drhd-rmrr-validation-fix.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="drhd-rmrr-validation-fix.patch" diff -r ca0759a08057 -r 2b40508f7645 xen/drivers/passthrough/vtd/dmar.c --- a/xen/drivers/passthrough/vtd/dmar.c Fri Jan 22 11:01:18 2010 +0000 +++ b/xen/drivers/passthrough/vtd/dmar.c Mon Jan 25 15:36:32 2010 +0900 @@ -396,8 +396,65 @@ if ( ret ) xfree(dmaru); + else if ( 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 all devices under the scope of the DRHD are non-existent, + * this DRHD is invalid but safely ignorable, so ignore it. + * If some devices under the scope of the DRHD are non-existent, + * this DRHD is invalid, so disable VT-d unless "iommu=force" + * option is specified. + * When "iommu=force" option is specified, even the invalid DRHD + * will be registered, because DRHD that has some existent devices + * must not be ignored due to security reasons. + */ + if ( invalid_cnt ) + { + 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"); + xfree(dmaru); + } + else + { + dprintk(XENLOG_WARNING VTDPREFIX, + " The DRHD is invalid due to some devices under " + "its scope are not PCI discoverable!\n"); + if ( force_iommu ) + acpi_register_drhd_unit(dmaru); + else + { + xfree(dmaru); + ret = -EINVAL; + } + } + } + else + acpi_register_drhd_unit(dmaru); + } + return ret; } @@ -444,7 +501,7 @@ else { u8 b, d, f; - int i, ignore = 0; + int i, invalid_cnt = 0; for ( i = 0; i < rmrru->scope.devices_cnt; i++ ) { @@ -458,24 +515,44 @@ " Non-existent device (%x:%x.%x) is reported " "in RMRR (%"PRIx64", %"PRIx64")'s scope!\n", b, d, f, rmrru->base_address, rmrru->end_address); - ignore = 1; + invalid_cnt++; + } + } + + /* + * If all devices under the scope of the RMRR are non-existent, + * this RMMR is invalid but ignorable, so ignore it. + * If some devices under the scope of the RMRR are non-existent, + * this RMRR is invalid, so disable VT-d unless "iommu=force" + * option is specified. When "iommu=force" option is specified, + * the invalid RMRR is ignored. + */ + if ( invalid_cnt ) + { + if ( invalid_cnt == rmrru->scope.devices_cnt ) + { + dprintk(XENLOG_WARNING VTDPREFIX, + " Ignore the RMRR (%"PRIx64", %"PRIx64") due to " + "devices under its scope are not PCI discoverable!\n", + rmrru->base_address, rmrru->end_address); + xfree(rmrru); + return 0; } else { - ignore = 0; - break; + dprintk(XENLOG_WARNING VTDPREFIX, + " The RMRR (%"PRIx64", %"PRIx64") is invalid due to " + "some devices under its scope are not PCI discoverable!\n", + rmrru->base_address, rmrru->end_address); + if ( !force_iommu ) + { + xfree(rmrru); + return -EINVAL; + } } } - if ( ignore ) - { - dprintk(XENLOG_WARNING VTDPREFIX, - " Ignore the RMRR (%"PRIx64", %"PRIx64") due to " - "devices under its scope are not PCI discoverable!\n", - rmrru->base_address, rmrru->end_address); - xfree(rmrru); - } - else if ( base_addr > end_addr ) + if ( base_addr > end_addr ) { dprintk(XENLOG_WARNING VTDPREFIX, " The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n", --------------070701090808060906000805 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 --------------070701090808060906000805--