Re: [PATCH v4 2/7] arch, arm: add consistency checks to REMOVE p2m changes

[Julien Grall <julien.grall@linaro.org>]

On 03/25/2014 12:51 PM, Julien Grall wrote:
> Hi Arianna,
> 
> Thank you for the patch,
> 
> On 03/25/2014 02:02 AM, Arianna Avanzini wrote:
>> Currently, the REMOVE case of the switch in apply_p2m_changes()
>> does not perform any consistency check on the mapping to be removed.
>> More in detail, the code does not check if the guest address to be
>> unmapped is actually mapped to the machine address given as a
>> parameter.
>> This commit attempts to add the above-described consistency check
>> to the REMOVE path of apply_p2m_changes(). This is instrumental to
>> one of the following commits which implements the possibility to
>> trigger the removal of p2m ranges via the memory_mapping DOMCTL
>> for ARM.
>>
>> Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
>> Cc: Dario Faggioli <dario.faggioli@citrix.com>
>> Cc: Paolo Valente <paolo.valente@unimore.it>
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Cc: Julien Grall <julien.grall@citrix.com>
>> Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
>> Cc: Jan Beulich <JBeulich@suse.com>
>> Cc: Keir Fraser <keir@xen.org>
>> Cc: Tim Deegan <tim@xen.org>
>> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> Cc: Eric Trudeau <etrudeau@broadcom.com>
>> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
>>
>> ---
>>
>>     v4:
>>         - Remove useless and slow lookup and use already-available
>>           data from pte instead.
>>         - Correctly increment the local variable used to keep the
>>           machine address whose mapping is currently being removed.
>>         - Return with an error upon finding a mismatch between the
>>           actual machine address mapped to the guest address and
>>           the machine address passed as parameter, instead of just
>>           skipping the page.
>>
>> ---
>>  xen/arch/arm/p2m.c | 24 ++++++++++++++++++++----
>>  1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index d00c882..bb0db16 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -243,12 +243,13 @@ static int apply_p2m_changes(struct domain *d,
>>      int rc;
>>      struct p2m_domain *p2m = &d->arch.p2m;
>>      lpae_t *first = NULL, *second = NULL, *third = NULL;
>> -    paddr_t addr;
>> +    paddr_t addr, _maddr;
>>      unsigned long cur_first_page = ~0,
>>                    cur_first_offset = ~0,
>>                    cur_second_offset = ~0;
>>      unsigned long count = 0;
>>      unsigned int flush = 0;
>> +    unsigned long mfn;
>>      bool_t populate = (op == INSERT || op == ALLOCATE);
>>      lpae_t pte;
>>  
>> @@ -258,6 +259,7 @@ static int apply_p2m_changes(struct domain *d,
>>          p2m_load_VTTBR(d);
>>  
>>      addr = start_gpaddr;
>> +    _maddr = maddr;
> 
> You don't need a temporary variable to hold maddr and increment it.
> You can directly use maddr, but you will have to remove "maddr +=
> PAGE_SIZE" in INSERT.
> 
> You also need to increment maddr when first/second level are not valid.
> 
> While reading the code, I'm wondering if we need to return an error if
> we are trying to remove a non-existent page. Ian, Stefano, any thoughs?
> 
>>      while ( addr < end_gpaddr )
>>      {
>>          if ( cur_first_page != p2m_first_level_index(addr) )
>> @@ -327,6 +329,7 @@ static int apply_p2m_changes(struct domain *d,
>>  
>>          flush |= pte.p2m.valid;
>>  
>> +        mfn = pte.p2m.base;
> 
> I would move mfn = ... in REMOVE part because it may not be valid on
> some place and wrongly used in the future.
> 
>>          /* TODO: Handle other p2m type
>>           *
>>           * It's safe to do the put_page here because page_alloc will
>> @@ -335,8 +338,6 @@ static int apply_p2m_changes(struct domain *d,
>>           */
>>          if ( pte.p2m.valid && p2m_is_foreign(pte.p2m.type) )
>>          {
>> -            unsigned long mfn = pte.p2m.base;
>> -
>>              ASSERT(mfn_valid(mfn));
>>              put_page(mfn_to_page(mfn));
>>          }
>> @@ -367,9 +368,23 @@ static int apply_p2m_changes(struct domain *d,
>>                      maddr += PAGE_SIZE;
>>                  }
>>                  break;
>> -            case RELINQUISH:
>>              case REMOVE:
>>                  {
>> +                    ASSERT(pte.p2m.valid);
> 
> Why did you add an ASSERT here?

Hmmm ... after reading you patch #4, this assert is wrong. Anyone that
can call XEN_DOMCTL_memory_mapping (e.g the toolstack) can crash Xen
because there is no check that the GFN has a valid mapping.

You should replace this ASSERT by an if ...

Regards,

-- 
Julien Grall