Re: [PATCH] VT-d: improve RMRR validity checking

[Weidong Han <weidong.han@intel.com>]

Noboru Iwamatsu wrote:
> 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.
>   

Yes, from the VT-d point of view, there might be a device not protected 
by any DRHD if the device is re-enabled by malicious s/w and its DRHD is 
ignored.
> 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".
>   
In order to still boot Xen, we will disable VT-d if detect some BIOS 
bugs, such as incorrect RMRR. iommu=force was introduced to enable VT-d 
anyway for security purpose, that means it won't allow to disable VT-d 
to boot Xen.  I agree to do the strict check for security by default, 
Maybe we can add an option like "workaround_bogus_bios" which will try 
to workaround known BIOS issues, such as ignore DRHD. I don't prefer to 
add too many options like ignore_bogus_rmrr and ignore_bogus_drhd, it's 
not practical for end users.

My suggestion is like:
iommu=1 (default): won't ignore any DRHD. when detect non-existent 
devices under DRHD's scope, and find incorrect RMRR setting, disable 
whole VT-d with warning message. This guarantees security when VT-d is 
enabled, or just disable VT-d to let Xen work without VT-d because there 
are users who don't need VT-d.
iommu=force: keep the same behavior. that's make sure VT-d enabled. It 
won't ignore any DRHD, and if VT-d is disabled due to above BIOS issues, 
it will quit Xen boot with warning message.
iommu=workaround_bogus_bios: if "all" devices under scope, ignore the 
DRHD, if "some" devices under scope, disable whole VT-d in Xen. This 
might be insecure because there might be a device not protected by any 
DRHD if the device is re-enabled by malicious s/w. This is for user who 
want to use VT-d regardless of security.

Welcome comments.

Regards,
Weidong
> 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 <n_iwamatsu@jp.fujitsu.com>
>>>> Signed-off-by: Weidong Han <weidong.han@intel.com>
>>>>
>>>>
>>>> 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" <linux@eikelenboom.it>
>>>>>>>>> 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 <n_iwamatsu@jp.fujitsu.com>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>                         
>>>>>>>>>>>>> Keir Fraser wrote:
>>>>>>>>>>>>>                           
>>>>>>>>>>>>>> On 21/01/2010 10:19, "Weidong Han" <weidong.han@intel.com>
>>>>>>>>>>>>>> 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 <weidong.han@intel.com>
>>>>>>>>>>>>>                           
>
>
>
>