Re: [Xen-devel] [PATCH 13/25] argo: implement the register op

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Dec 21, 2018 at 03:05:03PM -0800, Christopher Clark wrote:
> On Thu, Dec 20, 2018 at 4:52 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> >
> > On Wed, Dec 19, 2018 at 09:41:59PM -0800, Christopher Clark wrote:
> > > On Wed, Dec 12, 2018 at 8:48 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> 
> > > wrote:
> > > >
> > > > On Fri, Nov 30, 2018 at 05:32:52PM -0800, Christopher Clark wrote:
> > Then I wonder why you need such check in any case if the code can
> > handle such cases, the more than the check itself is racy.
> 
> OK, so at the root of the question here is: does it matter what the p2m
> type of the memory is at these points:
> 
> 1) when the gfn is translated to mfn, at the time of ring registration

This is the important check, because that's where you should take a
reference to the page. In this case you should check that the page is
of ram_rw type.

> 2) when the hypervisor writes into guest memory:
>     - where the tx_ptr index is initialized in the register op
>     - where ringbuf data is written in sendv
>     - where ring description data is written in notify

As long as you keep a reference to the pages that are part of the ring
you don't need to do any checks when writing/reading from them. If the
guest messes up it's p2m and does change the gfn -> mfn mappings for
pages that are part of the ring that's the guest problem, the
hypervisor still has a reference to those pages so they won't be
reused.

> or is having PGT_writable_page type and ownership by the domain
> sufficient?
> 
> For 1), I think there's some use in saying no to a guest that has
> supplied a region that appears misconfigured.

Sure, when you setup the ring in Xen you should execute all the checks
and make sure the ring pages are of type ram_rw and take a reference
to each page.

> For 2), input would be appreciated. It currently works under the
> assumption that a p2m type check is unnecessary, which is why the
> put_gfn is where it is.

Right, and this should go away since it's confusing and unnecessary
AFAICT.

> For further background context, here's my understanding of this section:
> 
> When the guest invokes the hypercall operation to register a ring, it
> identifies the memory that it owns and wants the hypervisor to use by
> supplying an array of gfns (or in v2, addresses which are shifted to
> extract their gfns).
> 
> The hypervisor translates from gfns to mfns, using the translation that
> exists at that time, and then refers to that memory internally by mfn
> from there on out. This find_ring_mfn function is where the gfn->mfn
> translation happens.  (The variable name does needs renaming from pfn,
> as you noted - thanks.)
> 
> To do the translation from gfn to mfn, (on x86) it's using
> get_gfn_unshare. That's doing three things:
> * returns the mfn, if there is one.
> * returns the p2m type of that memory.
> * acquires a reference to that gfn, which needs to be dropped at some
>   point.
> 
> The p2m type type check on the gfn in find_ring_mfn at that time is
> possibly conservative, rejecting more types than perhaps it needs to,
> but the type that it accepts (p2m_ram_rw) is sane. It is a validation of
> the p2m type at that instant, intended to detect if the guest has
> supplied memory to the ring register op that does not make sense for it
> to use as a ring, as indicated by the current p2m type, and if so, fail
> early, or indicate that a retry later is needed.
> 
> Then the get_page_and_type call is where the memory identified by the
> mfn that was just obtained, gets locked to PGT_writable_page type, and
> ownership fixed to its current owner domain, by adding to its reference
> count.

Here you should make sure the get_page is performed while the gfn is
still locked, and once you have a reference to the page you can unlock
the gfn. As said above, if the guest then wants to mess up with the
gfn -> mfn mapping that's fine, the hypervisor already has a
reference to the underlying original memory page.

> Then the gfn reference count is dropped with the put_gfn call. This
> means that the guest can elect to change the p2m type afterwards, if it
> wants; (any change needs to be consistent with its domain ownership and
> PGT_writable_page type though -- not sure if that constrains possible types).

Hm, not really get_gfn / put_gfn doesn't actually take references, but
rather pick the p2m lock. What takes references is get_page.

> That memory can have guest-supplied data written into it, either by the
> domain owning the page itself, or in response to argo sendv operations
> by other domains that are authorized to transmit into the ring.
> 
> Your note that the "check itself is racy": ie. that a change of p2m type
> could occur immediately afterwards is true.
> 
> So: Do you think that a check on the current p2m type of the pages in
> the ring is needed at the points where the hypervisor issues writes into
> that ring memory?

No, as long as you have a reference to the page (note: not the gfn)
that you are writing to it should be fine.

Roger.

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