Re: [PATCH v4 02/12] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel