Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

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

On 4/5/22 13:17, Jason Andryuk wrote:
> On Mon, Apr 4, 2022 at 11:34 AM Daniel P. Smith
> <dpsmith@xxxxxxxxxxxxxxxxxxxx> wrote:
>>
>> On 3/31/22 09:16, Jason Andryuk wrote:
>>> On Wed, Mar 30, 2022 at 3:05 PM Daniel P. Smith
>>> <dpsmith@xxxxxxxxxxxxxxxxxxxx> wrote:
>>>>
>>>> There are now instances where internal hypervisor logic needs to make 
>>>> resource
>>>> allocation calls that are protected by XSM checks. The internal hypervisor 
>>>> logic
>>>> is represented a number of system domains which by designed are 
>>>> represented by
>>>> non-privileged struct domain instances. To enable these logic blocks to
>>>> function correctly but in a controlled manner, this commit introduces a 
>>>> pair
>>>> of privilege escalation and demotion functions that will make a system 
>>>> domain
>>>> privileged and then remove that privilege.
>>>>
>>>> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>>>> ---
>>>>  xen/include/xsm/xsm.h | 22 ++++++++++++++++++++++
>>>>  1 file changed, 22 insertions(+)
>>>>
>>>> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
>>>> index e22d6160b5..157e57151e 100644
>>>> --- a/xen/include/xsm/xsm.h
>>>> +++ b/xen/include/xsm/xsm.h
>>>> @@ -189,6 +189,28 @@ struct xsm_operations {
>>>>  #endif
>>>>  };
>>>>
>>>> +static always_inline int xsm_elevate_priv(struct domain *d)
>>>> +{
>>>> +    if ( is_system_domain(d) )
>>>> +    {
>>>> +        d->is_privileged = true;
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    return -EPERM;
>>>> +}
>>>
>>> These look sufficient for the default policy, but they don't seem
>>> sufficient for Flask.  I think you need to create a new XSM hook.  For
>>> Flask, you would want the demote hook to transition xen_boot_t ->
>>> xen_t.  That would start xen_boot_t with privileges that are dropped
>>> in a one-way transition.  Does that require all policies to then have
>>> xen_boot_t and xen_t?  I guess it does unless the hook code has some
>>> logic to skip the transition.
>>
>> I am still thinking this through but my initial concern for Flask is
>> that I don't think we want dedicated domain transitions directly in
>> code. My current thinking would be to use a Kconfig to use xen_boot_t
>> type as the initial sid for the idle domain which would then require the
>> default policy to include an allowed transition from xen_boot_t to
>> xen_t. Then rely upon a boot domain to issue an xsm_op to do a relabel
>> transition for the idle domain with an assertion that the idle domain is
>> no longer labeled with its initial sid before Xen transitions its state
>> to SYS_STATE_active. The one wrinkle to this is whether I will be able
>> to schedule the boot domain before allowing Xen to transition into
>> SYS_STATE_active.
> 
> That is an interesting approach.  While it would work, I find it
> unusual that a domain would relabel Xen.  I think Xen should be
> responsible for itself and not rely on a domain for this operation.

The boot domain is not a general domain as no domain can/should be
created with its domid or flask label post transition to
SYS_STATE_active. Its purpose was specifically meant to be a natural way
to push out complicated pre-execution domain configuration from having
to be in they hypervisor code. Therefore in a way it can be considered a
user provided de-privileged part of the hypervisor.

With that said, I just realized a flaw in the basis of my position. What
is the difference between codifying a check that the idle domain is not
the boot label versus codifying a transition from the boot label to the
running label? None really, both will require some knowledge that there
is a boot label and some running label. Combine with the fact that the
idle domain really shouldn't have any other label than xen_t. I will
work out how to incorporate the domain transition.

>>> For the default policy, you could start by creating the system domains
>>> as privileged and just have a single hook to drop privs.  Then you
>>> don't have to worry about the "elevate" hook existing.  The patch 2
>>> asserts could instead become the location of xsm_drop_privs calls to
>>> have a clear demarcation point.  That expands the window with
>>> privileges though.  It's a little simpler, but maybe you don't want
>>> that.  However, it seems like you can only depriv once for the Flask
>>> case since you want it to be one-way.
>>
>> This does simplify the solution and since today we cannot differentiate
>> between hypervisor setup and hypervisor initiated domain construction
>> contexts, it does not run counter to what I have proposed. As for flask,
>> again I do not believe codifying a domain transition bound to a new XSM
>> op is the appropriate approach.
> 
> This hard coded domain transition does feel a little weird.  But it
> seems like a natural consequence of trying to use Flask to
> deprivilege.  I guess the transition could be behind a
> dom0less/hyperlaunch Kconfig option.  I just don't see a way around it
> in some fashion with Flask enforcing.
> 
> Another idea: Flask could start in permissive and only transition to
> enforcing at the deprivilege point.  Kinda gross, but it works without
> needing a transition.
> 
> To reiterate, XSM isn't really appropriate to enforce anything
> internal to Xen.  We are working around the need to go through hook
> points during correct operation.  Code exec in Xen means all bets are
> off.  Memory writes to Xen data mean the XSM checks can be disabled
> (flip Flask to permissive) or bypassed (set d->is_privileged or change
> d->ssid).  We shouldn't lose sight of this when we talk about
> deprivileging the idle domain.

I would far prefer a transition than trying to reason about whether
flask should be in permissive or enforcing based on this state change in
combination of command line setting.

v/r
dps