Xen Security Advisory 360 v1 - IRQ vector leak on x86

Xen Security Advisory 360 v1 - IRQ vector leak on x86

[Xen.org security team]

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

                    Xen Security Advisory XSA-360

                        IRQ vector leak on x86

ISSUE DESCRIPTION
=================

A x86 HVM guest with PCI pass through devices can force the allocation
of all IDT vectors on the system by rebooting itself with MSI or MSI-X
capabilities enabled and entries setup.

Such reboots will leak any vectors used by the MSI(-X) entries that the
guest might had enabled, and hence will lead to vector exhaustion on the
system, not allowing further PCI pass through devices to work properly.

IMPACT
======

HVM guests with PCI pass through devices can mount a Denial of Service (DoS)
attack affecting the pass through of PCI devices to other guests or the
hardware domain.  In the latter case this would affect the entire host.

VULNERABLE SYSTEMS
==================

Xen versions 4.12.3, 4.12.4, and all versions from 4.13.1 onwards are
vulnerable.  Xen version 4.13.0 and all versions up to 4.12.2 are not
affected.

Only x86 systems running HVM guests with PCI pass through devices are
vulnerable.

MITIGATION
==========

Not running HVM guests with PCI pass through devices will avoid the
vulnerability.  Note that even non-malicious guests can trigger this
vulnerability as part of normal operation.

RESOLUTION
==========

Applying the appropriate attached patch resolves this issue.

Note that patches for released versions are generally prepared to
apply to the stable branches, and may not apply cleanly to the most
recent release tarball.  Downstreams are encouraged to update to the
tip of the stable branch before applying these patches.

xsa360.patch           xen-unstable
xsa360-4.14.patch      Xen 4.14 - 4.12

$ sha256sum xsa360*
c874ad2b9edb0791ac975735306d055b1916f4acbc59e6f1550fbf33223d6106  xsa360.meta
592f3afda63777d31844e0e34d85fbe387a62d59fa7903ee19b22a98fba68894  xsa360.patch
809515011efb781a2a8742e9acfd76412d3920c2d4142bb187588cd36f77383e  xsa360-4.14.patch
$

CREDITS
=======

This issue was discovered by James McCoy, debugged in combination with
Samuel Verschelde of Vates, and recognised as a security issue by Roger
Pau Monné of Citrix.

NOTE REGARDING LACK OF EMBARGO
==============================

This was reported and debugged publicly, before the security
implications were apparent.
-----BEGIN PGP SIGNATURE-----

iQFABAEBCAAqFiEEI+MiLBRfRHX6gGCng/4UyVfoK9kFAmAJixQMHHBncEB4ZW4u
b3JnAAoJEIP+FMlX6CvZh4cH/RyA5POGYEJEj4jHUFK+UmT08Bo6igUBMyJSvAJs
T81eb35E2E2I8P35L7q8OOuLIGPWnTXOGPRnwizr2YF7UhmMm/773q5ellShUBgm
SHtYl+btRaAp6gXB1PhgiETN3EH3aRgn89YBAQmg3U4Zb1RUiB2P2x6pVEGjMfBw
Ks3Zj/ElCtfJcBA6xerNNLuqhwamueCEukw5b8eEHnop+y7TuLordpGGMybpQctx
m04lp7zuJDAeshf47wlMQps79Ysx72CaThVKe/9A09z/c2mcR3m+NbieP7PJPggr
n1I6QEaSUuapszkj+lC/L05tiyHdjXkoNAHwtdPr8jKtbKo=
=YdXv
-----END PGP SIGNATURE-----

