>>> On 19.03.18 at 16:34, <[email protected]> wrote:
>> From: Jan Beulich [mailto:[email protected]]
>> Sent: 19 March 2018 15:12
>>
>> >>> On 12.02.18 at 11:47, <[email protected]> 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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
