On 29.03.2022 20:57, Daniel P. Smith wrote: > 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?
Well - yes and no. Even if further instances appeared, overall state would still end up inconsistent. > 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. Such an audit shouldn't be overly difficult, as the majority of XSM hook invocations sit clearly visible on hypercall paths, where it is clear that current->domain is not a system one. >>> 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). Of course this can be dealt with by adjusting policy file(s), but until people do so they'd end up with a perceived regression and/or unexplained difference in behavior from running in dummy (or SILO, once also taken care of) mode. Jan