[-- Attachment #2: xsa360.meta --]
[-- Type: application/octet-stream, Size: 1589 bytes --]

{
  "XSA": 360,
  "SupportedVersions": [
    "master",
    "4.14",
    "4.13",
    "4.12",
    "4.11",
    "4.10"
  ],
  "Trees": [
    "xen"
  ],
  "Recipes": {
    "4.10": {
      "Recipes": {
        "xen": {
          "StableRef": "6ea37c69c7d3948d9bb6f217235ae8bd767e8c46",
          "Prereqs": [],
          "Patches": []
        }
      }
    },
    "4.11": {
      "Recipes": {
        "xen": {
          "StableRef": "310ab79875cb705cc2c7daddff412b5a4899f8c9",
          "Prereqs": [],
          "Patches": []
        }
      }
    },
    "4.12": {
      "Recipes": {
        "xen": {
          "StableRef": "2525a745e18bbf14b4f7b1b18209a0ab9166178d",
          "Prereqs": [
            360
          ],
          "Patches": [
            "xsa360-4.14.patch"
          ]
        }
      }
    },
    "4.13": {
      "Recipes": {
        "xen": {
          "StableRef": "10c7c213bef26274684798deb3e351a6756046d2",
          "Prereqs": [
            360
          ],
          "Patches": [
            "xsa360-4.14.patch"
          ]
        }
      }
    },
    "4.14": {
      "Recipes": {
        "xen": {
          "StableRef": "ad844aa352559a8b1f36e391a27d9d7dbddbdc36",
          "Prereqs": [
            360
          ],
          "Patches": [
            "xsa360-4.14.patch"
          ]
        }
      }
    },
    "master": {
      "Recipes": {
        "xen": {
          "StableRef": "e8adbf680b56a3f4b9600c7bcc04fec1877a6213",
          "Prereqs": [
            360
          ],
          "Patches": [
            "xsa360.patch"
          ]
        }
      }
    }
  }
}

[-- Attachment #3: xsa360.patch --]
[-- Type: application/octet-stream, Size: 3431 bytes --]

From: Roger Pau Monne <roger.pau@citrix.com>
Subject: x86/dpci: do not remove pirqs from domain tree on unbind

A fix for a previous issue removed the pirqs from the domain tree when
they are unbound in order to prevent shared pirqs from triggering a
BUG_ON in __pirq_guest_unbind if they are unbound multiple times. That
caused free_domain_pirqs to no longer unmap the pirqs because they
are gone from the domain pirq tree, thus leaving stale unbound pirqs
after domain destruction if the domain had mapped dpci pirqs after
shutdown.

Take a different approach to fix the original issue, instead of
removing the pirq from d->pirq_tree clear the flags of the dpci pirq
struct to signal that the pirq is now unbound. This prevents calling
pirq_guest_unbind multiple times for the same pirq without having to
remove it from the domain pirq tree.

This is XSA-360.

Fixes: 5b58dad089 ('x86/pass-through: avoid double IRQ unbind during domain cleanup')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes since v1:
 - Do not switch the original BUG to BUG_ON.

--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1344,7 +1344,7 @@ void (pirq_cleanup_check)(struct pirq *pirq, struct domain *d)
     }
 
     if ( radix_tree_delete(&d->pirq_tree, pirq->pirq) != pirq )
-        BUG_ON(!d->is_dying);
+        BUG();
 }
 
 /* Flush all ready EOIs from the top of this CPU's pending-EOI stack. */
