Re: [Xen-devel] [PATCH v2 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Apr 15, 2019 at 11:28 AM George Dunlap <George.Dunlap@xxxxxxxxxx> wrote:
>
>
>
> > On Apr 15, 2019, at 4:31 PM, Tamas K Lengyel <tamas@xxxxxxxxxxxxx> wrote:
> >
> > On Mon, Apr 15, 2019 at 9:21 AM George Dunlap <george.dunlap@xxxxxxxxxx> 
> > wrote:
> >>
> >> On 4/12/19 8:47 PM, Tamas K Lengyel wrote:
> >>> Patch 0502e0adae2 "x86: correct instances of PGC_allocated clearing" 
> >>> introduced
> >>> grabbing extra references for pages that drop references tied to 
> >>> PGC_allocated.
> >>> However, the way these extra references were grabbed were incorrect, 
> >>> resulting
> >>> in both share_pages and unshare_pages failing.
> >>
> >> Why?  Is cd not the owner of the page?
> >
> > It's not, dom_cow is.
> >
> >>> @@ -1002,7 +994,10 @@ static int share_pages(struct domain *sd, gfn_t 
> >>> sgfn, shr_handle_t sh,
> >>>     /* Free the client page */
> >>>     if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
> >>>         put_page(cpage);
> >>> -    put_page(cpage);
> >>> +
> >>> +    ASSERT(put_count);
> >>> +    while ( put_count-- )
> >>> +        put_page_and_type(cpage);
> >>
> >> I don't think this is in the spirit of the fix in 0502e0adae2; the idea
> >> there seems to have been to make sure that cpage belongs to the right
> >> domain, and that the ownership doesn't change.  If such a race / mistake
> >> were possible before that change, such a race / mistake would be
> >> possible after this change, wouldn't it?
> >
> > I don't have an answer to your question. AFAIU this is just to ensure
> > that the page isn't dropped before test_and_clear_bit is used on the
> > page.
>
> Right, but why would that be a bad thing?
>
> I think it’s to avoid races like this:
>
> P1: Gets pointer to page A, owned by domain M
> P2: Gets pointer to page A, owned by domain M
> P2: test_and_clear_bit(PGC_allocated) succeeds
> P2: put_page()
>   -> page freed
>   -> Page re-allocated to domain N
> P1: test_and_clear_bit(PGC_allocated) succeeds
> P1: put_page()
>   -> page freed ##
>
> Now P1 incorrectly “frees” a page owned by domain N.
>
> If both P1 and P2 call get_page(A, M) first, and drop that reference 
> afterwards, this race can’t happen.
>
> Jan, you’re the author of that patch — is that not the case?
>
> Could a similar race to the above occur in this code?  It looks like it could 
> have before 0502e0adae2 for sure, since all references and locks were 
> dropped.  It’s not clear to me whether mem_sharing_page_lock() will protect 
> against ownership changes.
>
> *If* that assertion you added is always true, that there is always at least 
> one reference in the rmap that will be held; *and* those references are 
> always “held” on behalf of dom_cow, then I think your version will probably 
> be safe (as the reference in the rmap will prevent it from changing ownership 
> until the final put_page_and_type()).  But if #2 ever false for any reason 
> (even due to a bug in Xen), then this will open up a potential security issue 
> (since ASSERT()s don’t trigger in production code).
>
> A couple of options:
> * Make it a BUG_ON(); that would change a potential privilege escalation into 
> a DoS

I'm fine with changing that ASSERT into a BUG_ON.

> * Only clear PGC_allocated if put_count > 0.  This changes a potential 
> privilege escalation into a slow memory leak
> * If put_count == 0, grab an extra reference and drop it afterwards (with an 
> ASSERT_UNREACHABLE() to help catch errors early).  This avoids any XSA, but 
> makes the code a bit uglier.
> * Leave the code as it is; just change the get_page() to grabbing dom_cow 
> rather than cd.  This avoids any XSA, but adds an extra get / put page which 
> will be pure overhead in the common case.

Simply changing to get_page with dom_cow is not enough, I ran into
issues where the system still deadlocks due to the ordering of
put_page_and_type being called before mem_sharing_page_unlock. So the
ordering still has to be changed.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel