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

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

On 4/4/22 11:12, Roger Pau Monné wrote:
> On Mon, Apr 04, 2022 at 10:21:18AM -0400, Daniel P. Smith wrote:
>> On 3/31/22 08:36, Roger Pau Monné wrote:
>>> On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith 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 ++++++++++++++++++++++
>>>
>>> I'm not sure this needs to be in xsm code, AFAICT it could live in a
>>> more generic file.
>>
>> From my perspective this is access control logic, thus why I advocate
>> for it to be under XSM. A personal goal is to try to get all security,
>> i.e. access control, centralized to the extent that it doing so does not
>> make the code base unnecessarily complicated.
> 
> Maybe others have a different opinion, but IMO setting is_privileged
> is not XSM specific. It happens to solve an XSM issue, but that's no
> reason to place it in the xsm code base.

I think this deserves a separate more dedicated thread as I would take
the position that Xen privilege model is at best fractured because
is_control_domain() (and underlying is_privileged),
is_hardware_domain,() and is_xenstore_domain() are used independent of
XSM. Perhaps the discussion can be had when I get back to the XSM Roles
patches to enable disaggregating Xen with hyperlaunch (or at a Xen
Summit design session).

>>>>  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)
>>>
>>> I don't think it needs to be always_inline, using just inline would be
>>> fine IMO.
>>>
>>> Also this needs to be __init.
>>
>> AIUI always_inline is likely the best way to preserve the speculation
>> safety brought in by the call to is_system_domain().
> 
> There's nothing related to speculation safety in is_system_domain()
> AFAICT. It's just a plain check against d->domain_id. It's my
> understanding there's no need for any speculation barrier there
> because d->domain_id is not an external input.

Hmmm, this actually raises a good question. Why is is_control_domain(),
is_hardware_domain, and others all have evaluate_nospec() wrapping the
check of a struct domain element while is_system_domain() does not?

> In any case this function should be __init only, at which point there
> are no untrusted inputs to Xen.

I thought it was agreed that __init on inline functions in headers had
no meaning?