Re: [PATCH v9 1/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing

From: Daniel De Graaf <dgdegra@tycho.nsa.gov>
To: Stefano Stabellini <sstabellini@kernel.org>, xen-devel@lists.xen.org
Cc: Stefano Stabellini <stefanos@xilinx.com>,
	wei.liu2@citrix.com, blackskygg@gmail.com,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	ian.jackson@eu.citrix.com, Tim Deegan <tim@xen.org>,
	julien.grall@arm.com, Jan Beulich <jbeulich@suse.com>
Subject: Re: [PATCH v9 1/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing
Date: Fri, 7 Dec 2018 14:33:08 -0500	[thread overview]
Message-ID: <9df60941-02c3-8167-6ee0-2bc7ab63335b@tycho.nsa.gov> (raw)
In-Reply-To: <1544048163-27499-1-git-send-email-sstabellini@kernel.org>

On 12/5/18 5:15 PM, Stefano Stabellini wrote:
> From: Zhongze Liu <blackskygg@gmail.com>
> 
> The existing XENMAPSPACE_gmfn_foreign subop of XENMEM_add_to_physmap forbids
> a Dom0 to map memory pages from one DomU to another, which restricts some useful
> yet not dangerous use cases -- such as sharing pages among DomU's so that they
> can do shm-based communication.
> 
> This patch introduces XENMAPSPACE_gmfn_share to address this inconvenience,
> which is mostly the same as XENMAPSPACE_gmfn_foreign but has its own xsm check.
> 
> Specifically, the patch:
> 
> * Introduces a new av permission MMU__SHARE_MEM to denote if two domains can
>    share memory by using the new subop;
> * Introduces xsm_map_gmfn_share() to check if (current) has proper permission
>    over (t) AND MMU__SHARE_MEM is allowed between (d) and (t);
> * Modify the default xen.te to allow MMU__SHARE_MEM for normal domains that
>    allow grant mapping/event channels.
> 
> The new subop is marked unsupported for x86 because calling p2m_add_foregin
> on two DomU's is currently not supported on x86.
> 
> This is for the proposal "Allow setting up shared memory areas between VMs
> from xl config file" (see [1]).
> 
> [1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html
> 
> Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>[...]
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index b0ac1f6..9d109b0 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -535,6 +535,20 @@ static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str
>      return xsm_default_action(action, d, t);
>  }
>  
> +/*
> + * Be aware that this is not an exact default equivalence of its flask
> + * variant which also checks if @d and @t "are allowed to share memory
> + * pages", for now, we don't have a proper default equivalence of such a
> + * check.
> + */
> +static XSM_INLINE int xsm_map_gmfn_share(XSM_DEFAULT_ARG struct domain *d,
> +                                         struct domain *t)
> +{
> +    XSM_ASSERT_ACTION(XSM_TARGET);
> +    return xsm_default_action(action, current->domain, d) ?:
> +           xsm_default_action(action, current->domain, t);
> +}

In all of the callers that I checked, we've already made a call to the
xsm_add_to_physmap hook checking that (current) can modify (d), so the
check here is redundant.  If it's useful to keep the redundant check in
case another caller is added later (or if there's one I missed), it would
also be useful to re-verify the MMU__PHYSMAP permission in the flask code
so that the checks remain equivalent.

If you want the comment on the (d,t) check documented in code, the XSM_HOOK
action is a useful no-op.

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