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 18:22:08 +0900 Message-ID: <4B5D62C0.7060404@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> <4B5D4306.5060307@jp.fujitsu.com> <4B5D4EA8.4000206@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4B5D4EA8.4000206@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 Cc: xen-devel@lists.xensource.com, linux@eikelenboom.it, joseph.cihula@intel.com, allen.m.kay@intel.com, keir.fraser@eu.citrix.com List-Id: xen-devel@lists.xenproject.org Hi, > No, we cannot ignore it if iommu=force. The invisible device may be > disabled, not really non-existent. it is possibly that it is re-enabled > by malfunctional s/w. So when iommu=force, we should not ignore any > DRHD. We ignores it just to workaround the BIOS issue you encountered. OK, I return to the same question as Pasi asked. You mean even ignoring the DRHD that has no existent devices is insecure, right? In other word, iommu=1 might be insecure while working with workaround. We might have to consider security and BIOS workaround separately. I believe default action must be secure and enabled with strictly checked values. If "force" or some workaround options (e.g ignore_bogus_rmrr, ignore_bogus_drhd, force_enable_with_bogus_drhd, ...) specified, VT-d enabled with some special workaround, with uncertain values, but these mode should be considered "not always secure". What do you think? Regards, Noboru. > Noboru Iwamatsu wrote: >> 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. > No, we cannot ignore it if iommu=force. The invisible device may be > disabled, not really non-existent. it is possibly that it is re-enabled > by malfunctional s/w. So when iommu=force, we should not ignore any > DRHD. We ignores it just to workaround the BIOS issue you encountered. >> 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" > RMRR is much different from DRHD, it's just reversed memories for > specific devices (now only Intel IGD and USB contollers need RMRR), it's > no security issue like described above. > if "all" devices under the scope of the RMRR are non-existent, we can > ignore the RMRR because no devices will use it. > if some" devices under the scope of the RMRR are non-existent, we cannot > ignore the RMRR, because there are still some devices want to use it. I > think we needn't to disable VT-d because it won't cause any issues. Of > course, we also can disable VT-d for this case strictly. >> 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? > > Noboru, > > I think it need not to change current code. BTW, your patch is not based > on latest Xen. > > Regards, > Weidong > > >> 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 >> >