Hey Julien,
On 3/29/22 17:57, Julien Grall wrote:
> Hi Daniel,
>
> On 29/03/2022 19: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? 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.
> Auditing existing calls is somewhat easy. The trouble are for new calls.
> I would say they are unlikely, but we would need to rely on the
> reviewers to spot any misuse. So this is a bit risky.
>
> I am also a bit worry that we would end up to convert a lot of
> XSM_TARGET to XSM_HOOK (Note I have Live-Update in mind). This would
> make more difficult to figure what would the XSM calls allows without
> looking at the helper.
This approach was not mean to be the long term solution but to deal with
the immediate need as I agree doing this long term would make the
default policy very nuanced.
> I quite like the proposal from Roger. If we define two helpers (e.g.
> xsm_{enable, disable}_build_domain()), we could elevate the privilege
> for the idle domain for a short period of time (this could be restricted
> to when the dummy policy is used).
This is where I was initially going but I am hesitant to change the XSM
API in what might be temporary API calls which have a good chance will
be displaced. That being said, and it does seem like there is more in
favor of it, if the priv escalation is the overall preferred approach I
would still agree and prefer it be done in the XSM API so any usage is
more easily tracked.
v/r,
dps
