Re: [Xen-devel] [PATCH v18 06/11] x86/hvm/ioreq: add a new mappable resource type...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 26 March 2018 12:55
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: JulienGrall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; George
> Dunlap <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org)
> <tim@xxxxxxx>
> Subject: Re: [PATCH v18 06/11] x86/hvm/ioreq: add a new mappable
> resource type...
> 
> >>> On 22.03.18 at 12:55, <paul.durrant@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel