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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.07.18 at 08:57, <xin.li@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: Friday, June 29, 2018 6:36 PM
>> >>> 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.

I understand this, but it doesn't answer my question. I agree that
grant tables are a means to share pages, but the wording looks odd
to me wrt 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.
> 
> 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.

With s/module/mode/ this is an improvement, but continues to leave open
in particular what an "unmediated communication channel" is.

Btw, thinking about it again - do we need a Kconfig option here in the first
place, when the mode isn't the default, and it's not a whole lot of code
that gets added?

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

No, for the purpose here you don't mean Dom0, which just happens to
be both hardware and (the only) control domain in most setups. From
a security pov though you need to distinguish all of these.

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

Yes, but the same level of clarity could be achieved when naming the
function in dummy.h dummy_evtchn_unbound() (aliased to
xsm_evtchn_unbound() for satisfying needs elsewhere).

> This indirect calls should not introduce any runtime penalty.

How does it not, when indirect calls are more expensive than direct
ones already without the Spectre v2 mitigations?

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

Taking that position, the unmap XSM hook's existence is questionable
altogether.

Jan


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