[PATCH 0/2] VT-d: further XSA-59 workaround adjustments

[PATCH 0/2] VT-d: further XSA-59 workaround adjustments

[Jan Beulich]

While doing the backports of the recently committed XSA-59 workaround
patches, when reaching 4.2 I had to inspect the fuzzy applies resulting
from the x86-64 conditionals in that code, making me realize that what
we're doing is still insufficient: We wrongly assume to be able to access
extended config registers (i.e. MMCFG space) at boot time.

Fixing that, in turn made me again look at the one workaround that was
in place in the same function before that recent series, just to find that
the list very likely should have been extended quite a while back.

1: apply quirks at device setup time rather than only at boot
2: extend error report masking workaround to newer chipsets

This (still) is CVE-2013-3495 / XSA-59.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

[PATCH 1/2] VT-d: apply quirks at device setup time rather than only at boot

[Jan Beulich]

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

Accessing extended config space may not be possible at boot time, e.g.
when the memory space used by MMCFG is reserved only via ACPI tables,
but not in the E820/UEFI memory maps (which we need Dom0 to tell us
about). Consequently the change here still leaves the issue unaddressed
for systems where the extended config space remains inaccessible (due
to firmware bugs, i.e. not properly reserving the address space of
those regions).

With the respective messages now potentially getting logged more than
once, we ought to consider whether we should issue them only if we in
fact were required to do any masking (i.e. if the relevant mask bits
weren't already set).

This is CVE-2013-3495 / XSA-59.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -99,7 +99,7 @@ void platform_quirks_init(void);
 void vtd_ops_preamble_quirk(struct iommu* iommu);
 void vtd_ops_postamble_quirk(struct iommu* iommu);
 void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map);
-void pci_vtd_quirk(struct pci_dev *pdev);
+void pci_vtd_quirk(const struct pci_dev *);
 int platform_supports_intremap(void);
 int platform_supports_x2apic(void);
 
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1483,6 +1483,9 @@ static int domain_context_mapping(
         break;
     }
 
+    if ( !ret && devfn == pdev->devfn )
+        pci_vtd_quirk(pdev);
+
     return ret;
 }
 
