Re: [Xen-devel] [PATCH v1 for-4.6 1/2] xen/pvh: check permissions when adding MMIO regions

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/12/14 18:27, Roger Pau Monne wrote:
> Check that MMIO regions added to PVH Dom0 are allowed. Previously a PVH Dom0
> would have access to the full MMIO range.
>
> Signed-off-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
>  xen/arch/x86/domain_build.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> index 7a6afea..aa3bf0f 100644
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -312,16 +312,47 @@ static __init void pvh_add_mem_mapping(struct domain 
> *d, unsigned long gfn,
>                                         unsigned long mfn, unsigned long 
> nr_mfns)
>  {
>      unsigned long i;
> +    xenmem_access_t def_access;
> +    bool_t r_only = false;
>      int rc;
>  
>      for ( i = 0; i < nr_mfns; i++ )
>      {
> +        if ( !iomem_access_permitted(d, mfn + i, mfn + i) )
> +            goto next;
> +
> +        if ( rangeset_contains_singleton(mmio_ro_ranges, mfn + i) && !r_only 
> )
> +        {
> +            rc = p2m_get_mem_access(d, ~0ul, &def_access);
> +            BUG_ON(rc);
> +            /* Set default access to read-only */
> +            rc = p2m_set_mem_access(d, ~0ul, 0, 0, 0, XENMEM_access_r);
> +            BUG_ON(rc);
> +            r_only = true;
> +        }
> +        else if ( r_only )
> +        {
> +            /* Set the default back */
> +            rc = p2m_set_mem_access(d, ~0ul, 0, 0, 0, def_access);
> +            BUG_ON(rc);
> +            r_only = false;
> +        }

Am I missing something obvious, or would all this r_only juggling be far
more easy if set_mmio_p2m_entry() had a ro/rw boolean parameter?

As these entries are done one at a time, it would seem to be far less
error prone to explicitly choose a read-only or read-write mmio mapping,
rather than playing with the entire default.

> +
>          if ( (rc = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i))) )
>              panic("pvh_add_mem_mapping: gfn:%lx mfn:%lx i:%ld rc:%d\n",
>                    gfn, mfn, i, rc);
> +
> + next:
>          if ( !(i & 0xfffff) )
>                  process_pending_softirqs();

You could remove the next label by moving this clause to the top and
adding an i != 0 check.

>      }
> +
> +    if ( r_only )
> +    {
> +        /* Set the default back */
> +        rc = p2m_set_mem_access(d, ~0ul, 0, 0, 0, def_access);

This will clobber the p2m default access type based on whether the final
mfn is read-only or read-write.

~Andrew

> +        BUG_ON(rc);
> +    }
>  }
>  
>  /*



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