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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 25 Mar 2014, 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@xxxxxxxxx>
> Cc: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> Cc: Paolo Valente <paolo.valente@xxxxxxxxxx>
> Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxxxxx>
> Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
> Cc: Jan Beulich <JBeulich@xxxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> Cc: Eric Trudeau <etrudeau@xxxxxxxxxxxx>
> Cc: Viktor Kleinik <viktor.kleinik@xxxxxxxxxxxxxxx>
> 

Acked-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>


> 
>     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;
>      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;
>          /* 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);
> +                    /*
> +                     * Ensure that the guest address given as argument to
> +                     * this function is actually mapped to the specified
> +                     * machine address. _maddr here is the machine address
> +                     * given to the function, while mfn is the machine
> +                     * frame number actually mapped to the guest address:
> +                     * check if the two correspond.
> +                     */
> +                    if ( _maddr != pfn_to_paddr(mfn) )
> +                        return -EINVAL;
> +                }
> +                /* fall through */
> +            case RELINQUISH:
> +                {
>                      if ( !pte.p2m.valid )
>                      {
>                          count++;
> @@ -408,6 +423,7 @@ static int apply_p2m_changes(struct domain *d,
>  
>          /* Got the next page */
>          addr += PAGE_SIZE;
> +        _maddr += PAGE_SIZE;
>      }
>  
>      if ( flush )
> -- 
> 1.9.0
> 

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