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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

Wei,
It might be best for you to give two tags in the future: Reviewed-by and
Tested-by separately. Lars uses scripts to parse git logs for tags and
generate stats. If you use non-standard tags, they won't be accounted
for correctly.


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?
> 
> On 2016/5/17 13:45, Wei Chen wrote:
> > Hi Julien,
> > 
> > This code looks good to me, and I have tested that
> > the deadlock is fixed by this patch.
> > 
> > Reviewed-and-Tested-by: Wei Chen <Wei.Chen@xxxxxxx>
> > 
> > Original:
> > (XEN) smmu: /smb/smmu@e0800000: P2M IPA size not supported (P2M=44
> > SMMU=40)!
> > (XEN) I/O virtualisation disabled
> > (XEN) Request p2m Spinlock...
> > (XEN) Request p2m Spinlock...OK
> > (XEN) apply_p2m_changes failed! rollback! call apply_p2m_changes
> > recursively!
> > (XEN) Request p2m Spinlock...
> > 
> > Patched:
> > (XEN) smmu: /smb/smmu@e0800000: P2M IPA size not supported (P2M=44
> > SMMU=40)!
> > (XEN) I/O virtualisation disabled
> > (XEN) Request p2m Spinlock...
> > (XEN) Request p2m Spinlock...OK
> > (XEN) apply_p2m_changes failed! rollback! call apply_p2m_changes
> > recursively!
> > (XEN) Request p2m Spinlock...
> > (XEN) Request p2m Spinlock...OK
> > 
> > 
> > On 2016/5/16 22:08, Julien Grall wrote:
> > > Since commit 4b25423a "arch/arm: unmap partially-mapped memory regions",
> > > Xen has been undoing the P2M mappings when an error occurred during
> > > insertion or memory allocation.
> > > 
> > > This is done by calling recursively apply_p2m_changes, however the
> > > second call is done with the p2m lock taken which will result in a
> > > deadlock for the current processor.
> > > 
> > > Fix it by moving the recursive call after the lock has been released.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > > 
> > > ---
> > >     I think we could unlock the p2m lock before freeing the temporary
> > >     mapping. Although, I played safe as this is a bug fix for Xen 4.7
> > >     and to be backported up to Xen 4.5.
> > > ---
> > >  xen/arch/arm/p2m.c | 16 ++++++++--------
> > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > index 68c67b0..838d004 100644
> > > --- a/xen/arch/arm/p2m.c
> > > +++ b/xen/arch/arm/p2m.c
> > > @@ -1184,6 +1184,14 @@ out:
> > >      while ( (pg = page_list_remove_head(&free_pages)) )
> > >          free_domheap_page(pg);
> > > 
> > > +    for ( level = P2M_ROOT_LEVEL; level < 4; level ++ )
> > > +    {
> > > +        if ( mappings[level] )
> > > +            unmap_domain_page(mappings[level]);
> > > +    }
> > > +
> > > +    spin_unlock(&p2m->lock);
> > > +
> > >      if ( rc < 0 && ( op == INSERT || op == ALLOCATE ) &&
> > >           addr != start_gpaddr )
> > >      {
> > > @@ -1196,14 +1204,6 @@ out:
> > >                            mattr, 0, p2m_invalid,
> > > d->arch.p2m.default_access);
> > >      }
> > > 
> > > -    for ( level = P2M_ROOT_LEVEL; level < 4; level ++ )
> > > -    {
> > > -        if ( mappings[level] )
> > > -            unmap_domain_page(mappings[level]);
> > > -    }
> > > -
> > > -    spin_unlock(&p2m->lock);
> > > -
> > >      return rc;
> > >  }
> > > 
> > > 
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.
> 

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