Message ID | 20170905113716.3960-3-paul.durrant@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, Sep 05, 2017 at 12:37:06PM +0100, Paul Durrant wrote: [...] > > +static int xenmem_acquire_grant_table(struct domain *d, > + unsigned long frame, > + unsigned long nr_frames, > + unsigned long mfn_list[]) > +{ > + unsigned int i; > + > + /* > + * Iterate through the list backwards so that gnttab_get_frame() is > + * first called for the highest numbered frame. This means that the > + * out-of-bounds check will be done on the first iteration and, if > + * the table needs to grow, it will only grow once. > + */ > + i = nr_frames; > + while ( i-- != 0 ) > + { > + mfn_t mfn = gnttab_get_frame(d, frame + i); > + I think you should lock guest grant table first and use the _locked variant here to get a consistent view of guest grant table frames. > + if ( mfn_eq(mfn, INVALID_MFN) ) > + return -EINVAL; > + > + mfn_list[i] = mfn_x(mfn); > + } > + > + return 0; > +} > + > +static int xenmem_acquire_resource(xen_mem_acquire_resource_t *xmar) > +{ > + 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); > + > + rc = -ENOMEM; > + if ( !mfn_list ) > + goto out; > + > + switch ( xmar->type ) > + { > + case XENMEM_resource_grant_table: > + rc = -EINVAL; > + if ( xmar->id ) /* must be zero for grant_table */ > + break; > + > + rc = xenmem_acquire_grant_table(d, xmar->frame, xmar->nr_frames, > + mfn_list); > + break; > + > + default: > + rc = -EOPNOTSUPP; > + break; > + } > + > + if ( rc ) > + goto free_and_out; > + > + if ( !paging_mode_translate(currd) ) > + { > + if ( __copy_to_guest_offset(xmar->gmfn_list, 0, mfn_list, > + xmar->nr_frames) ) Please use the copy_to_guest_offset variant which has more checks, or you need to check a priori if the range is okay. > + rc = -EFAULT; > + } > + else > + { > + unsigned int i; > + > + for ( i = 0; i < xmar->nr_frames; i++ ) > + { > + xen_pfn_t gfn; > + > + rc = -EFAULT; > + if ( __copy_from_guest_offset(&gfn, xmar->gmfn_list, i, 1) ) Same here -- although HVM guest takes another path, it would be good to be consistent.
> -----Original Message----- > From: Wei Liu [mailto:wei.liu2@citrix.com] > Sent: 07 September 2017 12:11 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: xen-devel@lists.xenproject.org; Andrew Cooper > <Andrew.Cooper3@citrix.com>; Jan Beulich <jbeulich@suse.com>; Wei Liu > <wei.liu2@citrix.com> > Subject: Re: [Xen-devel] [PATCH v4 02/12] x86/mm: add > HYPERVISOR_memory_op to acquire guest resources > > On Tue, Sep 05, 2017 at 12:37:06PM +0100, Paul Durrant wrote: > [...] > > > > +static int xenmem_acquire_grant_table(struct domain *d, > > + unsigned long frame, > > + unsigned long nr_frames, > > + unsigned long mfn_list[]) > > +{ > > + unsigned int i; > > + > > + /* > > + * Iterate through the list backwards so that gnttab_get_frame() is > > + * first called for the highest numbered frame. This means that the > > + * out-of-bounds check will be done on the first iteration and, if > > + * the table needs to grow, it will only grow once. > > + */ > > + i = nr_frames; > > + while ( i-- != 0 ) > > + { > > + mfn_t mfn = gnttab_get_frame(d, frame + i); > > + > > I think you should lock guest grant table first and use the _locked > variant here to get a consistent view of guest grant table frames. Once the table has grown, is there any way they can change? > > > + if ( mfn_eq(mfn, INVALID_MFN) ) > > + return -EINVAL; > > + > > + mfn_list[i] = mfn_x(mfn); > > + } > > + > > + return 0; > > +} > > + > > +static int xenmem_acquire_resource(xen_mem_acquire_resource_t > *xmar) > > +{ > > + 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); > > + > > + rc = -ENOMEM; > > + if ( !mfn_list ) > > + goto out; > > + > > + switch ( xmar->type ) > > + { > > + case XENMEM_resource_grant_table: > > + rc = -EINVAL; > > + if ( xmar->id ) /* must be zero for grant_table */ > > + break; > > + > > + rc = xenmem_acquire_grant_table(d, xmar->frame, xmar- > >nr_frames, > > + mfn_list); > > + break; > > + > > + default: > > + rc = -EOPNOTSUPP; > > + break; > > + } > > + > > + if ( rc ) > > + goto free_and_out; > > + > > + if ( !paging_mode_translate(currd) ) > > + { > > + if ( __copy_to_guest_offset(xmar->gmfn_list, 0, mfn_list, > > + xmar->nr_frames) ) > > Please use the copy_to_guest_offset variant which has more checks, or > you need to check a priori if the range is okay. > > > + rc = -EFAULT; > > + } > > + else > > + { > > + unsigned int i; > > + > > + for ( i = 0; i < xmar->nr_frames; i++ ) > > + { > > + xen_pfn_t gfn; > > + > > + rc = -EFAULT; > > + if ( __copy_from_guest_offset(&gfn, xmar->gmfn_list, i, 1) ) > > Same here -- although HVM guest takes another path, it would be good to > be consistent. Ok, if you think it's necessary. (This is a tools-only hypercall and the ranges are supplied by privcmd, allocated in kernel). Paul
On Thu, Sep 07, 2017 at 12:18:25PM +0100, Paul Durrant wrote: > > -----Original Message----- > > From: Wei Liu [mailto:wei.liu2@citrix.com] > > Sent: 07 September 2017 12:11 > > To: Paul Durrant <Paul.Durrant@citrix.com> > > Cc: xen-devel@lists.xenproject.org; Andrew Cooper > > <Andrew.Cooper3@citrix.com>; Jan Beulich <jbeulich@suse.com>; Wei Liu > > <wei.liu2@citrix.com> > > Subject: Re: [Xen-devel] [PATCH v4 02/12] x86/mm: add > > HYPERVISOR_memory_op to acquire guest resources > > > > On Tue, Sep 05, 2017 at 12:37:06PM +0100, Paul Durrant wrote: > > [...] > > > > > > +static int xenmem_acquire_grant_table(struct domain *d, > > > + unsigned long frame, > > > + unsigned long nr_frames, > > > + unsigned long mfn_list[]) > > > +{ > > > + unsigned int i; > > > + > > > + /* > > > + * Iterate through the list backwards so that gnttab_get_frame() is > > > + * first called for the highest numbered frame. This means that the > > > + * out-of-bounds check will be done on the first iteration and, if > > > + * the table needs to grow, it will only grow once. > > > + */ > > > + i = nr_frames; > > > + while ( i-- != 0 ) > > > + { > > > + mfn_t mfn = gnttab_get_frame(d, frame + i); > > > + > > > > I think you should lock guest grant table first and use the _locked > > variant here to get a consistent view of guest grant table frames. > > Once the table has grown, is there any way they can change? > Hmm... no. I think you can leave the code as-is. > > Ok, if you think it's necessary. (This is a tools-only hypercall and the ranges are supplied by privcmd, allocated in kernel). > IMHO we should allow for use case for semi-trusted users of this hypercall in the future.
> -----Original Message----- > From: Wei Liu [mailto:wei.liu2@citrix.com] > Sent: 07 September 2017 12:36 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com>; xen-devel@lists.xenproject.org; Andrew > Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich <jbeulich@suse.com> > Subject: Re: [Xen-devel] [PATCH v4 02/12] x86/mm: add > HYPERVISOR_memory_op to acquire guest resources > > On Thu, Sep 07, 2017 at 12:18:25PM +0100, Paul Durrant wrote: > > > -----Original Message----- > > > From: Wei Liu [mailto:wei.liu2@citrix.com] > > > Sent: 07 September 2017 12:11 > > > To: Paul Durrant <Paul.Durrant@citrix.com> > > > Cc: xen-devel@lists.xenproject.org; Andrew Cooper > > > <Andrew.Cooper3@citrix.com>; Jan Beulich <jbeulich@suse.com>; Wei > Liu > > > <wei.liu2@citrix.com> > > > Subject: Re: [Xen-devel] [PATCH v4 02/12] x86/mm: add > > > HYPERVISOR_memory_op to acquire guest resources > > > > > > On Tue, Sep 05, 2017 at 12:37:06PM +0100, Paul Durrant wrote: > > > [...] > > > > > > > > +static int xenmem_acquire_grant_table(struct domain *d, > > > > + unsigned long frame, > > > > + unsigned long nr_frames, > > > > + unsigned long mfn_list[]) > > > > +{ > > > > + unsigned int i; > > > > + > > > > + /* > > > > + * Iterate through the list backwards so that gnttab_get_frame() is > > > > + * first called for the highest numbered frame. This means that the > > > > + * out-of-bounds check will be done on the first iteration and, if > > > > + * the table needs to grow, it will only grow once. > > > > + */ > > > > + i = nr_frames; > > > > + while ( i-- != 0 ) > > > > + { > > > > + mfn_t mfn = gnttab_get_frame(d, frame + i); > > > > + > > > > > > I think you should lock guest grant table first and use the _locked > > > variant here to get a consistent view of guest grant table frames. > > > > Once the table has grown, is there any way they can change? > > > > Hmm... no. > > I think you can leave the code as-is. > > > > > Ok, if you think it's necessary. (This is a tools-only hypercall and the ranges > are supplied by privcmd, allocated in kernel). > > > > IMHO we should allow for use case for semi-trusted users of this > hypercall in the future. Ok, I'll send a v5. Cheers, Paul
>>> On 07.09.17 at 13:36, <wei.liu2@citrix.com> wrote: > On Thu, Sep 07, 2017 at 12:18:25PM +0100, Paul Durrant wrote: >> Ok, if you think it's necessary. (This is a tools-only hypercall and the > ranges are supplied by privcmd, allocated in kernel). > > IMHO we should allow for use case for semi-trusted users of this > hypercall in the future. There are many other Dom0-only hypercalls where we don't apply relaxed checking, so the one here shouldn't be an exception. Jan
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index bd8aeac59e..b125b85bf9 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -4711,6 +4711,107 @@ int xenmem_add_to_physmap_one( return rc; } +static int xenmem_acquire_grant_table(struct domain *d, + unsigned long frame, + unsigned long nr_frames, + unsigned long mfn_list[]) +{ + unsigned int i; + + /* + * Iterate through the list backwards so that gnttab_get_frame() is + * first called for the highest numbered frame. This means that the + * out-of-bounds check will be done on the first iteration and, if + * the table needs to grow, it will only grow once. + */ + i = nr_frames; + while ( i-- != 0 ) + { + mfn_t mfn = gnttab_get_frame(d, frame + i); + + if ( mfn_eq(mfn, INVALID_MFN) ) + return -EINVAL; + + mfn_list[i] = mfn_x(mfn); + } + + return 0; +} + +static int xenmem_acquire_resource(xen_mem_acquire_resource_t *xmar) +{ + 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); + + rc = -ENOMEM; + if ( !mfn_list ) + goto out; + + switch ( xmar->type ) + { + case XENMEM_resource_grant_table: + rc = -EINVAL; + if ( xmar->id ) /* must be zero for grant_table */ + break; + + rc = xenmem_acquire_grant_table(d, xmar->frame, xmar->nr_frames, + mfn_list); + break; + + default: + rc = -EOPNOTSUPP; + break; + } + + if ( rc ) + goto free_and_out; + + if ( !paging_mode_translate(currd) ) + { + if ( __copy_to_guest_offset(xmar->gmfn_list, 0, mfn_list, + xmar->nr_frames) ) + rc = -EFAULT; + } + else + { + unsigned int i; + + for ( i = 0; i < xmar->nr_frames; i++ ) + { + xen_pfn_t gfn; + + rc = -EFAULT; + if ( __copy_from_guest_offset(&gfn, xmar->gmfn_list, i, 1) ) + goto free_and_out; + + rc = set_foreign_p2m_entry(currd, gfn, _mfn(mfn_list[i])); + if ( rc ) + goto free_and_out; + } + } + + free_and_out: + xfree(mfn_list); + + out: + rcu_unlock_domain(d); + return rc; +} + long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { int rc; @@ -4933,6 +5034,16 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) return rc; } + case XENMEM_acquire_resource: + { + xen_mem_acquire_resource_t xmar; + + if ( copy_from_guest(&xmar, arg, 1) ) + return -EFAULT; + + return xenmem_acquire_resource(&xmar); + } + default: return subarch_memory_op(cmd, arg); } diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index e8a57d118c..c503a7f1d2 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1118,8 +1118,7 @@ static int set_typed_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn, } /* Set foreign mfn in the given guest's p2m table. */ -static int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, - mfn_t mfn) +int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn) { return set_typed_p2m_entry(d, gfn, mfn, PAGE_ORDER_4K, p2m_map_foreign, p2m_get_hostp2m(d)->default_access); diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 9e18dc0493..d4ae744172 100644 --- 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; - if ( idx < nr_status_frames(d->grant_table) ) - *mfn = _mfn(virt_to_mfn(d->grant_table->status[idx])); - else - rc = -EINVAL; + if ( idx < nr_status_frames(gt) ) + mfn = _mfn(virt_to_mfn(gt->status[idx])); } else { - if ( (idx >= nr_grant_frames(d->grant_table)) && + if ( (idx >= nr_grant_frames(gt)) && (idx < max_grant_frames) ) gnttab_grow_table(d, idx + 1); - if ( idx < nr_grant_frames(d->grant_table) ) - *mfn = _mfn(virt_to_mfn(d->grant_table->shared_raw[idx])); - else - rc = -EINVAL; + if ( idx < nr_grant_frames(gt) ) + mfn = _mfn(virt_to_mfn(gt->shared_raw[idx])); } - gnttab_set_frame_gfn(d, idx, gfn); + return mfn; +} + +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); + + return mfn; +} + +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); + + *mfn = gnttab_get_frame_locked(d, idx); + if ( mfn_eq(*mfn, INVALID_MFN) ) + rc = -EINVAL; + else + gnttab_set_frame_gfn(d, idx, gfn); grant_write_unlock(d->grant_table); diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index 6395e8fd1d..3ccec250d8 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -613,6 +613,9 @@ void p2m_memory_type_changed(struct domain *d); int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start, unsigned long end); +/* Set foreign entry in the p2m table (for priv-mapping) */ +int set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn); + /* Set mmio addresses in the p2m table (for pass-through) */ int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn, unsigned int order, p2m_access_t access); diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h index 29386df98b..9bf58e7384 100644 --- 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; + /* 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 + * entry, gmfn_list will be populated with a list of GFNs + * that will be mapped to the MFNs of the resource. + */ + XEN_GUEST_HANDLE(xen_pfn_t) gmfn_list; +}; +typedef struct xen_mem_acquire_resource xen_mem_acquire_resource_t; + +#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */ + +/* Next available subop number is 29 */ #endif /* __XEN_PUBLIC_MEMORY_H__ */ diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h index 43ec6c4d80..f9e89375bb 100644 --- a/xen/include/xen/grant_table.h +++ b/xen/include/xen/grant_table.h @@ -136,6 +136,7 @@ static inline unsigned int grant_to_status_frames(int grant_frames) int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref, gfn_t *gfn, uint16_t *status); +mfn_t gnttab_get_frame(struct domain *d, unsigned long idx); int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn);