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

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

[-- Attachment #1: Type: text/plain, Size: 6476 bytes --]

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>
>>>>>>>>>                   
>>>       
>
>
>   


[-- Attachment #2: drhd-ignore.patch --]
[-- Type: text/plain, Size: 1744 bytes --]

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;
 }
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel