Re: [Xen-devel] [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 10/19/2017 08:55 PM, Zhongze Liu wrote:
> 2017-10-20 8:34 GMT+08:00 Zhongze Liu <blackskygg@xxxxxxxxx>:
> > Hi Daniel,
> >
> > 2017-10-20 1:36 GMT+08:00 Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>:
> > > On 10/18/2017 10:36 PM, Zhongze Liu wrote:
> > > > The original dummy xsm_map_gmfn_foregin checks if source domain has the
> > > > proper
> > > > privileges over the target domain. Under this policy, it's not allowed if
> > > > a Dom0
> > > > wants to map pages from one DomU to another, which restricts some useful
> > > > yet not
> > > > dangerous use cases of the API, such as sharing pages among DomU's by
> > > > calling
> > > > XENMEM_add_to_physmap from Dom0.
> > > >
> > > > For the dummy xsm_map_gmfn_foregin, change to policy to: IFF the current
> > > > domain
> > > > has the proper privileges on (d) and (t), grant the access.
> > > >
> > > > For the flask side: 1) Introduce a new av permission MMU__SHARE_MEM to
> > > > denote if
> > > > two domains can share memory through map_gmfn_foregin. 2) Change to hook
> > > > to
> > > > grant the access IFF the current domain has proper MMU privileges on (d)
> > > > and (t),
> > > > and MMU__SHARE_MEM is allowed between (d) and (t). 3) Modify the default
> > > > xen.te
> > > > to allow MMU__SHARE_MEM for normal domains that allow grant mapping/event
> > > > channels.
> > > >
> > > > 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@xxxxxxxxx>
> > > >
> > > > Cc: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> > > > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > > > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > > > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > > Cc: Julien Grall <julien.grall@xxxxxxx>
> > > > Cc: xen-devel@xxxxxxxxxxxxx
> > > > ---
> > > >     V3:
> > > >     * Change several if statements to the GCC '... = a ?: b' extention.
> > > >     * lookup the current domain in the hooks instead of passing it as an
> > > > arg
> > > > ---
> > > >    tools/flask/policy/modules/xen.if   | 2 ++
> > > >    xen/include/xsm/dummy.h             | 3 ++-
> > > >    xen/xsm/flask/hooks.c               | 4 +++-
> > > >    xen/xsm/flask/policy/access_vectors | 4 ++++
> > > >    4 files changed, 11 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/tools/flask/policy/modules/xen.if
> > > > b/tools/flask/policy/modules/xen.if
> > > > index 55437496f6..3ffd1c6239 100644
> > > > --- a/tools/flask/policy/modules/xen.if
> > > > +++ b/tools/flask/policy/modules/xen.if
> > > > @@ -127,6 +127,8 @@ define(`domain_comms', `
> > > >          domain_event_comms($1, $2)
> > > >          allow $1 $2:grant { map_read map_write copy unmap };
> > > >          allow $2 $1:grant { map_read map_write copy unmap };
> > > > +       allow $1 $2:mmu share_mem;
> > > > +       allow $2 $1:mmu share_mem;
> > > >    ')
> > > >      # domain_self_comms(domain)
> > > > diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> > > > index b2cd56cdc5..65e7060ad5 100644
> > > > --- a/xen/include/xsm/dummy.h
> > > > +++ b/xen/include/xsm/dummy.h
> > > > @@ -516,7 +516,8 @@ static XSM_INLINE int
> > > > xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
> > > >    static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain
> > > > *d, struct domain *t)
> > > >    {
> > > >        XSM_ASSERT_ACTION(XSM_TARGET);
> > > > -    return xsm_default_action(action, d, t);
> > > > +    return xsm_default_action(action, current->domain, d) ?:
> > > > +        xsm_default_action(action, current->domain, t);
> > > >    }
> > >
> > > Same comment as below, the check between (current->domain) and (d) should
> > > be redundant with one higher up in the call stack.  The check between
> > > (current->domain) and (t) should remain, although this *does* result in a
> > > relaxing of the existing permission checks on the call as Jan noted.
> > >
> > > >    static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d,
> > > > unsigned long op)
> > > > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> > > > index f01b4cfaaa..16103bafc9 100644
> > > > --- a/xen/xsm/flask/hooks.c
> > > > +++ b/xen/xsm/flask/hooks.c
> > > > @@ -1199,7 +1199,9 @@ static int flask_remove_from_physmap(struct domain
> > > > *d1, struct domain *d2)
> > > >      static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
> > > >    {
> > > > -    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ |
> > > > MMU__MAP_WRITE);
> > > > +    return domain_has_perm(current->domain, d, SECCLASS_MMU,
> > > > MMU__MAP_READ | MMU__MAP_WRITE) ?:
> > > > +        domain_has_perm(current->domain, t, SECCLASS_MMU, MMU__MAP_READ |
> > > > MMU__MAP_WRITE) ?:
> > > > +        domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
> > > >    }
> > >
> > > This is at least partially redundant with the higher-level permission checks
> > > needed to get to the xenmem_add_* functions (xatp_permission_check call in
> > > xen/common/memory.c, for example).  That check already verifies the
> > > permission
> > > for (current->domain) to modify (d)'s page tables.
> > >
> > > The other two checks here look correct.
> > Do you mean that the checks that verify the permission for (current->domain) to
> > modify (d)'s page tables have already been done somewhere higher up in the
> > call stack so that I can eliminate them in both hooks?
> Although xatp_permission_chec() does check (current->domain)'s permission over
> (d), I'm not sure if this is the case for all the call paths that
> would finally lead to map_gmfn_foregin().
> If the answer is yes, I would happily remove the redundant checks.
>
> Cheers,
>
> Zhongze Liu.
If this were not the case, there would be other security bugs for the other
XENMAPSPACE_* flags that don't have their own XSM check at this level.  Last
time I checked, all paths that led here have already gone through such a
check.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel