Re: [Xen-devel] [PATCH RFC v2 07/12] x86: allow per-domain mappings without NX bit or with specific mfn

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.01.18 at 13:32, <jgross@xxxxxxxx> wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -1568,7 +1568,7 @@ void init_xen_l4_slots(l4_pgentry_t *l4t, mfn_t l4mfn,
>  
>      /* Slot 260: Per-domain mappings (if applicable). */
>      l4t[l4_table_offset(PERDOMAIN_VIRT_START)] =
> -        d ? l4e_from_page(d->arch.perdomain_l3_pg, __PAGE_HYPERVISOR_RW)
> +        d ? l4e_from_page(d->arch.perdomain_l3_pg, __PAGE_HYPERVISOR)
>            : l4e_empty();
>  
>      /* Slot 261-: text/data/bss, RW M2P, vmap, frametable, directmap. */
> @@ -5269,7 +5269,7 @@ int create_perdomain_mapping(struct domain *d, unsigned 
> long va,
>          }
>          l2tab = __map_domain_page(pg);
>          clear_page(l2tab);
> -        l3tab[l3_table_offset(va)] = l3e_from_page(pg, __PAGE_HYPERVISOR_RW);
> +        l3tab[l3_table_offset(va)] = l3e_from_page(pg, __PAGE_HYPERVISOR);
>      }
>      else
>          l2tab = map_l2t_from_l3e(l3tab[l3_table_offset(va)]);
> @@ -5311,7 +5311,7 @@ int create_perdomain_mapping(struct domain *d, unsigned 
> long va,
>                  l1tab = __map_domain_page(pg);
>              }
>              clear_page(l1tab);
> -            *pl2e = l2e_from_page(pg, __PAGE_HYPERVISOR_RW);
> +            *pl2e = l2e_from_page(pg, __PAGE_HYPERVISOR);

These changes (in the absence of the description saying otherwise)
leave open whether any of the per-domain mappings now suddenly
become executable.

> @@ -5401,6 +5401,81 @@ void destroy_perdomain_mapping(struct domain *d, 
> unsigned long va,
>      unmap_domain_page(l3tab);
>  }
>  
> +void flipflags_perdomain_mapping(struct domain *d, unsigned long va,
> +                                 unsigned int flags)

Flipping flags means the caller has to know (perhaps track) what state
the flags are in at present. I think it would be better to pass in two
masks - one for flags to be set, and the other for flags to be cleared.

> +void addmfn_to_perdomain_mapping(struct domain *d, unsigned long va, mfn_t 
> mfn)
> +{
> +    const l3_pgentry_t *l3tab, *pl3e;
> +
> +    ASSERT(va >= PERDOMAIN_VIRT_START &&
> +           va < PERDOMAIN_VIRT_SLOT(PERDOMAIN_SLOTS));
> +
> +    if ( !d->arch.perdomain_l3_pg )
> +        return;
> +
> +    l3tab = __map_domain_page(d->arch.perdomain_l3_pg);
> +    pl3e = l3tab + l3_table_offset(va);
> +
> +    if ( l3e_get_flags(*pl3e) & _PAGE_PRESENT )
> +    {
> +        const l2_pgentry_t *l2tab = map_l2t_from_l3e(*pl3e);
> +        const l2_pgentry_t *pl2e = l2tab + l2_table_offset(va);
> +
> +        if ( l2e_get_flags(*pl2e) & _PAGE_PRESENT )
> +        {
> +            l1_pgentry_t *l1tab = map_l1t_from_l2e(*pl2e);
> +            unsigned int off = l1_table_offset(va);
> +
> +            if ( (l1e_get_flags(l1tab[off]) & (_PAGE_PRESENT | 
> _PAGE_AVAIL0)) ==
> +                 (_PAGE_PRESENT | _PAGE_AVAIL0) )
> +                free_domheap_page(l1e_get_page(l1tab[off]));
> +
> +            l1tab[off] = l1e_from_mfn(mfn, __PAGE_HYPERVISOR_RW);
> +
> +            unmap_domain_page(l1tab);
> +        }
> +
> +        unmap_domain_page(l2tab);
> +    }
> +
> +    unmap_domain_page(l3tab);
> +}

Here even more than in the flipflags function - what if an
intermediate page table entry was not present? The caller will
have no ideal that what was requested wasn't carried out.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel