Re: [Xen-devel] [PATCH v2 2/3] xen/pvh: check permissions when adding MMIO regions

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/01/15 17:05, 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>
> ---
> Changes since v1:
>  - Use the newly introduced p2m_access_t to set the access type.
>  - Don't add a next label.
> ---
>  xen/arch/x86/domain_build.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> index f687c78..41d2541 100644
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -319,12 +319,25 @@ static __init void pvh_add_mem_mapping(struct domain 
> *d, unsigned long gfn,
>                                         unsigned long mfn, unsigned long 
> nr_mfns)
>  {
>      unsigned long i;
> +    mfn_t omfn;
> +    p2m_type_t t;
> +    p2m_access_t a;
>      int rc;
>  
>      for ( i = 0; i < nr_mfns; i++ )
>      {
> -        if ( (rc = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i),
> -                                      p2m_get_hostp2m(d)->default_access)) )
> +        if ( !iomem_access_permitted(d, mfn + i, mfn + i) ) {
> +            omfn = get_gfn_query_unlocked(d, gfn + i, &t);
> +            guest_physmap_remove_page(d, gfn + i, mfn_x(omfn), 
> PAGE_ORDER_4K);
> +            continue;
> +        }

This suggests a design flaw (possibly pre-existing).  We should not be
removing physmap entries in pvh_add_mem_mapping(), nor should we be a
position to need to revoke physmap entries during domain build.

If there is anything needing revoking at this stage, it should not have
been added earlier.  How did you come to introduce this code?

~Andrew

> +
> +        if ( rangeset_contains_singleton(mmio_ro_ranges, mfn + i) )
> +            a = p2m_access_r;
> +        else
> +            a = p2m_access_rw;
> +
> +        if ( (rc = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i), a)) )
>              panic("pvh_add_mem_mapping: gfn:%lx mfn:%lx i:%ld rc:%d\n",
>                    gfn, mfn, i, rc);
>          if ( !(i & 0xfffff) )



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