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

[Noboru Iwamatsu <n_iwamatsu@jp.fujitsu.com>]

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

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 <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-rmrr-validation-fix.patch --]
[-- Type: text/plain, Size: 4881 bytes --]

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

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

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