Re: [Xen-devel] [for-4.7 2/2] xen/arm: p2m: Release the p2m lock before undoing the mappings

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 17 May 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 17/05/16 13:27, Stefano Stabellini wrote:
> > On Tue, 17 May 2016, Julien Grall wrote:
> > > On 17/05/16 12:24, Stefano Stabellini wrote:
> > > > I think you are right. Especially with backports in mind, it would be
> > > > better to introduce an __apply_p2m_changes function which assumes that
> > > > the p2m lock has already been taken by the caller. Then you can base the
> > > > implementation of apply_p2m_changes on it.
> > > 
> > > > On Tue, 17 May 2016, Wei Chen wrote:
> > > > > Hi Julien,
> > > > > 
> > > > > I have some concern about this patch. Because we released the spinlock
> > > > > before remove the mapped memory. If somebody acquires the spinlock
> > > > > before we remove the mapped memory, this mapped memory region can be
> > > > > accessed by guest.
> > > > > 
> > > > > The apply_p2m_changes is no longer atomic. Is it a security risk?
> > > 
> > > Accesses to the page table have never been atomic, as soon as an entry is
> > > written in the page tables, the guest vCPUs or a prefetcher could read it.
> > > 
> > > The spinlock is only here to protect the page tables against concurrent
> > > modifications. Releasing the lock is not an issue as Xen does not promise
> > > any
> > > ordering for the p2m changes.
> > 
> > I understand that. However I am wondering whether it might be possible
> > for the guest to run commands which cause concurrent p2m change requests
> > on purpose, inserting something else between the first phase and the
> > second phase of apply_p2m_changes, causing problems to the hypervisor.
> 
> Removing and inserting entries are 2 distinct steps.
> 
> > Or maybe not even on purpose, but causing problem to itself nonetheless.
> 
> Each vCPU can only trigger one command at the time. So concurrent p2m changes
> would involve 2 vCPUs.
> 
> Even if vCPU A send the command before vCPU B, nothing prevents Xen to serve B
> before A.
> 
> The only way a guest could harm itself would be to have the 2 requests
> modifying the same regions in the page tables. However, per-above this
> behavior is undefined no matter the implementation of apply_p2m_changes.

All right, so the use case that should haved worked before (but didn't
because the implementation was broken) and wouldn't work anymore with
this patch is the following:

- vcpu1 asks region1 to be mapped at gpaddrA
  the mapping fails at least partially
- vcpu2 asks region2 to be mapped at gpaddrA
  the mapping succeeds

This doesn't work anymore because the second mapping could be done in
between the two halves of the first mapping, resulting in a partially
mapped region2.

I realize that this is an unimportant case and not worth supporting. I,
for one, would prefer not to have to think about implementation halves
of apply_p2m_changes going forward so I would prefer a different patch.
That said, I still retract my comment and leave it up to you. If you
would like to change this patch, I'll be happy to review v2, otherwise,
if you prefer to keep it as is, let me know and I'll commit this
version.


> > Honestly it is true that it doesn't look like Xen could run into
> > troubles. But still this is a change in behaviour compared to the
> > current code (which I know doesn't actually work) and I wanted to flag
> > it.
> 
> This code has always been buggy, and I suspect the goal was to call back
> without the lock.
> 
> There is no reason to keep the lock more than necessary. Releasing the lock
> allow other p2m changes to be executed rather than spinning while the long
> execution (INSERTION + REMOVAL) is done.

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