Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM

["Jan Beulich" <JBeulich@suse.com>]

>>> On 29.06.18 at 11:28, <talons.lee@gmail.com> wrote:
> When SILO is enabled, there would be no page-sharing between
> unprivileged VMs (no grant tables or event channels).

What is the relation between page sharing and event channels?

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -143,6 +143,17 @@ config XSM_FLASK_POLICY
>  
>  	  If unsure, say Y.
>  
> +config XSM_SILO
> +	def_bool y
> +	prompt "SILO support"
> +	depends on XSM
> +	---help---
> +	  Enables SILO as the access control mechanism used by the XSM framework.
> +	  This will deny any unmediated communication channels between unprivileged
> +	  VMs.
> +
> +	  If unsure, say Y.

It would be helpful to clarify here that this is not the default mode of
operation. In fact, another Kconfig (choice) might be useful to have to
select the built-in default. In fact "deny any" suggests that this is what
is going to happen regardless of command line options. At the very
least I think this wants to be "This will allow to deny any ..." or "In this
mode, any ... will by denied".

Andrew, the chosen name here may underline the relevance of my
comment regarding XSM_FLASK vs just FLASK, albeit things are
unclear/ambiguous if I also take into account the code further down.
The descriptions above make it sound as if this was an override to
whatever access control mechanism was in place (dummy or flask
currently). Code below suggests though that this is meant to be a
clone of dummy, with just some minimal adjustments. I guess it's
rather the description that needs adjustment, but the alternative
of being a global override even in FLASK mode certainly exists.

Furthermore it is unclear here what an "unmediated communication
channel" is, and what "mediated communication channels" (if any)
are still available in this new mode.

> --- /dev/null
> +++ b/xen/xsm/silo.c
> @@ -0,0 +1,106 @@
> +/******************************************************************************
> + * xsm/silo.c
> + *
> + * SILO module for XSM(Xen Security Modules)
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Copyright (c) 2018 Citrix Systems Ltd.
> + */
> +
> +#include <xen/sched.h>
> +#include <xsm/xsm.h>
> +
> +struct xsm_operations silo_xsm_ops;
> +
> +/*
> + * check if inter-domain communication is allowed
> + * return true when pass check
> + */

Uppercase first letter please, and I'd prefer if you also put a full stop here.

> +static bool silo_mode_dom_check(domid_t ldom, domid_t rdom)
> +{
> +    domid_t hd_dom = hardware_domain->domain_id;

I don't think you mean the hardware domain here, but the control domain
(of which in theory there may be multiple).

> +    domid_t cur_dom = current->domain->domain_id;
> +
> +    if ( ldom == DOMID_SELF )
> +        ldom = cur_dom;
> +    if ( rdom == DOMID_SELF )
> +        rdom = cur_dom;
> +
> +    return (hd_dom == cur_dom || hd_dom == ldom || hd_dom == rdom ||
> +            ldom == rdom);
> +}
> +
> +static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn,
> +                               domid_t id2)
> +{
> +    if ( silo_mode_dom_check(d1->domain_id, id2) )
> +        return dummy_xsm_ops.evtchn_unbound(d1, chn, id2);

Urgh. Why is this not xsm_evtchn_unbound() from dummy.h? It would be
really nice to avoid such extra indirect calls here.

Furthermore, this hook is called in two contexts. Is the above really
appropriate also in the alloc_unbound_xen_event_channel() case?

> +static int silo_grant_mapref(struct domain *d1, struct domain *d2,
> +                             uint32_t flags)
> +{
> +    if ( silo_mode_dom_check(d1->domain_id, d2->domain_id) )
> +        return dummy_xsm_ops.grant_mapref(d1, d2, flags);
> +    return -EPERM;
> +}

What about the unmap counterpart?

Jan


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