--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -1036,6 +1036,10 @@ static int pci_clean_dpci_irq(struct domain *d,
 {
     struct dev_intx_gsi_link *digl, *tmp;
 
+    if ( !pirq_dpci->flags )
+        /* Already processed. */
+        return 0;
+
     pirq_guest_unbind(d, dpci_pirq(pirq_dpci));
 
     if ( pt_irq_need_timer(pirq_dpci->flags) )
@@ -1046,15 +1050,10 @@ static int pci_clean_dpci_irq(struct domain *d,
         list_del(&digl->list);
         xfree(digl);
     }
+    /* Note the pirq is now unbound. */
+    pirq_dpci->flags = 0;
 
-    radix_tree_delete(&d->pirq_tree, dpci_pirq(pirq_dpci)->pirq);
-
-    if ( !pt_pirq_softirq_active(pirq_dpci) )
-        return 0;
-
-    domain_get_irq_dpci(d)->pending_pirq_dpci = pirq_dpci;
-
-    return -ERESTART;
+    return pt_pirq_softirq_active(pirq_dpci) ? -ERESTART : 0;
 }
 
 int arch_pci_clean_pirqs(struct domain *d)
@@ -1071,18 +1070,8 @@ int arch_pci_clean_pirqs(struct domain *d)
     hvm_irq_dpci = domain_get_irq_dpci(d);
     if ( hvm_irq_dpci != NULL )
     {
-        int ret = 0;
-
-        if ( hvm_irq_dpci->pending_pirq_dpci )
-        {
-            if ( pt_pirq_softirq_active(hvm_irq_dpci->pending_pirq_dpci) )
-                 ret = -ERESTART;
-            else
-                 hvm_irq_dpci->pending_pirq_dpci = NULL;
-        }
+        int ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
 
-        if ( !ret )
-            ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
         if ( ret )
         {
             spin_unlock(&d->event_lock);
--- a/xen/include/asm-x86/hvm/irq.h
+++ b/xen/include/asm-x86/hvm/irq.h
@@ -160,8 +160,6 @@ struct hvm_irq_dpci {
     DECLARE_BITMAP(isairq_map, NR_ISAIRQS);
     /* Record of mapped Links */
     uint8_t link_cnt[NR_LINK];
-    /* Clean up: Entry with a softirq invocation pending / in progress. */
-    struct hvm_pirq_dpci *pending_pirq_dpci;
 };
 
 /* Machine IRQ to guest device/intx mapping. */

[-- Attachment #4: xsa360-4.14.patch --]
[-- Type: application/octet-stream, Size: 3318 bytes --]

From: Roger Pau Monne <roger.pau@citrix.com>
Subject: x86/dpci: do not remove pirqs from domain tree on unbind

A fix for a previous issue removed the pirqs from the domain tree when
they are unbound in order to prevent shared pirqs from triggering a
BUG_ON in __pirq_guest_unbind if they are unbound multiple times. That
caused free_domain_pirqs to no longer unmap the pirqs because they
are gone from the domain pirq tree, thus leaving stale unbound pirqs
after domain destruction if the domain had mapped dpci pirqs after
shutdown.

Take a different approach to fix the original issue, instead of
removing the pirq from d->pirq_tree clear the flags of the dpci pirq
struct to signal that the pirq is now unbound. This prevents calling
pirq_guest_unbind multiple times for the same pirq without having to
remove it from the domain pirq tree.

This is XSA-360.

Fixes: 5b58dad089 ('x86/pass-through: avoid double IRQ unbind during domain cleanup')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1331,7 +1331,7 @@ void (pirq_cleanup_check)(struct pirq *p
     }
 
     if ( radix_tree_delete(&d->pirq_tree, pirq->pirq) != pirq )
-        BUG_ON(!d->is_dying);
+        BUG();
 }
 
 /* Flush all ready EOIs from the top of this CPU's pending-EOI stack. */
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -862,6 +862,10 @@ static int pci_clean_dpci_irq(struct dom
 {
     struct dev_intx_gsi_link *digl, *tmp;
 
+    if ( !pirq_dpci->flags )
+        /* Already processed. */
+        return 0;
+
     pirq_guest_unbind(d, dpci_pirq(pirq_dpci));
 
     if ( pt_irq_need_timer(pirq_dpci->flags) )
@@ -872,15 +876,10 @@ static int pci_clean_dpci_irq(struct dom
         list_del(&digl->list);
         xfree(digl);
     }
+    /* Note the pirq is now unbound. */
+    pirq_dpci->flags = 0;
 
-    radix_tree_delete(&d->pirq_tree, dpci_pirq(pirq_dpci)->pirq);
-
-    if ( !pt_pirq_softirq_active(pirq_dpci) )
-        return 0;
-
-    domain_get_irq_dpci(d)->pending_pirq_dpci = pirq_dpci;
-
-    return -ERESTART;
+    return pt_pirq_softirq_active(pirq_dpci) ? -ERESTART : 0;
 }
 
 static int pci_clean_dpci_irqs(struct domain *d)
@@ -897,18 +896,8 @@ static int pci_clean_dpci_irqs(struct do
     hvm_irq_dpci = domain_get_irq_dpci(d);
     if ( hvm_irq_dpci != NULL )
     {
-        int ret = 0;
-
-        if ( hvm_irq_dpci->pending_pirq_dpci )
-        {
-            if ( pt_pirq_softirq_active(hvm_irq_dpci->pending_pirq_dpci) )
-                 ret = -ERESTART;
-            else
-                 hvm_irq_dpci->pending_pirq_dpci = NULL;
-        }
+        int ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
 
-        if ( !ret )
-            ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
         if ( ret )
         {
             spin_unlock(&d->event_lock);
--- a/xen/include/asm-x86/hvm/irq.h
+++ b/xen/include/asm-x86/hvm/irq.h
@@ -160,8 +160,6 @@ struct hvm_irq_dpci {
     DECLARE_BITMAP(isairq_map, NR_ISAIRQS);
     /* Record of mapped Links */
     uint8_t link_cnt[NR_LINK];
-    /* Clean up: Entry with a softirq invocation pending / in progress. */
-    struct hvm_pirq_dpci *pending_pirq_dpci;
 };
 
 /* Machine IRQ to guest device/intx mapping. */

Re: Xen Security Advisory 360 v1 - IRQ vector leak on x86

[Marek Marczykowski-Górecki]

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

On Thu, Jan 21, 2021 at 02:10:48PM +0000, Xen.org security team wrote:
>                     Xen Security Advisory XSA-360
> 
>                         IRQ vector leak on x86
> 
> ISSUE DESCRIPTION
> =================
> 
> A x86 HVM guest with PCI pass through devices can force the allocation
> of all IDT vectors on the system by rebooting itself with MSI or MSI-X
> capabilities enabled and entries setup.

(...)

> MITIGATION
> ==========
> 
> Not running HVM guests with PCI pass through devices will avoid the
> vulnerability.  Note that even non-malicious guests can trigger this
> vulnerability as part of normal operation.

Does the 'on_reboot="destroy"' mitigate the issue too? Or on_soft_reset?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Xen Security Advisory 360 v1 - IRQ vector leak on x86

[Jan Beulich]

On 21.01.2021 15:20, Marek Marczykowski-Górecki wrote:
> On Thu, Jan 21, 2021 at 02:10:48PM +0000, Xen.org security team wrote:
>> MITIGATION
>> ==========
>>
>> Not running HVM guests with PCI pass through devices will avoid the
>> vulnerability.  Note that even non-malicious guests can trigger this
>> vulnerability as part of normal operation.
> 
> Does the 'on_reboot="destroy"' mitigate the issue too? Or on_soft_reset?

I don't think so, no.

Jan

Re: Xen Security Advisory 360 v1 - IRQ vector leak on x86

[Roger Pau Monné]

On Thu, Jan 21, 2021 at 03:20:12PM +0100, Marek Marczykowski-Górecki wrote:
> On Thu, Jan 21, 2021 at 02:10:48PM +0000, Xen.org security team wrote:
> >                     Xen Security Advisory XSA-360
> > 
> >                         IRQ vector leak on x86
> > 
> > ISSUE DESCRIPTION
> > =================
> > 
> > A x86 HVM guest with PCI pass through devices can force the allocation
> > of all IDT vectors on the system by rebooting itself with MSI or MSI-X
> > capabilities enabled and entries setup.
> 
> (...)
> 
> > MITIGATION
> > ==========
> > 
> > Not running HVM guests with PCI pass through devices will avoid the
> > vulnerability.  Note that even non-malicious guests can trigger this
> > vulnerability as part of normal operation.
> 
> Does the 'on_reboot="destroy"' mitigate the issue too? Or on_soft_reset?

Kind of. Note you will still leak the in use vectors when the guest is
destroyed, but that would prevent the guest from entering a reboot
loop and exhausting all vectors on the system unless the admin starts
it again.

In that case I think the premise of a guest 'rebooting itself' doesn't
apply anymore, since the guest won't be able to perform such
operation.

Roger.

Re: Xen Security Advisory 360 v1 - IRQ vector leak on x86

[Jan Beulich]

On 21.01.2021 15:34, Roger Pau Monné wrote:
> On Thu, Jan 21, 2021 at 03:20:12PM +0100, Marek Marczykowski-Górecki wrote:
>> On Thu, Jan 21, 2021 at 02:10:48PM +0000, Xen.org security team wrote:
>>>                     Xen Security Advisory XSA-360
>>>
>>>                         IRQ vector leak on x86
>>>
>>> ISSUE DESCRIPTION
>>> =================
>>>
>>> A x86 HVM guest with PCI pass through devices can force the allocation
>>> of all IDT vectors on the system by rebooting itself with MSI or MSI-X
>>> capabilities enabled and entries setup.
>>
>> (...)
>>
>>> MITIGATION
>>> ==========
>>>
>>> Not running HVM guests with PCI pass through devices will avoid the
>>> vulnerability.  Note that even non-malicious guests can trigger this
>>> vulnerability as part of normal operation.
>>
>> Does the 'on_reboot="destroy"' mitigate the issue too? Or on_soft_reset?
> 
> Kind of. Note you will still leak the in use vectors when the guest is
> destroyed, but that would prevent the guest from entering a reboot
> loop and exhausting all vectors on the system unless the admin starts
> it again.
> 
> In that case I think the premise of a guest 'rebooting itself' doesn't
> apply anymore, since the guest won't be able to perform such
> operation.

And how exactly would an admin tell a guest from rebooting for
fair reasons from one rebooting for malicious reasons? To me,
setting 'on_reboot="destroy"' would imply there's then some
other mechanism to restart the guest (possibly with some delay),
or else a reboot attempt by this guest would effectively be a
DoS to its users.

Jan

Re: Xen Security Advisory 360 v1 - IRQ vector leak on x86

[Roger Pau Monné]

On Thu, Jan 21, 2021 at 03:50:55PM +0100, Jan Beulich wrote:
> On 21.01.2021 15:34, Roger Pau Monné wrote:
> > On Thu, Jan 21, 2021 at 03:20:12PM +0100, Marek Marczykowski-Górecki wrote:
> >> On Thu, Jan 21, 2021 at 02:10:48PM +0000, Xen.org security team wrote:
> >>>                     Xen Security Advisory XSA-360
> >>>
> >>>                         IRQ vector leak on x86
> >>>
> >>> ISSUE DESCRIPTION
> >>> =================
> >>>
> >>> A x86 HVM guest with PCI pass through devices can force the allocation
> >>> of all IDT vectors on the system by rebooting itself with MSI or MSI-X
> >>> capabilities enabled and entries setup.
> >>
> >> (...)
> >>
> >>> MITIGATION
> >>> ==========
> >>>
> >>> Not running HVM guests with PCI pass through devices will avoid the
> >>> vulnerability.  Note that even non-malicious guests can trigger this
> >>> vulnerability as part of normal operation.
> >>
> >> Does the 'on_reboot="destroy"' mitigate the issue too? Or on_soft_reset?
> > 
> > Kind of. Note you will still leak the in use vectors when the guest is
> > destroyed, but that would prevent the guest from entering a reboot
> > loop and exhausting all vectors on the system unless the admin starts
> > it again.
> > 
> > In that case I think the premise of a guest 'rebooting itself' doesn't
> > apply anymore, since the guest won't be able to perform such
> > operation.
> 
> And how exactly would an admin tell a guest from rebooting for
> fair reasons from one rebooting for malicious reasons? To me,
> setting 'on_reboot="destroy"' would imply there's then some
> other mechanism to restart the guest (possibly with some delay),
> or else a reboot attempt by this guest would effectively be a
> DoS to its users.

Well, I would expect there are deployments or configurations that
simply don't expect (some) domains to reboot themselves. Ie: for
example you won't expect driver domains to restart themselves I think,
and hence you could safely use on_reboot="destroy" in that case to
mitigate a compromised driver domain from exploiting this
vulnerability.

Roger.

Re: Xen Security Advisory 360 v1 - IRQ vector leak on x86

[Jan Beulich]

On 21.01.2021 16:05, Roger Pau Monné wrote:
> On Thu, Jan 21, 2021 at 03:50:55PM +0100, Jan Beulich wrote:
>> On 21.01.2021 15:34, Roger Pau Monné wrote:
>>> On Thu, Jan 21, 2021 at 03:20:12PM +0100, Marek Marczykowski-Górecki wrote:
>>>> On Thu, Jan 21, 2021 at 02:10:48PM +0000, Xen.org security team wrote:
>>>>>                     Xen Security Advisory XSA-360
>>>>>
>>>>>                         IRQ vector leak on x86
>>>>>
>>>>> ISSUE DESCRIPTION
>>>>> =================
>>>>>
>>>>> A x86 HVM guest with PCI pass through devices can force the allocation
>>>>> of all IDT vectors on the system by rebooting itself with MSI or MSI-X
>>>>> capabilities enabled and entries setup.
>>>>
>>>> (...)
>>>>
>>>>> MITIGATION
>>>>> ==========
>>>>>
>>>>> Not running HVM guests with PCI pass through devices will avoid the
>>>>> vulnerability.  Note that even non-malicious guests can trigger this
>>>>> vulnerability as part of normal operation.
>>>>
>>>> Does the 'on_reboot="destroy"' mitigate the issue too? Or on_soft_reset?
>>>
>>> Kind of. Note you will still leak the in use vectors when the guest is
>>> destroyed, but that would prevent the guest from entering a reboot
>>> loop and exhausting all vectors on the system unless the admin starts
>>> it again.
>>>
>>> In that case I think the premise of a guest 'rebooting itself' doesn't
>>> apply anymore, since the guest won't be able to perform such
>>> operation.
>>
>> And how exactly would an admin tell a guest from rebooting for
>> fair reasons from one rebooting for malicious reasons? To me,
>> setting 'on_reboot="destroy"' would imply there's then some
>> other mechanism to restart the guest (possibly with some delay),
>> or else a reboot attempt by this guest would effectively be a
>> DoS to its users.
> 
> Well, I would expect there are deployments or configurations that
> simply don't expect (some) domains to reboot themselves. Ie: for
> example you won't expect driver domains to restart themselves I think,
> and hence you could safely use on_reboot="destroy" in that case to
> mitigate a compromised driver domain from exploiting this
> vulnerability.

Otoh a driver domain may warrant 'oncrash="restart"', to limit
downtime of depending domains. Or, like Xen does by default, a
driver domain may invoke its own restart when crashed.

Jan