Re: [RFC PATCH 1/1] xsm: allows system domains to allocate evtchn

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 3/29/22 02:43, Jan Beulich wrote:
> On 28.03.2022 22:36, Daniel P. Smith wrote:
>> During domain construction under dom0less and hyperlaunch it is necessary to
>> allocate at least the event channel for xenstore and potentially the event
>> channel for the core console. When dom0less and hyperlaunch are doing their
>> construction logic they are executing under the idle domain context. The idle
>> domain is not a privileged domain, it is not the target domain, and as a 
>> result
>> under the current default XSM policy is not allowed to allocate the event
>> channel.
> 
> I appreciate the change is only needed there right now, but it feels
> inconsistent. _If_ it is to remain that way, at least a comment needs
> to be put in xsm_evtchn_unbound() making clear why this is a special
> case, and hence clarifying to people what the approximate conditions
> are to have such also put elsewhere. But imo it would be better to
> make the adjustment right in xsm_default_action(), without touching
> event_channel.c at all. Iirc altering xsm_default_action() was
> discussed before, but I don't recall particular reasons speaking
> against that approach.

By inconsistent, I take it you mean this is first place within an XSM
hook where an access decision is based on the current domain being a
system domain? I do agree and would add a comment to the change in the
XSM hook in a non-RFC version of the patch.

As to moving the check down into xsm_default_action(), the concern I
have with doing so is that this would then make every XSM check succeed
if `current->domain` is a system domain. Doing so would require a review
of every function which has an XSM hoook to evaluate every invocation of
those functions that,
  1. is there ever a time when current->domain may be a system domain
  2. if so,
    a. is the invocation on behalf of the system domain
    b. or is the invocation on behalf of a non-system domain

If there is any instance of 2b, then an inadvertent privilege escalation
can occur on that path. For evtchn_alloc_unbound() I verified the only
place, besides the new hyperlaunch calls, it is invoked is in the evtchn
hypercall handler, where current should be pointing at the domain that
made the hypercall.

>> This patch only addresses the event channel situation by adjust the default 
>> XSM
>> policy for xsm_evtchn_unbound to explicitly allow system domains to be able 
>> to
>> make the allocation call.
> 
> Indeed I'm having trouble seeing how your change would work for SILO
> mode, albeit Stefano having tested this would make me assume he did
> so in SILO mode, as that's the default on Arm iirc. Afaict
> silo_mode_dom_check() should return false in the described situation.

Correct, this patch only addressed the default policy. If an equivalent
change for SILO is desired, then it would be placed in
silo_evtchn_unbound() and not in silo_mode_dom_check() for the same
reasons I would be hesitant to place it in xsm_default_action().

> Similarly I don't see how things would work transparently with a
> Flask policy in place. Regardless of you mentioning the restriction,
> I think this wants resolving before the patch can go in.

To enable the equivalent in flask would require no hypervisor code
changes. Instead that would be handled by adding the necessary rules to
the appropriate flask policy file(s).


v/r,
dps