Re: [Xen-devel] [PATCH] x86/mm: Take the p2m lock even in shadow mode.

[Andres Lagar-Cavilla <andreslc@xxxxxxxxxxxxxx>]

The reworking of p2m lookups to use get_gfn()/put_gfn() left the
shadow code not taking the p2m lock, even in cases where the p2m would
be updated (i.e. PoD).

In many cases, shadow code doesn't need the exclusion that
get_gfn()/put_gfn() provides, as it has its own interlocks against p2m
updates, but this is taking things too far, and can lead to crashes in
the PoD code.

Now that most shadow-code p2m lookups are done with explicitly
unlocked accessors, or with the get_page_from_gfn() accessor, which is
often lock-free, we can just turn this locking on.

For the record: this includes the page table walking that both the emulate mapping callback and page fault handler need to do.

The remaining locked lookups are in sh_page_fault() (in a path that's
almost always already serializing on the paging lock), and in
emulate_map_dest() (which can probably be updated to use
get_page_from_gfn()).

That is absolutely the case. Here is a rough patch:

diff -r 7324955b35ad xen/arch/x86/mm/shadow/multi.c

--- a/xen/arch/x86/mm/shadow/multi.c

+++ b/xen/arch/x86/mm/shadow/multi.c

@@ -4861,6 +4861,7 @@ static mfn_t emulate_gva_to_mfn(struct v

                                 struct sh_emulate_ctxt *sh_ctxt)

 {

     unsigned long gfn;

+    struct page_info *page;

     mfn_t mfn;

     p2m_type_t p2mt;

     uint32_t pfec = PFEC_page_present | PFEC_write_access;

@@ -4878,22 +4879,30 @@ static mfn_t emulate_gva_to_mfn(struct v

 

     /* Translate the GFN to an MFN */

     ASSERT(!paging_locked_by_me(v->domain));

-    mfn = get_gfn(v->domain, _gfn(gfn), &p2mt);

-        

+    page = get_page_from_gfn(v->domain, gfn, &p2mt, P2M_ALLOC);

+

+    /* Sanity checking */

+    if ( page == NULL )

+    {

+        return _mfn(BAD_GFN_TO_MFN);

+    }

     if ( p2m_is_readonly(p2mt) )

     {

-        put_gfn(v->domain, gfn);

+        put_page(page);

         return _mfn(READONLY_GFN);

     }

     if ( !p2m_is_ram(p2mt) )

     {

-        put_gfn(v->domain, gfn);

+        put_page(page);

         return _mfn(BAD_GFN_TO_MFN);

     }

-

+    mfn = page_to_mfn(page);

     ASSERT(mfn_valid(mfn));

+

     v->arch.paging.last_write_was_pt = !!sh_mfn_is_a_page_table(mfn);

-    put_gfn(v->domain, gfn);

+    /* Note shadow cannot page out or unshare this mfn, so the map won't

+     * disappear. Otherwise, caller must hold onto page until done. */

+    put_page(page);

     return mfn;

 }

 

 They're not addressed here but may be in a
follow-up patch.

Acked-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>

or Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>? see http://lists.xen.org/archives/html/xen-devel/2012-04/msg00962.html

Thanks

Andres

Signed-off-by: Tim Deegan <tim@xxxxxxx>
---
xen/arch/x86/mm/p2m.c |    6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index de1dd82..2db73c9 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -218,8 +218,7 @@ mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn,
        return _mfn(gfn);
    }

-    /* For now only perform locking on hap domains */
-    if ( locked && (hap_enabled(p2m->domain)) )
+    if ( locked )
        /* Grab the lock here, don't release until put_gfn */
        gfn_lock(p2m, gfn, 0);

@@ -248,8 +247,7 @@ mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn,

