Re: [Xen-devel] [PATCH v2 1/1] xen: move TLB-flush filtering out into populate_physmap

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 06/09/16 10:55, David Vrabel wrote:
> On 06/09/16 10:52, George Dunlap wrote:
>> On 06/09/16 10:39, David Vrabel wrote:
>>> On 06/09/16 09:42, Dongli Zhang wrote:
>>>> This patch implemented parts of TODO left in commit id
>>>> a902c12ee45fc9389eb8fe54eeddaf267a555c58. It moved TLB-flush filtering out
>>>> into populate_physmap.
>>>>
>>>> Because of TLB-flush in alloc_heap_pages, it's very slow to create a guest
>>>> with memory size of more than 100GB on host with 100+ cpus.
>>>>
>>>> This patch introduced a "MEMF_no_tlbflush" bit to memflag to indicate
>>>> whether TLB-flush should be done in alloc_heap_pages or its caller
>>>> populate_physmap.  Once this bit is set in memflag, alloc_heap_pages will
>>>> ignore TLB-flush.
>>>
>>> This makes pages accessible to the guest B, when guest A may still have
>>> a cached mapping to them.
>>>
>>> I think it is only safe to do this when guest B is being constructed.
>>
>> Only before the populate_physmap hypercall returns, right?  In order to
>> run afoul of this, a second vcpu on the guest would have to map and then
>> put something secret / sensitive in the memory (such as a private key
>> which guest A could then read, or code it intended to execute that guest
>> A could then modify) before the hypercall finished.  That seems like
>> something we should be able to document as undefined / unsafe behavior
>> if we want.
> 
> I think populate_physmap can replace existing p2m entries, and thus
> guest B may already have mappings.

Swapping an actively in-use gpfn out from under your feet without any
form of synchronization seems like a pretty mad thing to do to me; you
couldn't tell whether any writes to the page would end up on the old
(soon-to-be-freed) page, or the newly allocated page, and executing on
such a page might at any moment be filled with a random page from Xen.

It used to be the case that pages freed to Xen other than at domain
death weren't scrubbed by Xen; looking briefly through the put_page()
call path it looks like that's still the case.

If so, then in the scenario above, sensitive information written to the
old page before the swap would be leaked to other guests anyway; and the
newly allocated page might have rogue code written by another guest
before it was freed.  So the new "window" wouldn't be any less of a
security risk than the old one.

So I think one could still make the argument for documenting that
behavior as undefined or unsafe, if we thought it would have an impact
on the speed of large ballooning operations.

But since what Dongli cares about at the moment is domain creation, it
certainly won't hurt to limit this optimization to domain creation time;
then we can discuss enabling it for ballooning when someone finds it to
be an issue.

 -George


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