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

[Wei Chen <wei.chen@arm.com>]

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@arm.com>
>
> 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@arm.com>
>>
>> ---
>>     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@lists.xen.org
http://lists.xen.org/xen-devel