Message ID | 20170831093605.2757-13-paul.durrant@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Thu, Aug 31, 2017 at 10:36:05AM +0100, Paul Durrant wrote: > ... XENMEM_resource_ioreq_server > > This patch adds support for a new resource type that can be mapped using > the XENMEM_acquire_resource memory op. > > If an emulator makes use of this resource type then, instead of mapping > gfns, the IOREQ server will allocate pages from the heap. These pages > will never be present in the P2M of the guest at any point and so are > not vulnerable to any direct attack by the guest. They are only ever > accessible by Xen and any domain that has mapping privilege over the > guest (which may or may not be limited to the domain running the emulator). > > NOTE: Use of the new resource type is not compatible with use of > XEN_DMOP_get_ioreq_server_info unless the XEN_DMOP_no_gfns flag is > set. > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > Acked-by: George Dunlap <George.Dunlap@eu.citrix.com> > --- > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Tim Deegan <tim@xen.org> > Cc: Wei Liu <wei.liu2@citrix.com> > --- > xen/arch/x86/hvm/ioreq.c | 126 +++++++++++++++++++++++++++++++++++++++- > xen/arch/x86/mm.c | 27 +++++++++ > xen/include/asm-x86/hvm/ioreq.h | 2 + > xen/include/public/hvm/dm_op.h | 4 ++ > xen/include/public/memory.h | 3 + > 5 files changed, 161 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c > index 2d98b43849..5d406bc1fb 100644 > --- a/xen/arch/x86/hvm/ioreq.c > +++ b/xen/arch/x86/hvm/ioreq.c > @@ -241,6 +241,15 @@ static int hvm_map_ioreq_gfn(struct hvm_ioreq_server *s, bool buf) > struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq; > int rc; > > + if ( iorp->page ) > + { > + /* Make sure the page has not been allocated */ > + if ( gfn_eq(iorp->gfn, INVALID_GFN) ) > + return -EPERM; > + > + return 0; > + } > + > if ( d->is_dying ) > return -EINVAL; > > @@ -263,6 +272,60 @@ static int hvm_map_ioreq_gfn(struct hvm_ioreq_server *s, bool buf) > return rc; > } > > +static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf) > +{ > + struct domain *currd = current->domain; > + struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq; > + > + if ( iorp->page ) > + { > + /* Make sure the page has not been mapped */ > + if ( !gfn_eq(iorp->gfn, INVALID_GFN) ) > + return -EPERM; > + > + return 0; > + } > + > + /* > + * Allocated IOREQ server pages are assigned to the emulating > + * domain, not the target domain. This is because the emulator is > + * likely to be destroyed after the target domain has been torn > + * down, and we must use MEMF_no_refcount otherwise page allocation > + * could fail if the emulating domain has already reached its > + * maximum allocation. > + */ > + iorp->page = alloc_domheap_page(currd, MEMF_no_refcount); So AFAICT (correct me if I'm wrong), the number of pages that can be allocated here is limited by MAX_NR_IOREQ_SERVERS, each ioreq server can only have at most one page. > + if ( !iorp->page ) > + return -ENOMEM; > + > + get_page(iorp->page, currd); Hm, didn't we agree that this get_page was not needed? AFAICT you need this if you use MEMF_no_owner, because the page is not added to d->page_list. Thanks, Roger.
> -----Original Message----- > From: Roger Pau Monne > Sent: 04 September 2017 16:02 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: xen-devel@lists.xenproject.org; Stefano Stabellini > <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; Andrew Cooper > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim > (Xen.org) <tim@xen.org>; Jan Beulich <jbeulich@suse.com> > Subject: Re: [Xen-devel] [PATCH v3 12/12] x86/hvm/ioreq: add a new > mappable resource type... > > On Thu, Aug 31, 2017 at 10:36:05AM +0100, Paul Durrant wrote: > > ... XENMEM_resource_ioreq_server > > > > This patch adds support for a new resource type that can be mapped using > > the XENMEM_acquire_resource memory op. > > > > If an emulator makes use of this resource type then, instead of mapping > > gfns, the IOREQ server will allocate pages from the heap. These pages > > will never be present in the P2M of the guest at any point and so are > > not vulnerable to any direct attack by the guest. They are only ever > > accessible by Xen and any domain that has mapping privilege over the > > guest (which may or may not be limited to the domain running the > emulator). > > > > NOTE: Use of the new resource type is not compatible with use of > > XEN_DMOP_get_ioreq_server_info unless the XEN_DMOP_no_gfns > flag is > > set. > > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > > Acked-by: George Dunlap <George.Dunlap@eu.citrix.com> > > --- > > Cc: Jan Beulich <jbeulich@suse.com> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Cc: Stefano Stabellini <sstabellini@kernel.org> > > Cc: Tim Deegan <tim@xen.org> > > Cc: Wei Liu <wei.liu2@citrix.com> > > --- > > xen/arch/x86/hvm/ioreq.c | 126 > +++++++++++++++++++++++++++++++++++++++- > > xen/arch/x86/mm.c | 27 +++++++++ > > xen/include/asm-x86/hvm/ioreq.h | 2 + > > xen/include/public/hvm/dm_op.h | 4 ++ > > xen/include/public/memory.h | 3 + > > 5 files changed, 161 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c > > index 2d98b43849..5d406bc1fb 100644 > > --- a/xen/arch/x86/hvm/ioreq.c > > +++ b/xen/arch/x86/hvm/ioreq.c > > @@ -241,6 +241,15 @@ static int hvm_map_ioreq_gfn(struct > hvm_ioreq_server *s, bool buf) > > struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq; > > int rc; > > > > + if ( iorp->page ) > > + { > > + /* Make sure the page has not been allocated */ > > + if ( gfn_eq(iorp->gfn, INVALID_GFN) ) > > + return -EPERM; > > + > > + return 0; > > + } > > + > > if ( d->is_dying ) > > return -EINVAL; > > > > @@ -263,6 +272,60 @@ static int hvm_map_ioreq_gfn(struct > hvm_ioreq_server *s, bool buf) > > return rc; > > } > > > > +static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf) > > +{ > > + struct domain *currd = current->domain; > > + struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq; > > + > > + if ( iorp->page ) > > + { > > + /* Make sure the page has not been mapped */ > > + if ( !gfn_eq(iorp->gfn, INVALID_GFN) ) > > + return -EPERM; > > + > > + return 0; > > + } > > + > > + /* > > + * Allocated IOREQ server pages are assigned to the emulating > > + * domain, not the target domain. This is because the emulator is > > + * likely to be destroyed after the target domain has been torn > > + * down, and we must use MEMF_no_refcount otherwise page > allocation > > + * could fail if the emulating domain has already reached its > > + * maximum allocation. > > + */ > > + iorp->page = alloc_domheap_page(currd, MEMF_no_refcount); > > So AFAICT (correct me if I'm wrong), the number of pages that can be > allocated here is limited by MAX_NR_IOREQ_SERVERS, each ioreq server > can only have at most one page. > Each server can have at most 2 pages (one for synchronous ioreqs and one for buffered). > > + if ( !iorp->page ) > > + return -ENOMEM; > > + > > + get_page(iorp->page, currd); > > Hm, didn't we agree that this get_page was not needed? AFAICT you need > this if you use MEMF_no_owner, because the page is not added to > d->page_list. > Oh, good catch. I completely forgot to ditch that. Paul > Thanks, Roger.
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c index 2d98b43849..5d406bc1fb 100644 --- a/xen/arch/x86/hvm/ioreq.c +++ b/xen/arch/x86/hvm/ioreq.c @@ -241,6 +241,15 @@ static int hvm_map_ioreq_gfn(struct hvm_ioreq_server *s, bool buf) struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq; int rc; + if ( iorp->page ) + { + /* Make sure the page has not been allocated */ + if ( gfn_eq(iorp->gfn, INVALID_GFN) ) + return -EPERM; + + return 0; + } + if ( d->is_dying ) return -EINVAL; @@ -263,6 +272,60 @@ static int hvm_map_ioreq_gfn(struct hvm_ioreq_server *s, bool buf) return rc; } +static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf) +{ + struct domain *currd = current->domain; + struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq; + + if ( iorp->page ) + { + /* Make sure the page has not been mapped */ + if ( !gfn_eq(iorp->gfn, INVALID_GFN) ) + return -EPERM; + + return 0; + } + + /* + * Allocated IOREQ server pages are assigned to the emulating + * domain, not the target domain. This is because the emulator is + * likely to be destroyed after the target domain has been torn + * down, and we must use MEMF_no_refcount otherwise page allocation + * could fail if the emulating domain has already reached its + * maximum allocation. + */ + iorp->page = alloc_domheap_page(currd, MEMF_no_refcount); + if ( !iorp->page ) + return -ENOMEM; + + get_page(iorp->page, currd); + + iorp->va = __map_domain_page_global(iorp->page); + if ( !iorp->va ) + { + put_page(iorp->page); + iorp->page = NULL; + return -ENOMEM; + } + + clear_page(iorp->va); + return 0; +} + +static void hvm_free_ioreq_mfn(struct hvm_ioreq_server *s, bool buf) +{ + struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq; + + if ( !iorp->page ) + return; + + unmap_domain_page_global(iorp->va); + iorp->va = NULL; + + put_page(iorp->page); + iorp->page = NULL; +} + bool is_ioreq_server_page(struct domain *d, const struct page_info *page) { unsigned int id; @@ -472,6 +535,27 @@ static void hvm_ioreq_server_unmap_pages(struct hvm_ioreq_server *s) hvm_unmap_ioreq_gfn(s, false); } +static int hvm_ioreq_server_alloc_pages(struct hvm_ioreq_server *s) +{ + int rc = -ENOMEM; + + rc = hvm_alloc_ioreq_mfn(s, false); + + if ( !rc && (s->bufioreq_handling != HVM_IOREQSRV_BUFIOREQ_OFF) ) + rc = hvm_alloc_ioreq_mfn(s, true); + + if ( rc ) + hvm_free_ioreq_mfn(s, false); + + return rc; +} + +static void hvm_ioreq_server_free_pages(struct hvm_ioreq_server *s) +{ + hvm_free_ioreq_mfn(s, true); + hvm_free_ioreq_mfn(s, false); +} + static void hvm_ioreq_server_free_rangesets(struct hvm_ioreq_server *s) { unsigned int i; @@ -598,7 +682,18 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s, fail_add: hvm_ioreq_server_remove_all_vcpus(s); + + /* + * NOTE: It is safe to call both hvm_ioreq_server_unmap_pages() and + * hvm_ioreq_server_free_pages() in that order. + * This is because the former will do nothing if the pages + * are not mapped, leaving the page to be freed by the latter. + * However if the pages are mapped then the former will set + * the page_info pointer to NULL, meaning the latter will do + * nothing. + */ hvm_ioreq_server_unmap_pages(s); + hvm_ioreq_server_free_pages(s); return rc; } @@ -608,6 +703,7 @@ static void hvm_ioreq_server_deinit(struct hvm_ioreq_server *s) ASSERT(!s->enabled); hvm_ioreq_server_remove_all_vcpus(s); hvm_ioreq_server_unmap_pages(s); + hvm_ioreq_server_free_pages(s); hvm_ioreq_server_free_rangesets(s); } @@ -738,7 +834,8 @@ int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id, goto out; } - *ioreq_gfn = gfn_x(s->ioreq.gfn); + if ( ioreq_gfn ) + *ioreq_gfn = gfn_x(s->ioreq.gfn); if ( HANDLE_BUFIOREQ(s) ) { @@ -756,6 +853,33 @@ int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id, return rc; } +mfn_t hvm_get_ioreq_server_frame(struct domain *d, ioservid_t id, + unsigned int idx) +{ + struct hvm_ioreq_server *s; + mfn_t mfn = INVALID_MFN; + + spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock); + + s = d->arch.hvm_domain.ioreq_server.server[id]; + + if ( id >= MAX_NR_IOREQ_SERVERS || !s || IS_DEFAULT(s) ) + goto out; + + if ( hvm_ioreq_server_alloc_pages(s) ) + goto out; + + if ( idx == 0 ) + mfn = _mfn(page_to_mfn(s->bufioreq.page)); + else if ( idx == 1 ) + mfn = _mfn(page_to_mfn(s->ioreq.page)); + + out: + spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock); + + return mfn; +} + int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id, uint32_t type, uint64_t start, uint64_t end) diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 3b5f2e9b80..0abd865364 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -122,6 +122,7 @@ #include <asm/fixmap.h> #include <asm/io_apic.h> #include <asm/pci.h> +#include <asm/hvm/ioreq.h> #include <asm/hvm/grant_table.h> #include <asm/pv/grant_table.h> @@ -4716,6 +4717,27 @@ static int xenmem_acquire_grant_table(struct domain *d, return 0; } +static int xenmem_acquire_ioreq_server(struct domain *d, + unsigned int id, + unsigned long frame, + unsigned long nr_frames, + unsigned long mfn_list[]) +{ + unsigned int i; + + for ( i = 0; i < nr_frames; i++ ) + { + mfn_t mfn = hvm_get_ioreq_server_frame(d, id, 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; @@ -4750,6 +4772,11 @@ static int xenmem_acquire_resource(xen_mem_acquire_resource_t *xmar) mfn_list); break; + case XENMEM_resource_ioreq_server: + rc = xenmem_acquire_ioreq_server(d, xmar->id, xmar->frame, + xmar->nr_frames, mfn_list); + break; + default: rc = -EOPNOTSUPP; break; diff --git a/xen/include/asm-x86/hvm/ioreq.h b/xen/include/asm-x86/hvm/ioreq.h index 1829fcf43e..032aeb6fa9 100644 --- a/xen/include/asm-x86/hvm/ioreq.h +++ b/xen/include/asm-x86/hvm/ioreq.h @@ -31,6 +31,8 @@ int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id, unsigned long *ioreq_gfn, unsigned long *bufioreq_gfn, evtchn_port_t *bufioreq_port); +mfn_t hvm_get_ioreq_server_frame(struct domain *d, ioservid_t id, + unsigned int idx); int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id, uint32_t type, uint64_t start, uint64_t end); diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h index 9677bd74e7..59b6006910 100644 --- a/xen/include/public/hvm/dm_op.h +++ b/xen/include/public/hvm/dm_op.h @@ -90,6 +90,10 @@ struct xen_dm_op_create_ioreq_server { * the frame numbers passed back in gfns <ioreq_gfn> and <bufioreq_gfn> * respectively. (If the IOREQ Server is not handling buffered emulation * only <ioreq_gfn> will be valid). + * + * NOTE: To access the synchronous ioreq structures and buffered ioreq + * ring, it is preferable to use the XENMEM_acquire_resource memory + * op specifying resource type XENMEM_resource_ioreq_server. */ #define XEN_DMOP_get_ioreq_server_info 2 diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h index 9bf58e7384..716941dc0c 100644 --- a/xen/include/public/memory.h +++ b/xen/include/public/memory.h @@ -664,10 +664,13 @@ struct xen_mem_acquire_resource { uint16_t type; #define XENMEM_resource_grant_table 0 +#define XENMEM_resource_ioreq_server 1 /* * IN - a type-specific resource identifier, which must be zero * unless stated otherwise. + * + * type == XENMEM_resource_ioreq_server -> id == ioreq server id */ uint32_t id; /* IN - number of (4K) frames of the resource to be mapped */