Re: [PATCH 7/7] x86: add iommu_ops to map and unmap pages, and also to flush the IOTLB

["Jan Beulich" <JBeulich@suse.com>]

>>> On 12.02.18 at 11:47, <paul.durrant@citrix.com> wrote:
> This patch adds iommu_ops to allow a domain with control_iommu privilege
> to map and unmap pages from any guest over which it has mapping privilege
> in the IOMMU.
> These operations implicitly disable IOTLB flushing so that the caller can
> batch operations and then explicitly flush the IOTLB using the iommu_op
> also added by this patch.

Can't this be abused for unmaps?

> --- a/xen/arch/x86/iommu_op.c
> +++ b/xen/arch/x86/iommu_op.c
> @@ -24,6 +24,174 @@
>  #include <xen/hypercall.h>
>  #include <xen/iommu.h>
>  
> +/* Override macros from asm/page.h to make them work with mfn_t */
> +#undef mfn_to_page
> +#define mfn_to_page(mfn) __mfn_to_page(mfn_x(mfn))
> +#undef page_to_mfn
> +#define page_to_mfn(page) _mfn(__page_to_mfn(page))

I guess with Julien's this needs to go away, but it looks like his
series hasn't landed yet.

> +struct check_rdm_ctxt {
> +    bfn_t bfn;
> +};
> +
> +static int check_rdm(xen_pfn_t start, xen_ulong_t nr, u32 id, void *arg)

uint32_t

