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

["Xin Li (Talons)" <xin.li@xxxxxxxxxx>]

Hello Jan,

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Friday, June 29, 2018 6:36 PM
> To: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Xin Li
> <talons.lee@xxxxxxxxx>
> Cc: Ming Lu <ming.lu@xxxxxxxxxx>; Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>;
> Wei Liu <wei.liu2@xxxxxxxxxx>; Xin Li (Talons) <xin.li@xxxxxxxxxx>; George
> Dunlap <George.Dunlap@xxxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxx; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Daniel de Graaf <dgdegra@xxxxxxxxxxxxx>; Tim
> (Xen.org) <tim@xxxxxxx>
> Subject: Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
> 
> >>> On 29.06.18 at 11:28, <talons.lee@xxxxxxxxx> 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?

They are the two mechanisms exist for inter-domain communication,
And we want to block them in SILO mode.

> 
> > --- 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.

Change to:

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 is not the default module, add boot parameter xsm=silo to choose
>-------  it. This will deny any unmediated communication channels (grant tables
>-------  and event channels) between unprivileged VMs.

>-------  If unsure, say Y.

> 
> > --- /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.

OK. Done.

 
> > +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).

I mean the one and only dom0.

> 
> > +    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.

This makes it clearer that we are calling the counterpart of dummy 
ops(overriding).
This indirect calls should not introduce any runtime penalty.

> 
> 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?

This is unnecessary, since we are blocking it from setting up,
Those calling unmap must pass the check of maping.

> 
> Jan


Best regards

Xin(Talons) Li

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