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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.03.18 at 16:34, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 19 March 2018 15:12
>> 
>> >>> On 12.02.18 at 11:47, <paul.durrant@xxxxxxxxxx> 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?
> 
> Hmm. I think we're ok. The calls just play with the CPU local flush disable 
> flag so they should only disable anything resulting from the current 
> hypercall. Manipulation of other IOMMU page tables (on behalf of other 
> domains) should not be affected AFAICT. I'll double check though.

Just think about the caller doing an unmap (which drops the page
ref) but never doing a flush. If the dropped ref was the last one,
the page will be freed before the caller even has a chance to issue
a flush.

>> > +    /*
>> > +     * 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)
>> 
> 
> I wanted the 'not implemented' case to be distinct from the 'not supported 
> because of some configuration detail' case, which is why I chose ENOSYS. I'll 
> change it if you don't think that matters though.

Distinguishing those two cases is perhaps indeed worthwhile, but
for ENOSYS we had the discussion multiple times, and I think we've
finally converged to this being intended to only be returned for
out of range hypercall numbers (not even sub-function ones). Of
course there continue to be many violators, but we'll try to not
allow in new ones.

>> 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.
> 
> Ok. I'll just check for all map, unmap and lookup in both cases.

Well, the unmap path probably doesn't need to check the map
hook.

>> > +    /* 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?
>> 
> 
> I don't think so. If we're doing a readonly mapping then the page should not 
> be forcibly populated, right?

I view it the other way around - no matter what mapping, the
page should be populated. If it's a writable one, an existing
page also needs to be unshared.

>> > +    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?
>> 
> 
> I'm confused.

Actually, if you request UNSHARE, you'll get back a shared type
only together with NULL for the page. See e.g. get_paged_frame()
in common/grant_table.c. There you'll also find an example of the
inverted use of the request types compared to what you have.

>> > +    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.
>> 
> 
> Yes, I guess I need to ensure that only non-RAM (i.e. RMRR and E820 reserved 
> areas) are mapped through the IOMMU or this could indeed be abused.

Now I'm confused - then you don't need to deal with struct page_info
and page references at all. Nor would you need to call
get_page_from_gfn() and check p2m_is_any_ram(). Also - what use
would the interface be if you couldn't map any RAM?

Jan

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