> +{
> +    struct check_rdm_ctxt *ctxt = arg;
> +
> +    if ( bfn_x(ctxt->bfn) >= start &&
> +         bfn_x(ctxt->bfn) < start + nr )
> +        return -EINVAL;

Something more distinguishable than EINVAL would certainly be
nice here. Also how come this check does not depend on the
domain? Only RMRRs of devices owned by a domain are relevant
in the BFN range (unless I still didn't fully understand how BFN is
meant to be different from GFN and MFN).

> +static int iommuop_map(struct xen_iommu_op_map *op, unsigned int flags)
> +{
> +    struct domain *d, *od, *currd = current->domain;
> +    struct domain_iommu *iommu = dom_iommu(currd);
> +    const struct iommu_ops *ops = iommu->platform_ops;
> +    domid_t domid = op->domid;
> +    gfn_t gfn = _gfn(op->gfn);
> +    bfn_t bfn = _bfn(op->bfn);
> +    mfn_t mfn;
> +    struct check_rdm_ctxt ctxt = {
> +        .bfn = bfn,
> +    };
> +    p2m_type_t p2mt;
> +    p2m_query_t p2mq;
> +    struct page_info *page;
> +    unsigned int prot;
> +    int rc;
> +
> +    if (op->pad0 != 0 || op->pad1 != 0)

Missing blanks again (and please again consider dropping the " != 0").

> +        return -EINVAL;
> +
> +    /*
> +     * Both map_page and lookup_page operations must be implemented.
> +     * The lookup_page method is not used here but is relied upon by
> +     * iommuop_unmap() to drop the page reference taken here.
> +     */
> +    if ( !ops->map_page || !ops->lookup_page )
> +        return -ENOSYS;

EOPNOTSUPP (also further down)

Also how about the unmap hook? If that's not implemented, how
would the page ref obtained below ever be dropped again? Or
you may need to re-order the unmap side code.

> +    /* Check whether the specified BFN falls in a reserved region */
> +    rc = iommu_get_reserved_device_memory(check_rdm, &ctxt);
> +    if ( rc )
> +        return rc;
> +
> +    d = rcu_lock_domain_by_any_id(domid);
> +    if ( !d )
> +        return -ESRCH;
> +
> +    p2mq = (flags & XEN_IOMMUOP_map_readonly) ?
> +        P2M_UNSHARE : P2M_ALLOC;

Isn't this the wrong way round?

> +    page = get_page_from_gfn(d, gfn_x(gfn), &p2mt, p2mq);
> +
> +    rc = -ENOENT;
> +    if ( !page )
> +        goto unlock;
> +
> +    if ( p2m_is_paged(p2mt) )
> +    {
> +        p2m_mem_paging_populate(d, gfn_x(gfn));
> +        goto release;
> +    }
> +
> +    if ( (p2mq & P2M_UNSHARE) && p2m_is_shared(p2mt) )
> +        goto release;

Same for this check then?

> +    /*
> +     * Make sure the page is RAM and, if it is read-only, that the
> +     * read-only flag is present.
> +     */
> +    rc = -EPERM;
> +    if ( !p2m_is_any_ram(p2mt) ||
> +         (p2m_is_readonly(p2mt) && !(flags & XEN_IOMMUOP_map_readonly)) )
> +        goto release;

Don't you also need to obtain a PGT_writable reference in the
"not r/o" case?

> +    /*
> +     * If the calling domain does not own the page then make sure it
> +     * has mapping privilege over the page owner.
> +     */
> +    od = page_get_owner(page);
> +    if ( od != currd )
> +    {
> +        rc = xsm_domain_memory_map(XSM_TARGET, od);
> +        if ( rc )
> +            goto release;
> +    }

With XSM_TARGET I don't see the point of the if() around here.
Perhaps simply

        rc = xsm_domain_memory_map(XSM_TARGET, page_get_owner(page));

?

> +static int iommuop_unmap(struct xen_iommu_op_unmap *op)
> +{
> +    struct domain *currd = current->domain;
> +    struct domain_iommu *iommu = dom_iommu(currd);
> +    const struct iommu_ops *ops = iommu->platform_ops;
> +    bfn_t bfn = _bfn(op->bfn);
> +    mfn_t mfn;
> +    struct check_rdm_ctxt ctxt = {
> +        .bfn = bfn,
> +    };
> +    unsigned int flags;
> +    struct page_info *page;
> +    int rc;
> +
> +    /*
> +     * Both unmap_page and lookup_page operations must be implemented.
> +     */

Single line comment (there are more below).

> +    if ( !ops->unmap_page || !ops->lookup_page )
> +        return -ENOSYS;
> +
> +    /* Check whether the specified BFN falls in a reserved region */
> +    rc = iommu_get_reserved_device_memory(check_rdm, &ctxt);
> +    if ( rc )
> +        return rc;
> +
> +    if ( ops->lookup_page(currd, bfn, &mfn, &flags) ||
> +         !mfn_valid(mfn) )
> +        return -ENOENT;
> +
> +    page = mfn_to_page(mfn);
> +
> +    if ( ops->unmap_page(currd, bfn) )
> +        return -EIO;

How are you making sure this is a mapping that was established via
the map op? Without that this can be (ab)used to ...

> +    put_page(page);

... underflow the refcount of a page.

> +    return 0;

Blank line above here please.

> @@ -101,6 +269,22 @@ static void iommu_op(xen_iommu_op_t *op)
>          op->status = iommuop_query_reserved(&op->u.query_reserved);
>          break;
>  
> +    case XEN_IOMMUOP_map:
> +        this_cpu(iommu_dont_flush_iotlb) = 1;
> +        op->status = iommuop_map(&op->u.map, op->flags);
> +        this_cpu(iommu_dont_flush_iotlb) = 0;

true/false would be better in new code, even if the type of the
variable is still bool_t.

> --- a/xen/include/public/iommu_op.h
> +++ b/xen/include/public/iommu_op.h
> @@ -57,13 +57,50 @@ struct xen_iommu_op_query_reserved {
>      XEN_GUEST_HANDLE(xen_iommu_reserved_region_t) regions;
>  };
>  
> +/*
> + * XEN_IOMMUOP_map: Map a page in the IOMMU.
> + */
> +#define XEN_IOMMUOP_map 2
> +
> +struct xen_iommu_op_map {
> +    /* IN - The IOMMU frame number which will hold the new mapping */
> +    xen_bfn_t bfn;
> +    /* IN - The guest frame number of the page to be mapped */
> +    xen_pfn_t gfn;
> +    /* IN - The domid of the guest */

"... owning the page"

> +    domid_t domid;
> +    unsigned short pad0;
> +    unsigned int pad1;
> +};

No built in batching here? Also fixed width types again please.

> +/*
> + * XEN_IOMMUOP_flush: Flush the IOMMU TLB.
> + */
> +#define XEN_IOMMUOP_flush 4

No inputs here at all makes this a rather simple interface, but makes
single-page updates quite expensive.

>  struct xen_iommu_op {
>      uint16_t op;
>      uint16_t flags; /* op specific flags */
> +
> +#define _XEN_IOMMUOP_map_readonly 0
> +#define XEN_IOMMUOP_map_readonly (1 << (_XEN_IOMMUOP_map_readonly))

Perhaps better have this next to the map op?

Jan

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