Re: [PATCH v9 01/14] arch/arm: add consistency check to REMOVE p2m changes

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

On 07/02/2014 07:42 PM, Arianna Avanzini wrote:
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 9960e17..7cb4a27 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -439,12 +441,37 @@ static int apply_p2m_changes(struct domain *d,
>                      pte = mfn_to_p2m_entry(maddr >> PAGE_SHIFT, mattr, t);
>                      p2m_write_pte(&third[third_table_offset(addr)],
>                                    pte, flush_pt);
> -                    maddr += PAGE_SIZE;
>                  }
>                  break;
> -            case RELINQUISH:
>              case REMOVE:
>                  {
> +                    unsigned long mfn = pte.p2m.base;
> +
> +                    /*
> +                     * Ensure that the guest address addr currently being
> +                     * handled (that is in the range given as argument to
> +                     * this function) is actually mapped to the corresponding
> +                     * machine address in the specified range. 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 ( !pte.p2m.valid || maddr != pfn_to_paddr(mfn) )
> +                    {
> +                        gdprintk(XENLOG_WARNING,
> +                                 "p2m_remove: mapping at %"PRIpaddr" is of maddr %"PRIpaddr" not %"PRIpaddr" as expected",
> +                                 addr, pfn_to_paddr(mfn), maddr);

gdprintk is using the current domain to print the domid. We are not
necessarily remove the mapping from the current domain.

> +                        /*
> +                         * Continue to process the range even if an error is
> +                         * encountered, to prevent I/O-memory regions from
> +                         * being partially accessible to a domain.
> +                         */
> +                       continue;


I've just reviewed the patch #5 (which does the similar check for x86)
and I'm surprised that you differ. Here you let the mapping in place if
something is wrong rather than clearing it.

That made me think there is a possible security issue here. If you are
trying to clear a page that it's actually a foreign mapping, we already
drop the reference above. Xen may reallocate this page for anything
else, so the domain will have a mapping to a page which potentially
belong to another domain or Xen. Therefore we will leak information.

I would drop the continue here and just print a warning.

Regards,

-- 
Julien Grall