Re: [Xen-devel] [PATCH v7 02/12] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.09.17 at 17:31, <paul.durrant@xxxxxxxxxx> wrote:
> Certain memory resources associated with a guest are not necessarily
> present in the guest P2M and so are not necessarily available to be
> foreign-mapped by a tools domain unless they are inserted, which risks
> shattering a super-page mapping.

For grant tables I can see this as the primary issue, but isn't the
goal of not exposing IOREQ server pages an even more important
aspect, and hence more relevant to mention here?

> NOTE: Whilst the new op is not intrinsicly specific to the x86 architecture,
>       I have no means to test it on an ARM platform and so cannot verify
>       that it functions correctly. Hence it is currently only implemented
>       for x86.

Which will require things to be moved around later on. May I
instead suggest to put it in common code and simply have a
small #ifdef somewhere causing the operation to fail early for
ARM?

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4768,6 +4768,107 @@ int xenmem_add_to_physmap_one(
>      return rc;
>  }
>  
> +static int xenmem_acquire_grant_table(struct domain *d,

I don't think static functions need a xenmem_ prefix.

> +static int xenmem_acquire_resource(xen_mem_acquire_resource_t *xmar)

const?

> +{
> +    struct domain *d, *currd = current->domain;
> +    unsigned long *mfn_list;
> +    int rc;
> +
> +    if ( xmar->nr_frames == 0 )
> +        return -EINVAL;
> +
> +    d = rcu_lock_domain_by_any_id(xmar->domid);
> +    if ( d == NULL )
> +        return -ESRCH;
> +
> +    rc = xsm_domain_memory_map(XSM_TARGET, d);
> +    if ( rc )
> +        goto out;
> +
> +    mfn_list = xmalloc_array(unsigned long, xmar->nr_frames);

Despite XSA-77 there should not be any new disaggregation related
security risks (here: memory exhaustion and long lasting loops).
Large requests need to be refused or broken up.

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -3607,38 +3607,58 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, 
> grant_ref_t ref,
>  }
>  #endif
>  
> -int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> -                     mfn_t *mfn)
> -{
> -    int rc = 0;
>  
> -    grant_write_lock(d->grant_table);
> +static mfn_t gnttab_get_frame_locked(struct domain *d, unsigned long idx)
> +{
> +    struct grant_table *gt = d->grant_table;
> +    mfn_t mfn = INVALID_MFN;
>  
> -    if ( d->grant_table->gt_version == 0 )
> -        d->grant_table->gt_version = 1;
> +    if ( gt->gt_version == 0 )
> +        gt->gt_version = 1;
>  
> -    if ( d->grant_table->gt_version == 2 &&
> +    if ( gt->gt_version == 2 &&
>           (idx & XENMAPIDX_grant_table_status) )
>      {
>          idx &= ~XENMAPIDX_grant_table_status;

I don't see the use of this bit mentioned anywhere in the public
header addition.

> +mfn_t gnttab_get_frame(struct domain *d, unsigned long idx)
> +{
> +    mfn_t mfn;
> +
> +    grant_write_lock(d->grant_table);
> +    mfn = gnttab_get_frame_locked(d, idx);
> +    grant_write_unlock(d->grant_table);

Hmm, a read lock is insufficient here only because of the possible
bumping of the version from 0 to 1 afaict, but I don't really see
why what now becomes gnttab_get_frame_locked() does that in
the first place.

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -650,7 +650,43 @@ struct xen_vnuma_topology_info {
>  typedef struct xen_vnuma_topology_info xen_vnuma_topology_info_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_vnuma_topology_info_t);
>  
> -/* Next available subop number is 28 */
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +
> +/*
> + * Get the pages for a particular guest resource, so that they can be
> + * mapped directly by a tools domain.
> + */
> +#define XENMEM_acquire_resource 28
> +struct xen_mem_acquire_resource {
> +    /* IN - the domain whose resource is to be mapped */
> +    domid_t domid;
> +    /* IN - the type of resource (defined below) */
> +    uint16_t type;
> +
> +#define XENMEM_resource_grant_table 0
> +
> +    /*
> +     * IN - a type-specific resource identifier, which must be zero
> +     *      unless stated otherwise.
> +     */
> +    uint32_t id;
> +    /* IN - number of (4K) frames of the resource to be mapped */
> +    uint32_t nr_frames;
> +    /* IN - the index of the initial frame to be mapped */
> +    uint64_aligned_t frame;

There are 32 bits of unnamed padding ahead of this field - please
name it and check it's set to zero.

> +    /* IN/OUT - If the tools domain is PV then, upon return, gmfn_list
> +     *          will be populated with the MFNs of the resource.
> +     *          If the tools domain is HVM then it is expected that, on

s/tools domain/calling domain/ (twice)?

> +     *          entry, gmfn_list will be populated with a list of GFNs

s/will be/is/ to further emphasize the input vs output difference?

> +     *          that will be mapped to the MFNs of the resource.
> +     */
> +    XEN_GUEST_HANDLE(xen_pfn_t) gmfn_list;

What about a 32-bit x86 tool stack domain as caller here? I
don't think you want to limit it to just the low 16Tb? Nor are
you adding a compat translation layer.

> +};
> +typedef struct xen_mem_acquire_resource xen_mem_acquire_resource_t;
> +
> +#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */

Also please group this with the other similar section, without
introducing a second identical #if. After all we have ...

> +/* Next available subop number is 29 */

... this to eliminate the need to have things numerically sorted in
this file.

Jan

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