void __put_gfn(struct p2m_domain *p2m, unsigned long gfn)
{
-    if ( !p2m || !paging_mode_translate(p2m->domain)
-              || !hap_enabled(p2m->domain) )
+    if ( !p2m || !paging_mode_translate(p2m->domain) )
        /* Nothing to do in this case */
        return;

--
1.7.10.4




------------------------------

Message: 2
Date: Thu, 21 Feb 2013 14:22:51 +0000
From: Tim Deegan <tim@xxxxxxx>
To: Olaf Hering <olaf@xxxxxxxxx>
Cc: Eddie Dong <eddie.dong@xxxxxxxxx>, Jun Nakajima
<jun.nakajima@xxxxxxxxx>, xen-devel@xxxxxxxxxxxxx
Subject: Re: [Xen-devel] crash in nvmx_vcpu_destroy
Message-ID: <20130221142251.GK24051@xxxxxxxxxxxxxxxxxxxxx>
Content-Type: text/plain; charset=iso-8859-1

At 15:19 +0100 on 21 Feb (1361459959), Olaf Hering wrote:

This patch fixes the crash for me. Thanks.

Great - in that case it will be fixed by Jan's more comprehensive patch
when that goes in.

Cheers,

Tim.



------------------------------

Message: 3
Date: Thu, 21 Feb 2013 14:23:31 +0000
From: Xen.org security team <security@xxxxxxx>
To: xen-announce@xxxxxxxxxxxxx, xen-devel@xxxxxxxxxxxxx,
xen-users@xxxxxxxxxxxxx, oss-security@xxxxxxxxxxxxxxxxxx
Cc: "Xen.org security team" <security@xxxxxxx>
Subject: [Xen-devel] Xen Security Advisory 36 (CVE-2013-0153) -
interrupt remap entries shared and old ones not cleared on AMD IOMMUs
Message-ID: <E1U8X3b-0007ni-Cw@xxxxxxxxxxxxxxx>
Content-Type: text/plain; charset="utf-8"

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

    Xen Security Advisory CVE-2013-0153 / XSA-36
     version 4

 interrupt remap entries shared and old ones not cleared on AMD IOMMUs

UPDATES IN VERSION 4
====================

Updated patches, to deal with a boot time crash resulting from the earlier
changes on systems with firmware broken in a way not previously accounted
for.

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

To avoid an erratum in early hardware, the Xen AMD IOMMU code by
default chooses to use a single interrupt remapping table for the
whole system.  This sharing implies that any guest with a passed
through PCI device that is bus mastering capable can inject interrupts
into other guests, including domain 0.

Furthermore, regardless of whether a shared interrupt remapping table
is in use, old entries are not always cleared, providing opportunities
(which accumulate over time) for guests to inject interrupts into
other guests, again including domain 0.

In a typical Xen system many devices are owned by domain 0 or driver
domains, leaving them vulnerable to such an attack. Such a DoS is
likely to have an impact on other guests running in the system.

IMPACT
======

A malicious domain which is given access to a physical PCI device can
mount a denial of service attack affecting the whole system.

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

Xen versions 3.3 onwards are vulnerable.  Earlier Xen versions do not
implement interrupt remapping, and hence do not support secure AMD-Vi
PCI passthrough in any case.

Only systems using AMD-Vi for PCI passthrough are vulnerable.

Any domain which is given access to a PCI device can take advantage of
this vulnerability.

MITIGATION
==========

This issue can be avoided by not assigning PCI devices to untrusted
guests.

In Xen versions 4.1.3 and above the sharing of the interrupt remapping
table (and hence the more severe part of this problem) can be avoided
by passing "iommu=amd-iommu-perdev-intremap" as a command line option
to the hypervisor.  This option is not fully functional on earlier
hypervisors.

RESOLUTION
==========

Applying the appropriate attached patch resolves this issue.

Note that on certain systems (SP5100 chipsets with erratum 28 present,
or such with broken IVRS ACPI table) these patches will result in the
IOMMU not being enabled anymore.  This should be dealt with by a BIOS
update, if available.  Alternatively the check can be overridden by
specifying "iommu=no-amd-iommu-perdev-intremap" on the Xen command
line ("iommu=amd-iommu-global-intremap" on 4.1.x), at the price of
re-opening the security hole addressed by these patches.

xsa36-unstable.patch              Xen unstable
xsa36-4.2.patch                   Xen 4.2.x
xsa36-4.1.patch                   Xen 4.1.x

$ sha256sum xsa36*.patch
4bdc0f1f94f82c6bc6c777971f22ef915215b72b98b29f9064e4df65c0efc6f4  xsa36-4.1.patch
dd32ecaa84edbf6d11241045f40ba53ec4a3bc6c24f719bc21204067c4eb8964  xsa36-4.2.patch
7c0b3a1b332a24a830c7a436b065943f60c54cd5b7e746c440e2992a7b5cfe41  xsa36-unstable.patch
$

Incremental patches on top of what was provided in version 3 can also be
taken from the respective mercurial trees:

http://xenbits.xen.org/hg/xen-unstable.hg/rev/e68f14b9e739
http://xenbits.xen.org/hg/staging/xen-4.2-testing.hg/rev/6a03b38b9cd6
http://xenbits.xen.org/hg/staging/xen-4.1-testing.hg/rev/4d522221fa77
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iQEcBAEBAgAGBQJRJf98AAoJEIP+FMlX6CvZ5ocH/jNY92kLw7BOencxa9R3TGTn
20O0+j1id+xi2vjVVF2xm2SJ7g/6Egx5WURUfy2cu+I8GdDHKmRrp3Vkazltzcnd
6AlI5aiPC2H1rFkU0FpneRk3mrluABLZO8Q5YcSJs24hwqded0W+SivH63aInki/
PsDGoBu8HUjYMWjXyqCJVJIGToLS9ApaQ8+iTylWb1ZocRm2VcPS8yJI7z82kj3A
zRNADG36oAFawSJsE9z3ykVoYv9UYckOaWkaXh7jZPHAvIjvP2wLb9gmMkMXbIOP
ICpJJFf0w7oW6KTY3g9n8CxUMBMoUw/9Fv+CQBzOf0ZZY/vIE8q65A0NhCcWixo=
=vmpB
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: xsa36-4.1.patch
Type: application/octet-stream
Size: 14403 bytes
Desc: not available
URL: <http://lists.xen.org/archives/html/xen-devel/attachments/20130221/02b3643c/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: xsa36-4.2.patch
Type: application/octet-stream
Size: 12586 bytes
Desc: not available
URL: <http://lists.xen.org/archives/html/xen-devel/attachments/20130221/02b3643c/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: xsa36-unstable.patch
Type: application/octet-stream
Size: 12528 bytes
Desc: not available
URL: <http://lists.xen.org/archives/html/xen-devel/attachments/20130221/02b3643c/attachment-0002.obj>

------------------------------

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


End of Xen-devel Digest, Vol 96, Issue 309
******************************************

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