From mboxrd@z Thu Jan 1 00:00:00 1970 From: Weidong Han Subject: Re: [PATCH] VT-d: improve RMRR validity checking Date: Mon, 25 Jan 2010 17:11:10 +0800 Message-ID: <4B5D602E.2020904@intel.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> <1721112626.20100125100200@eikelenboom.it> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1721112626.20100125100200@eikelenboom.it> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Sander Eikelenboom Cc: "xen-devel@lists.xensource.com" , "Kay, Allen M" , "Cihula, Joseph" , Noboru Iwamatsu , "keir.fraser@eu.citrix.com" List-Id: xen-devel@lists.xenproject.org Sander Eikelenboom wrote: > Hello Weidong, > > Is it possible to enable/disable DRHD's and RMRR's after boot ? > > For example if one would hotplug a pci device, that wasn't existent on boot .. > What would happen considering security ? > Is it possible to enable DRHD for that device although it was non existent at boot ? > There is a "INCLUDE_ALL" DRHD, any hot added devices after boot will be handled by this DRHD. So it is no problem in hotplug case. Regards, Weidong > -- > Sander > > > > Monday, January 25, 2010, 8:56:24 AM, you wrote: > > >> 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 >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>> >>>>> >>> >>> > > > > >