question: is it a CVE in relinquish_memory()[xen/arch/x86/domain.c]

Commit Message

Xuquan (Euler) Nov. 9, 2016, 12:01 p.m. UTC

Hi,
Based on CVE-2015-7814 and commit 1ef01396fdff, ' arm: handle races between relinquish_memory and free_domheap_pages'..
relinquish_memory() [xen/arch/arm/domain.c, arm code], 
when couldn't get a reference -- someone is freeing this page and has already committed to doing so, so no more to do here, continue.


But in relinquish_memory()[xen/arch/x86/domain.c, __x86__ code], when couldn't get a reference -- someone is freeing this page,
Why adding this page to d->arch.relmem_list again. 
Is it a CVE to double free page, then hit the ''" alloc_heap_pages() : BUG_ON(pg[i].count_info != PGC_state_free)"" in creating guests later..




~


Quan

Comments

Jan Beulich Nov. 9, 2016, 2:22 p.m. UTC | #1

>>> On 09.11.16 at 13:01, <[e-mail]> wrote:
> Based on CVE-2015-7814 and commit 1ef01396fdff, ' arm: handle races between 
> relinquish_memory and free_domheap_pages'..
> relinquish_memory() [xen/arch/arm/domain.c, arm code], 
> when couldn't get a reference -- someone is freeing this page and has already 
> committed to doing so, so no more to do here, continue.
> 
> 
> But in relinquish_memory()[xen/arch/x86/domain.c, __x86__ code], when 
> couldn't get a reference -- someone is freeing this page,
> Why adding this page to d->arch.relmem_list again. 
> Is it a CVE to double free page, then hit the ''" alloc_heap_pages() : 
> BUG_ON(pg[i].count_info != PGC_state_free)"" in creating guests later..

Well, considering that you've even quoted the description of the
patch, it should be clear to you that the difference in behavior
between ARM and x86 is intended. Hence I'm having difficulty
seeing what you actually want to point out.

And then, if you again suspect a security issue in the future,
please ask on security@ first, rather than posting publicly (on
xen-devel@ or elsewhere).

Thanks, Jan

Patch

========

[1] CVE-2015-7814
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-7814

[2] commit 1ef01396fdff

commit 1ef01396fdff88b1c3331a09ca5c69619b90f4ea
Author: Ian Campbell <[e-mail]>
Date:   Thu Oct 29 13:34:17 2015 +0100

    arm: handle races between relinquish_memory and free_domheap_pages

    Primarily this means XENMEM_decrease_reservation from a toolstack
    domain.

    Unlike x86 we have no requirement right now to queue such pages onto
    a separate list, if we hit this race then the other code has already
    fully accepted responsibility for freeing this page and therefore
    there is no more for relinquish_memory to do.

    This is CVE-2015-7814 / XSA-147.

    Signed-off-by: Ian Campbell <[e-mail]>
    Reviewed-by: Julien Grall <[e-mail]>
    Reviewed-by: Jan Beulich <[e-mail]>

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 683e769..880d0a6 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -772,8 +772,15 @@  static int relinquish_memory(struct domain *d, struct page_list_head *list)
     {
         /* Grab a reference to the page so it won't disappear from under us. */
         if ( unlikely(!get_page(page, d)) )
-            /* Couldn't get a reference -- someone is freeing this page. */
-            BUG();
+            /*
+             * Couldn't get a reference -- someone is freeing this page and
+             * has already committed to doing so, so no more to do here.
+             *
+             * Note that the page must be left on the list, a list_del
+             * here will clash with the list_del done by the other
+             * party in the race and corrupt the list head.
+             */
+            continue;

         if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
             put_page(page);