[v2] x86/mm: Short circuit damage from "fishy" ref/typecount failure

[PATCH v2] x86/mm: Short circuit damage from "fishy" ref/typecount failure

Posted by Andrew Cooper 3 years, 3 months ago

This code has been copied in 3 places, but it is problematic.

All cases will hit a BUG() later in domain teardown, when a the missing
type/count reference is underflowed.

Don't complicated the logic by leaving a totally unqualified domain crash, and
a timebomb which will be triggered by the toolstack at a slightly later, and
seemingly unrelated, point.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Paul Durrant <paul@xen.org>
CC: Tamas K Lengyel <tamas@tklengyel.com>

v2:
 * Reword the commit message.
 * Switch BUG() to BUG_ON() to further reduce code volume.
---
 xen/arch/x86/hvm/ioreq.c     | 11 ++---------
 xen/arch/x86/hvm/vmx/vmx.c   | 11 ++---------
 xen/arch/x86/mm/mem_paging.c | 17 ++++-------------
 3 files changed, 8 insertions(+), 31 deletions(-)

diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index 1cc27df87f..0c38cfa151 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -366,15 +366,8 @@ static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf)
     if ( !page )
         return -ENOMEM;
 
-    if ( !get_page_and_type(page, s->target, PGT_writable_page) )
-    {
-        /*
-         * The domain can't possibly know about this page yet, so failure
-         * here is a clear indication of something fishy going on.
-         */
-        domain_crash(s->emulator);
-        return -ENODATA;
-    }
+    /* Domain can't know about this page yet - something fishy going on. */
+    BUG_ON(!get_page_and_type(page, s->target, PGT_writable_page));
 
     iorp->va = __map_domain_page_global(page);
     if ( !iorp->va )
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 2d4475ee3d..8e438cb781 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3042,15 +3042,8 @@ static int vmx_alloc_vlapic_mapping(struct domain *d)
     if ( !pg )
         return -ENOMEM;
 
-    if ( !get_page_and_type(pg, d, PGT_writable_page) )
-    {
-        /*
-         * The domain can't possibly know about this page yet, so failure
-         * here is a clear indication of something fishy going on.
-         */
-        domain_crash(d);
-        return -ENODATA;
-    }
+    /* Domain can't know about this page yet - something fishy going on. */
+    BUG_ON(!get_page_and_type(page, s->target, PGT_writable_page));
 
     mfn = page_to_mfn(pg);
     clear_domain_page(mfn);
diff --git a/xen/arch/x86/mm/mem_paging.c b/xen/arch/x86/mm/mem_paging.c
index 01281f786e..6e90019e76 100644
--- a/xen/arch/x86/mm/mem_paging.c
+++ b/xen/arch/x86/mm/mem_paging.c
@@ -379,19 +379,10 @@ static int prepare(struct domain *d, gfn_t gfn,
         page = alloc_domheap_page(d, 0);
         if ( unlikely(page == NULL) )
             goto out;
-        if ( unlikely(!get_page(page, d)) )
-        {
-            /*
-             * The domain can't possibly know about this page yet, so failure
-             * here is a clear indication of something fishy going on.
-             */
-            gprintk(XENLOG_ERR,
-                    "%pd: fresh page for GFN %"PRI_gfn" in unexpected state\n",
-                    d, gfn_x(gfn));
-            domain_crash(d);
-            page = NULL;
-            goto out;
-        }
+
+        /* Domain can't know about this page yet - something fishy going on. */
+        BUG_ON(!get_page(page, s->target));
+
         mfn = page_to_mfn(page);
         page_extant = 0;
 
-- 
2.11.0

RE: [PATCH v2] x86/mm: Short circuit damage from "fishy" ref/typecount failure

Posted by Paul Durrant 3 years, 3 months ago

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 19 January 2021 12:28
> To: Xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>; Roger Pau Monné
> <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>; Tamas K Lengyel
> <tamas@tklengyel.com>
> Subject: [PATCH v2] x86/mm: Short circuit damage from "fishy" ref/typecount failure
> 
> This code has been copied in 3 places, but it is problematic.
> 
> All cases will hit a BUG() later in domain teardown, when a the missing
> type/count reference is underflowed.
> 
> Don't complicated the logic by leaving a totally unqualified domain crash, and
> a timebomb which will be triggered by the toolstack at a slightly later, and
> seemingly unrelated, point.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Paul Durrant <paul@xen.org>
> CC: Tamas K Lengyel <tamas@tklengyel.com>
> 
> v2:
>  * Reword the commit message.
>  * Switch BUG() to BUG_ON() to further reduce code volume.
> ---
>  xen/arch/x86/hvm/ioreq.c     | 11 ++---------
>  xen/arch/x86/hvm/vmx/vmx.c   | 11 ++---------
>  xen/arch/x86/mm/mem_paging.c | 17 ++++-------------
>  3 files changed, 8 insertions(+), 31 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index 1cc27df87f..0c38cfa151 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -366,15 +366,8 @@ static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf)
>      if ( !page )
>          return -ENOMEM;
> 
> -    if ( !get_page_and_type(page, s->target, PGT_writable_page) )
> -    {
> -        /*
> -         * The domain can't possibly know about this page yet, so failure
> -         * here is a clear indication of something fishy going on.
> -         */
> -        domain_crash(s->emulator);
> -        return -ENODATA;
> -    }
> +    /* Domain can't know about this page yet - something fishy going on. */
> +    BUG_ON(!get_page_and_type(page, s->target, PGT_writable_page));
> 
>      iorp->va = __map_domain_page_global(page);
>      if ( !iorp->va )
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 2d4475ee3d..8e438cb781 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3042,15 +3042,8 @@ static int vmx_alloc_vlapic_mapping(struct domain *d)
>      if ( !pg )
>          return -ENOMEM;
> 
> -    if ( !get_page_and_type(pg, d, PGT_writable_page) )
> -    {
> -        /*
> -         * The domain can't possibly know about this page yet, so failure
> -         * here is a clear indication of something fishy going on.
> -         */
> -        domain_crash(d);
> -        return -ENODATA;
> -    }
> +    /* Domain can't know about this page yet - something fishy going on. */
> +    BUG_ON(!get_page_and_type(page, s->target, PGT_writable_page));

