> -----Original Message-----
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 26 March 2018 12:55
> To: Paul Durrant <paul.durr...@citrix.com>
> Cc: JulienGrall <julien.gr...@arm.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Wei Liu <wei.l...@citrix.com>; George
> Dunlap <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>;
> Stefano Stabellini <sstabell...@kernel.org>; xen-devel@lists.xenproject.org;
> Konrad Rzeszutek Wilk <konrad.w...@oracle.com>; Tim (Xen.org)
> <t...@xen.org>
> Subject: Re: [PATCH v18 06/11] x86/hvm/ioreq: add a new mappable
> resource type...
>
> >>> On 22.03.18 at 12:55, <paul.durr...@citrix.com> 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 emulating domain's
> > heap. These pages will never be present in the P2M of the guest at any
> > point (and are not even shared with the guest) and so are not vulnerable to
> > any direct attack by the guest.
>
> "allocate pages from the emulating domain's heap" is a sub-optimal
> (at least slightly misleading) description, due to your use of
> MEMF_no_refcount together with the fact that domain's don't
> really have their own heaps.
>
Ok, I'll say 'allocate pages which are assigned to the emulating domain'
instead.
> > +static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf)
> > +{
> > + struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> > +
> > + if ( iorp->page )
> > + {
> > + /*
> > + * If a guest frame has already been mapped (which may happen
> > + * on demand if hvm_get_ioreq_server_info() is called), then
> > + * allocating a page is not permitted.
> > + */
> > + 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 safe because the emulating
> > + * domain cannot be destroyed until the ioreq server is destroyed.
> > + * Also 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(s->emulator, MEMF_no_refcount);
> > +
> > + if ( !iorp->page )
> > + return -ENOMEM;
> > +
> > + if ( !get_page_type(iorp->page, PGT_writable_page) )
> > + goto fail;
> > +
> > + iorp->va = __map_domain_page_global(iorp->page);
> > + if ( !iorp->va )
> > + goto fail;
> > +
> > + clear_page(iorp->va);
> > + return 0;
> > +
> > + fail:
> > + put_page_and_type(iorp->page);
>
> This is wrong in case it's the get_page_type() which failed.
>
Oh, I thought it was safe. I'll re-work the error path.
> > +int arch_acquire_resource(struct domain *d, unsigned int type,
> > + unsigned int id, unsigned long frame,
> > + unsigned int nr_frames, xen_pfn_t mfn_list[],
> > + unsigned int *flags)
> > +{
> > + int rc;
> > +
> > + switch ( type )
> > + {
> > + case XENMEM_resource_ioreq_server:
> > + {
> > + ioservid_t ioservid = id;
> > + unsigned int i;
> > +
> > + rc = -EINVAL;
> > + if ( id != (unsigned int)ioservid )
> > + break;
> > +
> > + rc = 0;
> > + for ( i = 0; i < nr_frames; i++ )
> > + {
> > + mfn_t mfn;
> > +
> > + rc = hvm_get_ioreq_server_frame(d, id, frame + i, &mfn);
> > + if ( rc )
> > + break;
> > +
> > + mfn_list[i] = mfn_x(mfn);
> > + }
> > +
> > + /*
> > + * The frames will be assigned to the tools domain that created
> > + * the ioreq server.
> > + */
>
> s/will be/have been/ and perhaps drop "tools"?
>
Ok.
> > --- a/xen/include/asm-arm/mm.h
> > +++ b/xen/include/asm-arm/mm.h
> > @@ -374,6 +374,14 @@ static inline void put_page_and_type(struct
> page_info *page)
> >
> > void clear_and_clean_page(struct page_info *page);
> >
> > +static inline int arch_acquire_resource(
> > + struct domain *d, unsigned int type, unsigned int id,
> > + unsigned long frame,unsigned int nr_frames, xen_pfn_t mfn_list[],
>
> Missing blank.
>
Ok.
Paul
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
