Message ID | 20170905113716.3960-13-paul.durrant@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, Sep 05, 2017 at 12:37:16PM +0100, Paul Durrant wrote: > > +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]; > + Check id < MAX_NR_IOREQ_SERVERS before getting s? > + 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)); Does the caller care about the order? If so this should be documented? The rest looks sensible but I haven't reviewed in detail. > + > + out: > + spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock); > + > + return mfn; > +} > +
On Tue, Sep 05, 2017 at 12:37:16PM +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 | 123 +++++++++++++++++++++++++++++++++++++++- > 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, 158 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c > index 158bfbba32..f4c06a7a2a 100644 > --- a/xen/arch/x86/hvm/ioreq.c > +++ b/xen/arch/x86/hvm/ioreq.c > @@ -250,6 +250,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; > > @@ -272,6 +281,57 @@ 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 */ Maybe it's just me being slightly lost, but here you imply that iorp->gfn being != INVALID_GFN means the page is mapped, while above you mention allocated for the same check. Is it because gfn has different usages depending on the context? > + if ( !gfn_eq(iorp->gfn, INVALID_GFN) ) > + return -EPERM; [...] > +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]; Can't you use GET_IOREQ_SERVER here? Thanks, Roger.
On Thu, Sep 07, 2017 at 03:56:16PM +0100, Roger Pau Monné wrote: > On Tue, Sep 05, 2017 at 12:37:16PM +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 | 123 +++++++++++++++++++++++++++++++++++++++- > > 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, 158 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c > > index 158bfbba32..f4c06a7a2a 100644 > > --- a/xen/arch/x86/hvm/ioreq.c > > +++ b/xen/arch/x86/hvm/ioreq.c > > @@ -250,6 +250,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; > > > > @@ -272,6 +281,57 @@ 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 */ > > Maybe it's just me being slightly lost, but here you imply that > iorp->gfn being != INVALID_GFN means the page is mapped, while above > you mention allocated for the same check. > > Is it because gfn has different usages depending on the context? > Haha, I'm not the only one who got confused while reading the patch. ;-) That's because the "page" can be obtained via two different methods. "Allocated" means getting it from xen heap, "mapped" means mapping guest page afaict.
On Thu, Sep 07, 2017 at 03:59:05PM +0100, Wei Liu wrote: > On Thu, Sep 07, 2017 at 03:56:16PM +0100, Roger Pau Monné wrote: > > On Tue, Sep 05, 2017 at 12:37:16PM +0100, Paul Durrant wrote: [...] > > > + /* 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; > > > > > > @@ -272,6 +281,57 @@ 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 */ > > > > Maybe it's just me being slightly lost, but here you imply that > > iorp->gfn being != INVALID_GFN means the page is mapped, while above > > you mention allocated for the same check. > > > > Is it because gfn has different usages depending on the context? > > > > Haha, I'm not the only one who got confused while reading the patch. ;-) > > That's because the "page" can be obtained via two different methods. > > "Allocated" means getting it from xen heap, "mapped" means mapping guest > page afaict. Oh, thanks. I think the comment should be slightly expanded then to give more context, or else someone else is likely to trip over this again IMHO. Roger.
> -----Original Message----- > From: Wei Liu [mailto:wei.liu2@citrix.com] > Sent: 07 September 2017 15:51 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: xen-devel@lists.xenproject.org; Jan Beulich <jbeulich@suse.com>; > Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson > <Ian.Jackson@citrix.com>; Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Tim > (Xen.org) <tim@xen.org>; Wei Liu <wei.liu2@citrix.com> > Subject: Re: [PATCH v4 12/12] x86/hvm/ioreq: add a new mappable resource > type... > > On Tue, Sep 05, 2017 at 12:37:16PM +0100, Paul Durrant wrote: > > > > +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]; > > + > > Check id < MAX_NR_IOREQ_SERVERS before getting s? Oh, I should be using my new macro here. Good spot. > > > + 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)); > > Does the caller care about the order? If so this should be documented? > True. I should document that 0 is the buffered frame and 1+ are synchronous frames (of which there is only one at the moment but this will need to change if we support more vcpus). > The rest looks sensible but I haven't reviewed in detail. > Ok. Paul > > + > > + out: > > + spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock); > > + > > + return mfn; > > +} > > +
> -----Original Message----- > From: Roger Pau Monne > Sent: 07 September 2017 16:24 > To: Wei Liu <wei.liu2@citrix.com> > Cc: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org; > Stefano Stabellini <sstabellini@kernel.org>; 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 v4 12/12] x86/hvm/ioreq: add a new > mappable resource type... > > On Thu, Sep 07, 2017 at 03:59:05PM +0100, Wei Liu wrote: > > On Thu, Sep 07, 2017 at 03:56:16PM +0100, Roger Pau Monné wrote: > > > On Tue, Sep 05, 2017 at 12:37:16PM +0100, Paul Durrant wrote: > [...] > > > > + /* 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; > > > > > > > > @@ -272,6 +281,57 @@ 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 */ > > > > > > Maybe it's just me being slightly lost, but here you imply that > > > iorp->gfn being != INVALID_GFN means the page is mapped, while above > > > you mention allocated for the same check. > > > > > > Is it because gfn has different usages depending on the context? > > > > > > > Haha, I'm not the only one who got confused while reading the patch. ;-) > > > > That's because the "page" can be obtained via two different methods. > > > > "Allocated" means getting it from xen heap, "mapped" means mapping > guest > > page afaict. > > Oh, thanks. I think the comment should be slightly expanded then to > give more context, or else someone else is likely to trip over this > again IMHO. Yes, Wei's explanation is correct. I will expand the comments in both places to explain they are mutually exclusive methods of getting hold of ioreq server pages. Paul > > Roger.
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c index 158bfbba32..f4c06a7a2a 100644 --- a/xen/arch/x86/hvm/ioreq.c +++ b/xen/arch/x86/hvm/ioreq.c @@ -250,6 +250,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; @@ -272,6 +281,57 @@ 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; + + iorp->va = __map_domain_page_global(iorp->page); + if ( !iorp->va ) + { + 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) { const struct hvm_ioreq_server *s; @@ -478,6 +538,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; @@ -604,7 +685,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; } @@ -614,6 +706,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); } @@ -745,7 +838,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) ) { @@ -763,6 +857,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 b125b85bf9..6c95c58675 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> @@ -4738,6 +4739,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; @@ -4772,6 +4794,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 */