Does this compile?

s/page/pg
s/s->target/d

...and similar below is needed AFAICT.

  Paul

> 
>      mfn = page_to_mfn(pg);
>      clear_domain_page(mfn);
> diff --git a/xen/arch/x86/mm/mem_paging.c b/xen/arch/x86/mm/mem_paging.c
> index 01281f786e..6e90019e76 100644
> --- a/xen/arch/x86/mm/mem_paging.c
> +++ b/xen/arch/x86/mm/mem_paging.c
> @@ -379,19 +379,10 @@ static int prepare(struct domain *d, gfn_t gfn,
>          page = alloc_domheap_page(d, 0);
>          if ( unlikely(page == NULL) )
>              goto out;
> -        if ( unlikely(!get_page(page, d)) )
> -        {
> -            /*
> -             * The domain can't possibly know about this page yet, so failure
> -             * here is a clear indication of something fishy going on.
> -             */
> -            gprintk(XENLOG_ERR,
> -                    "%pd: fresh page for GFN %"PRI_gfn" in unexpected state\n",
> -                    d, gfn_x(gfn));
> -            domain_crash(d);
> -            page = NULL;
> -            goto out;
> -        }
> +
> +        /* Domain can't know about this page yet - something fishy going on. */
> +        BUG_ON(!get_page(page, s->target));
> +
>          mfn = page_to_mfn(page);
>          page_extant = 0;
> 
> --
> 2.11.0

Re: [PATCH v2] x86/mm: Short circuit damage from "fishy" ref/typecount failure

Posted by Andrew Cooper 3 years, 3 months ago

On 19/01/2021 12:45, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper <andrew.cooper3@citrix.com>
>> Sent: 19 January 2021 12:28
>> To: Xen-devel <xen-devel@lists.xenproject.org>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>; Roger Pau Monné
>> <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>; Tamas K Lengyel
>> <tamas@tklengyel.com>
>> Subject: [PATCH v2] x86/mm: Short circuit damage from "fishy" ref/typecount failure
>>
>> This code has been copied in 3 places, but it is problematic.
>>
>> All cases will hit a BUG() later in domain teardown, when a the missing
>> type/count reference is underflowed.
>>
>> Don't complicated the logic by leaving a totally unqualified domain crash, and
>> a timebomb which will be triggered by the toolstack at a slightly later, and
>> seemingly unrelated, point.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Paul Durrant <paul@xen.org>
>> CC: Tamas K Lengyel <tamas@tklengyel.com>
>>
>> v2:
>>  * Reword the commit message.
>>  * Switch BUG() to BUG_ON() to further reduce code volume.
>> ---
>>  xen/arch/x86/hvm/ioreq.c     | 11 ++---------
>>  xen/arch/x86/hvm/vmx/vmx.c   | 11 ++---------
>>  xen/arch/x86/mm/mem_paging.c | 17 ++++-------------
>>  3 files changed, 8 insertions(+), 31 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
>> index 1cc27df87f..0c38cfa151 100644
>> --- a/xen/arch/x86/hvm/ioreq.c
>> +++ b/xen/arch/x86/hvm/ioreq.c
>> @@ -366,15 +366,8 @@ static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf)
>>      if ( !page )
>>          return -ENOMEM;
>>
>> -    if ( !get_page_and_type(page, s->target, PGT_writable_page) )
>> -    {
>> -        /*
>> -         * The domain can't possibly know about this page yet, so failure
>> -         * here is a clear indication of something fishy going on.
>> -         */
>> -        domain_crash(s->emulator);
>> -        return -ENODATA;
>> -    }
>> +    /* Domain can't know about this page yet - something fishy going on. */
>> +    BUG_ON(!get_page_and_type(page, s->target, PGT_writable_page));
>>
>>      iorp->va = __map_domain_page_global(page);
>>      if ( !iorp->va )
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index 2d4475ee3d..8e438cb781 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -3042,15 +3042,8 @@ static int vmx_alloc_vlapic_mapping(struct domain *d)
>>      if ( !pg )
>>          return -ENOMEM;
>>
>> -    if ( !get_page_and_type(pg, d, PGT_writable_page) )
>> -    {
>> -        /*
>> -         * The domain can't possibly know about this page yet, so failure
>> -         * here is a clear indication of something fishy going on.
>> -         */
>> -        domain_crash(d);
>> -        return -ENODATA;
>> -    }
>> +    /* Domain can't know about this page yet - something fishy going on. */
>> +    BUG_ON(!get_page_and_type(page, s->target, PGT_writable_page));
> Does this compile?
>
> s/page/pg
> s/s->target/d
>
> ...and similar below is needed AFAICT.

Urgh no - missing the delta in my working tree.  Fixed both.

~Andrew