Hello Jan,
> -----Original Message-----
> From: Jan Beulich [mailto:[email protected]]
> Sent: Monday, July 2, 2018 3:29 PM
> To: Xin Li (Talons) <[email protected]>; Xin Li <[email protected]>
> Cc: Andrew Cooper <[email protected]>; George Dunlap
> <[email protected]>; Ming Lu <[email protected]>; Sergey Dyasli
> <[email protected]>; Wei Liu <[email protected]>; Stefano Stabellini
> <[email protected]>; [email protected]; Konrad Rzeszutek Wilk
> <[email protected]>; Daniel de Graaf <[email protected]>; Tim
> (Xen.org) <[email protected]>
> Subject: RE: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
>
> >>> On 02.07.18 at 08:57, <[email protected]> wrote:
> >> From: Jan Beulich [mailto:[email protected]]
> >> Sent: Friday, June 29, 2018 6:36 PM
> >> >>> On 29.06.18 at 11:28, <[email protected]> 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).
>
> >> > --- 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.
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?
>
> >> > +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;
> >> > + 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.
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,
And remove static...
is it necessary?
>
> >> 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.
I think that's for completeness.
We can override it only when necessary.
But this change don't have to include that part.
>
> Jan
Best regards
Xin(Talons) Li
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
