Re: [PATCH v3 2/5] xen: make evtchn_alloc_unbound public

["Daniel P. Smith" <dpsmith.dev@xxxxxxxxx>]

On 3/24/22 20:30, Stefano Stabellini wrote:
> On Wed, 23 Mar 2022, Jan Beulich wrote:
>> On 23.03.2022 01:22, Stefano Stabellini wrote:
>>> On Tue, 15 Mar 2022, Daniel P. Smith wrote:
>>>> On 1/28/22 16:33, Stefano Stabellini wrote:
>>>>> From: Luca Miccio <lucmiccio@xxxxxxxxx>
>>>>>
>>>>> The xenstore event channel will be allocated for dom0less domains. It is
>>>>> necessary to have access to the evtchn_alloc_unbound function to do
>>>>> that, so make evtchn_alloc_unbound public.
>>>>>
>>>>> Add a skip_xsm parameter to allow disabling the XSM check in
>>>>> evtchn_alloc_unbound (xsm_evtchn_unbound wouldn't work for a call
>>>>> originated from Xen before running any domains.)
>>>>>
>>>>> Signed-off-by: Luca Miccio <lucmiccio@xxxxxxxxx>
>>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
>>>>> CC: Julien Grall <julien@xxxxxxx>
>>>>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
>>>>> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>>>>> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>> CC: George Dunlap <george.dunlap@xxxxxxxxxx>
>>>>> CC: Jan Beulich <jbeulich@xxxxxxxx>
>>>>> CC: Wei Liu <wl@xxxxxxx>
>>>>> ---
>>>>> Changes v3:
>>>>> - expose evtchn_alloc_unbound, assing a skip_xsm parameter
>>>>> ---
>>>>>  xen/common/event_channel.c | 13 ++++++++-----
>>>>>  xen/include/xen/event.h    |  3 +++
>>>>>  2 files changed, 11 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>>>>> index da88ad141a..be57d00a15 100644
>>>>> --- a/xen/common/event_channel.c
>>>>> +++ b/xen/common/event_channel.c
>>>>> @@ -284,7 +284,7 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
>>>>>      xsm_evtchn_close_post(chn);
>>>>>  }
>>>>>  
>>>>> -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>>>>> +int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool skip_xsm)
>>>>>  {
>>>>>      struct evtchn *chn;
>>>>>      struct domain *d;
>>>>> @@ -301,9 +301,12 @@ static int 
>>>>> evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>>>>>          ERROR_EXIT_DOM(port, d);
>>>>>      chn = evtchn_from_port(d, port);
>>>>>  
>>>>> -    rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
>>>>> -    if ( rc )
>>>>> -        goto out;
>>>>> +    if ( !skip_xsm )
>>>>> +    {
>>>>> +        rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
>>>>> +        if ( rc )
>>>>> +            goto out;
>>>>> +    }
>>>>
>>>> Please do not subvert the security framework because it causes an
>>>> inconvenience. As Jan recommended, work within the XSM check to allow
>>>> your access so that we may ensure it is done safely. If you need any
>>>> help making modifications to XSM, please do not hesitate to reach out as
>>>> I will gladly help.
>>>
>>> Thank you!
>>>
>>> First let me reply to Jan: this series is only introducing 1 more call
>>> to evtchn_alloc_unbound, which is to allocate the special xenstore event
>>> channel, the one configured via
>>> d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN].
>>>
>>> It is not meant to be a generic function, and it is not meant to be
>>> called more than once. It could (should?) be __init.
>>
>> How that? Its pre-existing use doesn't disappear, and requires it to be
>> non-__init.
> 
> Sorry I meant the new function (calling get_free_port) for the new
> use-case could be __init. The new function could be added to
> xen/common/event_channel.c or to xen/arch/arm/domain_build.c.
> 
> 
>>> The existing XSM check in evtchn_alloc_unbound cannot work and should
>>> not work: it is based on the current domain having enough privileges to
>>> create the event channel. In this case, we have no current domain at
>>> all. The current domain is Xen itself.
>>
>> And DOM_XEN cannot be given the appropriate permission, perhaps
>> explicitly when using a real policy and by default in dummy and SILO
>> modes?
> 
> The issue is that the check is based on "current", not one of the
> domains passed as an argument to evtchn_alloc_unbound. Otherwise,
> passing DOMID_XEN would be straightforward.
> 
> We would need to use a hack (like Daniel wrote in the other email) to
> set the idle_domain temporarily as a privileged domain only for the sake
> of passing an irrelevant (irrelevant as in "not relevant to this case")
> XSM check. That cannot be an improvement. Also, setting current to a
> "fake" domain is not great either.

My suggestion was not to intended to be simply a hack but looking at the
larger issue instead of simply doing a targeted fix for this one
instnace. While I cannot give an example right off hand, the reality is,
at least for hyperlaunch, that we cannot say for certain there will not
be further resource allocations that is protected by the security
framework and will require preliminary handling by the construction
logic in the hypervisor. The low-complexity approach is to address each
one in a case-by-case fashion using direct calls that go around the
security framework. A more security conscience, and higher complexity,
approach would be to consider a least-privilege approach and look at
introducing the ability to do controlled switching of contexts, i.e.
moving `current` from DOMID_IDLE to DOMID_CONSTRUCT, to one that is
granted only the necessary privileges to do the resource allocations in
which it is limited.

This is also not the first time this issue has come up, I don't recall
the exact thread but several months ago someone ran into the issue they
need to make a call to a resource function and was blocked by XSM
because DOMID_IDLE has no privileges. The reality is that the idea of
monolithic high-privileged entities is being dropped in favor of
least-privilege, and where possible hardware enforced, constraint. This
can be seen with Intel de-privileging SMM and running SMI handlers in
constrained ring 3. Arm is gaining capability pointers, CHERI, that will
enable the possibility for constrained, least-privileged kernel
subsystems. Would it not be advantageous for Xen to start moving in such
a direction that would enable it to provide a new level of safety and
security for consumers of Xen?

Coming back to the short-term, I would advocate for introducing the
concept and abstraction of constrained context switching through a set
of function calls, which would likely be under XSM to allow policy
enforcement. Likely the introductory implementation would just mask the
fact that it is just setting `is_privileged` for DOMID_IDLE. Future
evolution of the capability could see the introduction of new
"contexts", whether they are represented by a domain could be determined
then, and the ability to do controlled switching based on policy.

Just wanted to put more of my thinking out there before a path is taken.

> In the specific case of dom0less and this patch, this is the only
> instance of this issue and could be solved very straightforwardly by
> calling get_free_port directly as we discussed [1].
> 
> I know Julien had some reservations about that. Let's try to find a
> technical solution that makes everyone happy.
> 
> Maybe, instead of exporting get_free_port, we could create a new
> function in xen/common/event_channel.c and mark it as __init? This way:
> - we don't need to expose get_free_port
> - the new function would only be __init anyway, so no risk of runtime
>   misuse
> 
> What do you think?
> 
> [1] https://marc.info/?l=xen-devel&m=164197327305903