Re: [V3 PATCH 7/9] pvh: change xsm_add_to_physmap

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Wed, 27 Nov 2013 11:46:27 -0500
Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:

> On 11/26/2013 09:27 PM, Mukesh Rathor wrote:
> > In preparation for the next patch, we update xsm_add_to_physmap to
> > allow for checking of foreign domain. Thus, the current domain must
> > have the right to update the mappings of target domain with pages
> > from foreign domain.
> >
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> > CC: dgdegra@tycho.nsa.gov
> > ---
> >   xen/arch/x86/mm.c       |   16 +++++++++++++---
> >   xen/include/xsm/dummy.h |   10 ++++++++--
> >   xen/include/xsm/xsm.h   |    6 +++---
> >   xen/xsm/flask/hooks.c   |   10 ++++++++--
> >   4 files changed, 32 insertions(+), 10 deletions(-)
> 
> The XSM changes look good; however, the calling code needs a bit of
> tweaking.  Currently, if domain 0 is specified as the foreign domain,
> the check is skipped, and the check is also run unnecessarily when
> foreign_domid is nonzero but the operation is not
> XENMAPSPACE_gmfn_foreign. The locking in this version also implies a
> potential TOCTOU bug, but which in reality is impossible to trigger
> due to the existing RCU lock held on (d).  I would suggest passing
> the foreign struct domain instead of the domid, as below.
> 
> An unrelated question about XENMAPSPACE_gmfn_foreign that came up
> while looking at this: is the domain parameter (d) supposed to be
> ignored here, with maps always modifying the current domain? I would
> have expected this call to manipulate d's physmap, with the common
> case being (d == current->domain).

Right, that is the assumption at present that PVH dom0 is creating
user domains. Perhaps a debug assert would be a good idea. I can add 
that. It's probably straightforward to just manipulate d's physmap,
but I'd rather do it when it's really time for it, than to anticipate
something I can't test right now.

thanks for your input.
Mukesh