@@ -1922,6 +1925,8 @@ static int intel_iommu_enable_device(str
     struct acpi_drhd_unit *drhd = acpi_find_matched_drhd_unit(pdev);
     int ret = drhd ? ats_device(pdev, drhd) : -ENODEV;
 
+    pci_vtd_quirk(pdev);
+
     if ( ret <= 0 )
         return ret;
 
@@ -1994,12 +1999,7 @@ static int intel_iommu_remove_device(u8 
 
 static int __hwdom_init setup_hwdom_device(u8 devfn, struct pci_dev *pdev)
 {
-    int err;
-
-    err = domain_context_mapping(pdev->domain, devfn, pdev);
-    if ( !err && devfn == pdev->devfn )
-        pci_vtd_quirk(pdev);
-    return err;
+    return domain_context_mapping(pdev->domain, devfn, pdev);
 }
 
 void clear_fault_bits(struct iommu *iommu)
--- a/xen/drivers/passthrough/vtd/quirks.c
+++ b/xen/drivers/passthrough/vtd/quirks.c
@@ -385,7 +385,7 @@ void me_wifi_quirk(struct domain *domain
  *   - This can cause system failure upon non-fatal VT-d faults
  *   - Potential security issue if malicious guest trigger VT-d faults
  */
-void __hwdom_init pci_vtd_quirk(struct pci_dev *pdev)
+void pci_vtd_quirk(const struct pci_dev *pdev)
 {
     int seg = pdev->seg;
     int bus = pdev->bus;




[-- Attachment #2: VT-d-quirk-runtime.patch --]
[-- Type: text/plain, Size: 2761 bytes --]

VT-d: apply quirks at device setup time rather than only at boot

Accessing extended config space may not be possible at boot time, e.g.
when the memory space used by MMCFG is reserved only via ACPI tables,
but not in the E820/UEFI memory maps (which we need Dom0 to tell us
about). Consequently the change here still leaves the issue unaddressed
for systems where the extended config space remains inaccessible (due
to firmware bugs, i.e. not properly reserving the address space of
those regions).

With the respective messages now potentially getting logged more than
once, we ought to consider whether we should issue them only if we in
fact were required to do any masking (i.e. if the relevant mask bits
weren't already set).

This is CVE-2013-3495 / XSA-59.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -99,7 +99,7 @@ void platform_quirks_init(void);
 void vtd_ops_preamble_quirk(struct iommu* iommu);
 void vtd_ops_postamble_quirk(struct iommu* iommu);
 void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map);
-void pci_vtd_quirk(struct pci_dev *pdev);
+void pci_vtd_quirk(const struct pci_dev *);
 int platform_supports_intremap(void);
 int platform_supports_x2apic(void);
 
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1483,6 +1483,9 @@ static int domain_context_mapping(
         break;
     }
 
+    if ( !ret && devfn == pdev->devfn )
+        pci_vtd_quirk(pdev);
+
     return ret;
 }
 
@@ -1922,6 +1925,8 @@ static int intel_iommu_enable_device(str
     struct acpi_drhd_unit *drhd = acpi_find_matched_drhd_unit(pdev);
     int ret = drhd ? ats_device(pdev, drhd) : -ENODEV;
 
+    pci_vtd_quirk(pdev);
+
     if ( ret <= 0 )
         return ret;
 
@@ -1994,12 +1999,7 @@ static int intel_iommu_remove_device(u8 
 
 static int __hwdom_init setup_hwdom_device(u8 devfn, struct pci_dev *pdev)
 {
-    int err;
-
-    err = domain_context_mapping(pdev->domain, devfn, pdev);
-    if ( !err && devfn == pdev->devfn )
-        pci_vtd_quirk(pdev);
-    return err;
+    return domain_context_mapping(pdev->domain, devfn, pdev);
 }
 
 void clear_fault_bits(struct iommu *iommu)
--- a/xen/drivers/passthrough/vtd/quirks.c
+++ b/xen/drivers/passthrough/vtd/quirks.c
@@ -385,7 +385,7 @@ void me_wifi_quirk(struct domain *domain
  *   - This can cause system failure upon non-fatal VT-d faults
  *   - Potential security issue if malicious guest trigger VT-d faults
  */
-void __hwdom_init pci_vtd_quirk(struct pci_dev *pdev)
+void pci_vtd_quirk(const struct pci_dev *pdev)
 {
     int seg = pdev->seg;
     int bus = pdev->bus;

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

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

[PATCH 2/2] VT-d: extend error report masking workaround to newer chipsets

[Jan Beulich]

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

Add two more PCI IDs to the set that has been taken care of with a
different workaround long before XSA-59, and (for constency with the
newer workarounds) log a message here too.

Also move the function wide comment to the cases it applies to; this
should really have been done by d061d200 ("VT-d: suppress UR signaling
for server chipsets").

This is CVE-2013-3495 / XSA-59.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/quirks.c
+++ b/xen/drivers/passthrough/vtd/quirks.c
@@ -379,12 +379,6 @@ void me_wifi_quirk(struct domain *domain
     }
 }
 
-/*
- * Mask reporting Intel VT-d faults to IOH core logic:
- *   - Some platform escalates VT-d faults to platform errors 
- *   - This can cause system failure upon non-fatal VT-d faults
- *   - Potential security issue if malicious guest trigger VT-d faults
- */
 void pci_vtd_quirk(const struct pci_dev *pdev)
 {
     int seg = pdev->seg;
@@ -402,10 +396,20 @@ void pci_vtd_quirk(const struct pci_dev 
 
     switch ( pci_conf_read16(seg, bus, dev, func, PCI_DEVICE_ID) )
     {
+    /*
+     * Mask reporting Intel VT-d faults to IOH core logic:
+     *   - Some platform escalates VT-d faults to platform errors.
+     *   - This can cause system failure upon non-fatal VT-d faults.
+     *   - Potential security issue if malicious guest trigger VT-d faults.
+     */
+    case 0x0e28: /* Xeon-E5v2 (IvyBridge) */
     case 0x342e: /* Tylersburg chipset (Nehalem / Westmere systems) */
+    case 0x3728: /* Xeon C5500/C3500 (JasperForest) */
     case 0x3c28: /* Sandybridge */
         val = pci_conf_read32(seg, bus, dev, func, 0x1AC);
         pci_conf_write32(seg, bus, dev, func, 0x1AC, val | (1 << 31));
+        printk(XENLOG_INFO "Masked VT-d error signaling on %04x:%02x:%02x.%u\n",
+               seg, bus, dev, func);
         break;
 
     /* Tylersburg (EP)/Boxboro (MP) chipsets (NHM-EP/EX, WSM-EP/EX) */




[-- Attachment #2: VT-d-mask-error-reporting.patch --]
[-- Type: text/plain, Size: 2026 bytes --]

VT-d: extend error report masking workaround to newer chipsets

Add two more PCI IDs to the set that has been taken care of with a
different workaround long before XSA-59, and (for constency with the
newer workarounds) log a message here too.

Also move the function wide comment to the cases it applies to; this
should really have been done by d061d200 ("VT-d: suppress UR signaling
for server chipsets").

This is CVE-2013-3495 / XSA-59.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/quirks.c
+++ b/xen/drivers/passthrough/vtd/quirks.c
@@ -379,12 +379,6 @@ void me_wifi_quirk(struct domain *domain
     }
 }
 
-/*
- * Mask reporting Intel VT-d faults to IOH core logic:
- *   - Some platform escalates VT-d faults to platform errors 
- *   - This can cause system failure upon non-fatal VT-d faults
- *   - Potential security issue if malicious guest trigger VT-d faults
- */
 void pci_vtd_quirk(const struct pci_dev *pdev)
 {
     int seg = pdev->seg;
@@ -402,10 +396,20 @@ void pci_vtd_quirk(const struct pci_dev 
 
     switch ( pci_conf_read16(seg, bus, dev, func, PCI_DEVICE_ID) )
     {
+    /*
+     * Mask reporting Intel VT-d faults to IOH core logic:
+     *   - Some platform escalates VT-d faults to platform errors.
+     *   - This can cause system failure upon non-fatal VT-d faults.
+     *   - Potential security issue if malicious guest trigger VT-d faults.
+     */
+    case 0x0e28: /* Xeon-E5v2 (IvyBridge) */
     case 0x342e: /* Tylersburg chipset (Nehalem / Westmere systems) */
+    case 0x3728: /* Xeon C5500/C3500 (JasperForest) */
     case 0x3c28: /* Sandybridge */
         val = pci_conf_read32(seg, bus, dev, func, 0x1AC);
         pci_conf_write32(seg, bus, dev, func, 0x1AC, val | (1 << 31));
+        printk(XENLOG_INFO "Masked VT-d error signaling on %04x:%02x:%02x.%u\n",
+               seg, bus, dev, func);
         break;
 
     /* Tylersburg (EP)/Boxboro (MP) chipsets (NHM-EP/EX, WSM-EP/EX) */

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

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

Re: [PATCH 1/2] VT-d: apply quirks at device setup time rather than only at boot

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 3110 bytes --]

On 28/04/14 09:01, Jan Beulich wrote:
> Accessing extended config space may not be possible at boot time, e.g.
> when the memory space used by MMCFG is reserved only via ACPI tables,
> but not in the E820/UEFI memory maps (which we need Dom0 to tell us
> about). Consequently the change here still leaves the issue unaddressed
> for systems where the extended config space remains inaccessible (due
> to firmware bugs, i.e. not properly reserving the address space of
> those regions).
>
> With the respective messages now potentially getting logged more than
> once, we ought to consider whether we should issue them only if we in
> fact were required to do any masking (i.e. if the relevant mask bits
> weren't already set).
>
> This is CVE-2013-3495 / XSA-59.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I would agree that log messages should only be presented if Xen had to
change system state.

Content-wise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -99,7 +99,7 @@ void platform_quirks_init(void);
>  void vtd_ops_preamble_quirk(struct iommu* iommu);
>  void vtd_ops_postamble_quirk(struct iommu* iommu);
>  void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map);
> -void pci_vtd_quirk(struct pci_dev *pdev);
> +void pci_vtd_quirk(const struct pci_dev *);
>  int platform_supports_intremap(void);
>  int platform_supports_x2apic(void);
>  
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1483,6 +1483,9 @@ static int domain_context_mapping(
>          break;
>      }
>  
> +    if ( !ret && devfn == pdev->devfn )
> +        pci_vtd_quirk(pdev);
> +
>      return ret;
>  }
>  
> @@ -1922,6 +1925,8 @@ static int intel_iommu_enable_device(str
>      struct acpi_drhd_unit *drhd = acpi_find_matched_drhd_unit(pdev);
>      int ret = drhd ? ats_device(pdev, drhd) : -ENODEV;
>  
> +    pci_vtd_quirk(pdev);
> +
>      if ( ret <= 0 )
>          return ret;
>  
> @@ -1994,12 +1999,7 @@ static int intel_iommu_remove_device(u8 
>  
>  static int __hwdom_init setup_hwdom_device(u8 devfn, struct pci_dev *pdev)
>  {
> -    int err;
> -
> -    err = domain_context_mapping(pdev->domain, devfn, pdev);
> -    if ( !err && devfn == pdev->devfn )
> -        pci_vtd_quirk(pdev);
> -    return err;
> +    return domain_context_mapping(pdev->domain, devfn, pdev);
>  }
>  
>  void clear_fault_bits(struct iommu *iommu)
> --- a/xen/drivers/passthrough/vtd/quirks.c
> +++ b/xen/drivers/passthrough/vtd/quirks.c
> @@ -385,7 +385,7 @@ void me_wifi_quirk(struct domain *domain
>   *   - This can cause system failure upon non-fatal VT-d faults
>   *   - Potential security issue if malicious guest trigger VT-d faults
>   */
> -void __hwdom_init pci_vtd_quirk(struct pci_dev *pdev)
> +void pci_vtd_quirk(const struct pci_dev *pdev)
>  {
>      int seg = pdev->seg;
>      int bus = pdev->bus;
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 3949 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 2/2] VT-d: extend error report masking workaround to newer chipsets

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2268 bytes --]

On 28/04/14 09:01, Jan Beulich wrote:
> Add two more PCI IDs to the set that has been taken care of with a
> different workaround long before XSA-59, and (for constency with the
> newer workarounds) log a message here too.
>
> Also move the function wide comment to the cases it applies to; this
> should really have been done by d061d200 ("VT-d: suppress UR signaling
> for server chipsets").
>
> This is CVE-2013-3495 / XSA-59.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

As before, this would probably be better being a conditional message.

~Andrew

>
> --- a/xen/drivers/passthrough/vtd/quirks.c
> +++ b/xen/drivers/passthrough/vtd/quirks.c
> @@ -379,12 +379,6 @@ void me_wifi_quirk(struct domain *domain
>      }
>  }
>  
> -/*
> - * Mask reporting Intel VT-d faults to IOH core logic:
> - *   - Some platform escalates VT-d faults to platform errors 
> - *   - This can cause system failure upon non-fatal VT-d faults
> - *   - Potential security issue if malicious guest trigger VT-d faults
> - */
>  void pci_vtd_quirk(const struct pci_dev *pdev)
>  {
>      int seg = pdev->seg;
> @@ -402,10 +396,20 @@ void pci_vtd_quirk(const struct pci_dev 
>  
>      switch ( pci_conf_read16(seg, bus, dev, func, PCI_DEVICE_ID) )
>      {
> +    /*
> +     * Mask reporting Intel VT-d faults to IOH core logic:
> +     *   - Some platform escalates VT-d faults to platform errors.
> +     *   - This can cause system failure upon non-fatal VT-d faults.
> +     *   - Potential security issue if malicious guest trigger VT-d faults.
> +     */
> +    case 0x0e28: /* Xeon-E5v2 (IvyBridge) */
>      case 0x342e: /* Tylersburg chipset (Nehalem / Westmere systems) */
> +    case 0x3728: /* Xeon C5500/C3500 (JasperForest) */
>      case 0x3c28: /* Sandybridge */
>          val = pci_conf_read32(seg, bus, dev, func, 0x1AC);
>          pci_conf_write32(seg, bus, dev, func, 0x1AC, val | (1 << 31));
> +        printk(XENLOG_INFO "Masked VT-d error signaling on %04x:%02x:%02x.%u\n",
> +               seg, bus, dev, func);
>          break;
>  
>      /* Tylersburg (EP)/Boxboro (MP) chipsets (NHM-EP/EX, WSM-EP/EX) */
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 3044 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 1/2] VT-d: apply quirks at device setup time rather than only at boot

[Jan Beulich]

>>> On 28.04.14 at 11:30, <andrew.cooper3@citrix.com> wrote:
> On 28/04/14 09:01, Jan Beulich wrote:
>> Accessing extended config space may not be possible at boot time, e.g.
>> when the memory space used by MMCFG is reserved only via ACPI tables,
>> but not in the E820/UEFI memory maps (which we need Dom0 to tell us
>> about). Consequently the change here still leaves the issue unaddressed
>> for systems where the extended config space remains inaccessible (due
>> to firmware bugs, i.e. not properly reserving the address space of
>> those regions).
>>
>> With the respective messages now potentially getting logged more than
>> once, we ought to consider whether we should issue them only if we in
>> fact were required to do any masking (i.e. if the relevant mask bits
>> weren't already set).
>>
>> This is CVE-2013-3495 / XSA-59.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I would agree that log messages should only be presented if Xen had to
> change system state.

I'll wait for eventual other opinions, but might create a 3rd patch on
top of these then. Just for the record, the downside I see to this is
that then there's no trace left that a system is secure. An intermediate
option might be to downgrade the messages to XENLOG_DEBUG when
we didn't really need to do anything.

Jan

Re: [PATCH 2/2] VT-d: extend error report masking workaround to newer chipsets

[Jan Beulich]

>>> On 28.04.14 at 11:34, <andrew.cooper3@citrix.com> wrote:
> On 28/04/14 09:01, Jan Beulich wrote:
>> Add two more PCI IDs to the set that has been taken care of with a
>> different workaround long before XSA-59, and (for constency with the
>> newer workarounds) log a message here too.
>>
>> Also move the function wide comment to the cases it applies to; this
>> should really have been done by d061d200 ("VT-d: suppress UR signaling
>> for server chipsets").
>>
>> This is CVE-2013-3495 / XSA-59.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> As before, this would probably be better being a conditional message.

In any event I want this to be consistent, i.e. I'd want to keep this
patch as is and put a third one playing with the verbosity on top.

Jan

Re: [PATCH 2/2] VT-d: extend error report masking workaround to newer chipsets

[Andrew Cooper]

On 28/04/14 10:56, Jan Beulich wrote:
>>>> On 28.04.14 at 11:34, <andrew.cooper3@citrix.com> wrote:
>> On 28/04/14 09:01, Jan Beulich wrote:
>>> Add two more PCI IDs to the set that has been taken care of with a
>>> different workaround long before XSA-59, and (for constency with the
>>> newer workarounds) log a message here too.
>>>
>>> Also move the function wide comment to the cases it applies to; this
>>> should really have been done by d061d200 ("VT-d: suppress UR signaling
>>> for server chipsets").
>>>
>>> This is CVE-2013-3495 / XSA-59.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> As before, this would probably be better being a conditional message.
> In any event I want this to be consistent, i.e. I'd want to keep this
> patch as is and put a third one playing with the verbosity on top.
>
> Jan
>

Agreed.  In which case contentwise Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

Re: [PATCH 1/2] VT-d: apply quirks at device setup time rather than only at boot

[Andrew Cooper]

On 28/04/14 10:55, Jan Beulich wrote:
>>>> On 28.04.14 at 11:30, <andrew.cooper3@citrix.com> wrote:
>> On 28/04/14 09:01, Jan Beulich wrote:
>>> Accessing extended config space may not be possible at boot time, e.g.
>>> when the memory space used by MMCFG is reserved only via ACPI tables,
>>> but not in the E820/UEFI memory maps (which we need Dom0 to tell us
>>> about). Consequently the change here still leaves the issue unaddressed
>>> for systems where the extended config space remains inaccessible (due
>>> to firmware bugs, i.e. not properly reserving the address space of
>>> those regions).
>>>
>>> With the respective messages now potentially getting logged more than
>>> once, we ought to consider whether we should issue them only if we in
>>> fact were required to do any masking (i.e. if the relevant mask bits
>>> weren't already set).
>>>
>>> This is CVE-2013-3495 / XSA-59.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> I would agree that log messages should only be presented if Xen had to
>> change system state.
> I'll wait for eventual other opinions, but might create a 3rd patch on
> top of these then. Just for the record, the downside I see to this is
> that then there's no trace left that a system is secure. An intermediate
> option might be to downgrade the messages to XENLOG_DEBUG when
> we didn't really need to do anything.
>
> Jan
>

Hmm yes - that is a point.  It probably is best to leave some hint for
people really trying to find it.

~Andrew

Ping: [PATCH 0/2] VT-d: further XSA-59 workaround adjustments

[Jan Beulich]

>>> On 28.04.14 at 09:54, <JBeulich@suse.com> wrote:
> While doing the backports of the recently committed XSA-59 workaround
> patches, when reaching 4.2 I had to inspect the fuzzy applies resulting
> from the x86-64 conditionals in that code, making me realize that what
> we're doing is still insufficient: We wrongly assume to be able to access
> extended config registers (i.e. MMCFG space) at boot time.
> 
> Fixing that, in turn made me again look at the one workaround that was
> in place in the same function before that recent series, just to find that
> the list very likely should have been extended quite a while back.
> 
> 1: apply quirks at device setup time rather than only at boot
> 2: extend error report masking workaround to newer chipsets
> 
> This (still) is CVE-2013-3495 / XSA-59.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Even if patch 2 may require additional time to be validated (and
ideally may turn out not to be required at all), I would still
appreciate some feedback on patch 1 rather sooner than later.

Jan

Ping II: [PATCH 0/2] VT-d: further XSA-59 workaround adjustments

[Jan Beulich]

>>> On 08.05.14 at 10:07, <JBeulich@suse.com> wrote:
>>>> On 28.04.14 at 09:54, <JBeulich@suse.com> wrote:
>> While doing the backports of the recently committed XSA-59 workaround
>> patches, when reaching 4.2 I had to inspect the fuzzy applies resulting
>> from the x86-64 conditionals in that code, making me realize that what
>> we're doing is still insufficient: We wrongly assume to be able to access
>> extended config registers (i.e. MMCFG space) at boot time.
>> 
>> Fixing that, in turn made me again look at the one workaround that was
>> in place in the same function before that recent series, just to find that
>> the list very likely should have been extended quite a while back.
>> 
>> 1: apply quirks at device setup time rather than only at boot
>> 2: extend error report masking workaround to newer chipsets
>> 
>> This (still) is CVE-2013-3495 / XSA-59.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Even if patch 2 may require additional time to be validated (and
> ideally may turn out not to be required at all), I would still
> appreciate some feedback on patch 1 rather sooner than later.

Xiantao,

another week has passed with no response from you whatsoever.
As the maintainer for VT-d code, I think we can expect you to react
on patches in a half way timely manner. I'll give this a couple more
days, but will assume the absence of any objection on patch 1 if I
don't hear back (I'm willing to give patch 2 some more time for you
to investigate).

Regards, Jan

Re: [PATCH 1/2] VT-d: apply quirks at device setup time rather than only at boot

[Zhang, Xiantao]

Thanks, Acked-by: Xiantao Zhang <xiantao.zhang@intel.com>
Xiantao.

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, April 28, 2014 4:01 PM
> To: xen-devel
> Cc: Dugger, Donald D; Zhang, Xiantao
> Subject: [PATCH 1/2] VT-d: apply quirks at device setup time rather than only at
> boot
> 
> Accessing extended config space may not be possible at boot time, e.g.
> when the memory space used by MMCFG is reserved only via ACPI tables, but
> not in the E820/UEFI memory maps (which we need Dom0 to tell us about).
> Consequently the change here still leaves the issue unaddressed for systems
> where the extended config space remains inaccessible (due to firmware bugs,
> i.e. not properly reserving the address space of those regions).
> 
> With the respective messages now potentially getting logged more than once,
> we ought to consider whether we should issue them only if we in fact were
> required to do any masking (i.e. if the relevant mask bits weren't already set).
> 
> This is CVE-2013-3495 / XSA-59.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -99,7 +99,7 @@ void platform_quirks_init(void);  void
> vtd_ops_preamble_quirk(struct iommu* iommu);  void
> vtd_ops_postamble_quirk(struct iommu* iommu);  void me_wifi_quirk(struct
> domain *domain, u8 bus, u8 devfn, int map); -void pci_vtd_quirk(struct pci_dev
> *pdev);
> +void pci_vtd_quirk(const struct pci_dev *);
>  int platform_supports_intremap(void);
>  int platform_supports_x2apic(void);
> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1483,6 +1483,9 @@ static int domain_context_mapping(
>          break;
>      }
> 
> +    if ( !ret && devfn == pdev->devfn )
> +        pci_vtd_quirk(pdev);
> +
>      return ret;
>  }
> 
> @@ -1922,6 +1925,8 @@ static int intel_iommu_enable_device(str
>      struct acpi_drhd_unit *drhd = acpi_find_matched_drhd_unit(pdev);
>      int ret = drhd ? ats_device(pdev, drhd) : -ENODEV;
> 
> +    pci_vtd_quirk(pdev);
> +
>      if ( ret <= 0 )
>          return ret;
> 
> @@ -1994,12 +1999,7 @@ static int intel_iommu_remove_device(u8
> 
>  static int __hwdom_init setup_hwdom_device(u8 devfn, struct pci_dev *pdev)
> {
> -    int err;
> -
> -    err = domain_context_mapping(pdev->domain, devfn, pdev);
> -    if ( !err && devfn == pdev->devfn )
> -        pci_vtd_quirk(pdev);
> -    return err;
> +    return domain_context_mapping(pdev->domain, devfn, pdev);
>  }
> 
>  void clear_fault_bits(struct iommu *iommu)
> --- a/xen/drivers/passthrough/vtd/quirks.c
> +++ b/xen/drivers/passthrough/vtd/quirks.c
> @@ -385,7 +385,7 @@ void me_wifi_quirk(struct domain *domain
>   *   - This can cause system failure upon non-fatal VT-d faults
>   *   - Potential security issue if malicious guest trigger VT-d faults
>   */
> -void __hwdom_init pci_vtd_quirk(struct pci_dev *pdev)
> +void pci_vtd_quirk(const struct pci_dev *pdev)
>  {
>      int seg = pdev->seg;
>      int bus = pdev->bus;
> 
> 

Re: [PATCH 2/2] VT-d: extend error report masking workaround to newer chipsets

[Zhang, Xiantao]

Thanks, Acked-by: Xiantao Zhang <xiantao.zhang@intel.com>
Xiantao.

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, April 28, 2014 4:02 PM
> To: xen-devel
> Cc: Dugger, Donald D; Zhang, Xiantao
> Subject: [PATCH 2/2] VT-d: extend error report masking workaround to newer
> chipsets
> 
> Add two more PCI IDs to the set that has been taken care of with a different
> workaround long before XSA-59, and (for constency with the newer
> workarounds) log a message here too.
> 
> Also move the function wide comment to the cases it applies to; this should
> really have been done by d061d200 ("VT-d: suppress UR signaling for server
> chipsets").
> 
> This is CVE-2013-3495 / XSA-59.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/drivers/passthrough/vtd/quirks.c
> +++ b/xen/drivers/passthrough/vtd/quirks.c
> @@ -379,12 +379,6 @@ void me_wifi_quirk(struct domain *domain
>      }
>  }
> 
> -/*
> - * Mask reporting Intel VT-d faults to IOH core logic:
> - *   - Some platform escalates VT-d faults to platform errors
> - *   - This can cause system failure upon non-fatal VT-d faults
> - *   - Potential security issue if malicious guest trigger VT-d faults
> - */
>  void pci_vtd_quirk(const struct pci_dev *pdev)  {
>      int seg = pdev->seg;
> @@ -402,10 +396,20 @@ void pci_vtd_quirk(const struct pci_dev
> 
>      switch ( pci_conf_read16(seg, bus, dev, func, PCI_DEVICE_ID) )
>      {
> +    /*
> +     * Mask reporting Intel VT-d faults to IOH core logic:
> +     *   - Some platform escalates VT-d faults to platform errors.
> +     *   - This can cause system failure upon non-fatal VT-d faults.
> +     *   - Potential security issue if malicious guest trigger VT-d faults.
> +     */
> +    case 0x0e28: /* Xeon-E5v2 (IvyBridge) */
>      case 0x342e: /* Tylersburg chipset (Nehalem / Westmere systems) */
> +    case 0x3728: /* Xeon C5500/C3500 (JasperForest) */
>      case 0x3c28: /* Sandybridge */
>          val = pci_conf_read32(seg, bus, dev, func, 0x1AC);
>          pci_conf_write32(seg, bus, dev, func, 0x1AC, val | (1 << 31));
> +        printk(XENLOG_INFO "Masked VT-d error signaling
> on %04x:%02x:%02x.%u\n",
> +               seg, bus, dev, func);
>          break;
> 
>      /* Tylersburg (EP)/Boxboro (MP) chipsets (NHM-EP/EX, WSM-EP/EX) */
> 
> 

Re: [PATCH 1/2] VT-d: apply quirks at device setup time rather than only at boot

[Zhang, Yang Z]

> Accessing extended config space may not be possible at boot time, e.g.
> when the memory space used by MMCFG is reserved only via ACPI tables, but
> not in the E820/UEFI memory maps (which we need Dom0 to tell us about).
> Consequently the change here still leaves the issue unaddressed for systems
> where the extended config space remains inaccessible (due to firmware bugs,
> i.e. not properly reserving the address space of those regions).
> 
> With the respective messages now potentially getting logged more than once,
> we ought to consider whether we should issue them only if we in fact were
> required to do any masking (i.e. if the relevant mask bits weren't already set).
> 
> This is CVE-2013-3495 / XSA-59.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Yang Zhang <yang.z.zhang@intel.com>

> 
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -99,7 +99,7 @@ void platform_quirks_init(void);  void
> vtd_ops_preamble_quirk(struct iommu* iommu);  void
> vtd_ops_postamble_quirk(struct iommu* iommu);  void me_wifi_quirk(struct
> domain *domain, u8 bus, u8 devfn, int map); -void pci_vtd_quirk(struct pci_dev
> *pdev);
> +void pci_vtd_quirk(const struct pci_dev *);
>  int platform_supports_intremap(void);
>  int platform_supports_x2apic(void);
> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1483,6 +1483,9 @@ static int domain_context_mapping(
>          break;
>      }
> 
> +    if ( !ret && devfn == pdev->devfn )
> +        pci_vtd_quirk(pdev);
> +
>      return ret;
>  }
> 
> @@ -1922,6 +1925,8 @@ static int intel_iommu_enable_device(str
>      struct acpi_drhd_unit *drhd = acpi_find_matched_drhd_unit(pdev);
>      int ret = drhd ? ats_device(pdev, drhd) : -ENODEV;
> 
> +    pci_vtd_quirk(pdev);
> +
>      if ( ret <= 0 )
>          return ret;
> 
> @@ -1994,12 +1999,7 @@ static int intel_iommu_remove_device(u8
> 
>  static int __hwdom_init setup_hwdom_device(u8 devfn, struct pci_dev *pdev)
> {
> -    int err;
> -
> -    err = domain_context_mapping(pdev->domain, devfn, pdev);
> -    if ( !err && devfn == pdev->devfn )
> -        pci_vtd_quirk(pdev);
> -    return err;
> +    return domain_context_mapping(pdev->domain, devfn, pdev);
>  }
> 
>  void clear_fault_bits(struct iommu *iommu)
> --- a/xen/drivers/passthrough/vtd/quirks.c
> +++ b/xen/drivers/passthrough/vtd/quirks.c
> @@ -385,7 +385,7 @@ void me_wifi_quirk(struct domain *domain
>   *   - This can cause system failure upon non-fatal VT-d faults
>   *   - Potential security issue if malicious guest trigger VT-d faults
>   */
> -void __hwdom_init pci_vtd_quirk(struct pci_dev *pdev)
> +void pci_vtd_quirk(const struct pci_dev *pdev)
>  {
>      int seg = pdev->seg;
>      int bus = pdev->bus;

Best regards
Yang