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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.07.18 at 11:22, <xin.li@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: Monday, July 2, 2018 3:29 PM
>> >>> 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.
> Do you mean add " or event notifications",
> When SILO is enabled, there would be no page-sharing or event notifications
> between unprivileged VMs (no grant tables or event channels).

Yes, that's one way to clarify things.

>> > 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.
> This can't prevent side-channel attack.

???

>> 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?
> The existing XSM code use Kconfig,
> I just want to follow the similar style for new module.
> And yes, we can handle it in CONFIG_XSM like dummy.
> Which way is better?

Daniel, Andrew?

>> >> > +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.
> 
> Yes! thanks.
> I will use 
> is_control_domain(d)
> instead of comparing the hardware domain id.
> 
> This comment is misleading then:
> /* Is this guest fully privileged (aka dom0)? */
>    bool             is_privileged;

Yes, but it's likely going to remain that way until further disaggregation
work would happen.

>> >> > +    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?
> I only mean it's not runtime binding.
> 
> And I ran some performance test before, seems no performance penalty.

Sure, these paths aren't normally performance critical. But by doing what
you do we'd have a bad precedent, and if someone later cloned your
solution into something that does sit on a performance critical path, we'd
have a problem.

> The names in dummy.h are the same as xsm.h.
> If I call xsm_evtchn_unbound, that's from xsm.h,
> It probably call silo_evtchn_unbound, ends up in a loop.
> 
> So I may have to rename all the functions in dummy.h,

Note how I've said "naming the function in dummy.h
dummy_evtchn_unbound() (aliased to xsm_evtchn_unbound()
for satisfying needs elsewhere)."

> And remove static...

I don't think so, no.

> is it necessary?

It should imo be at least considered. But Daniel as the maintainer may
have something to say here as well...

Jan


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