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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

> >>  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.

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

Thanks, Roger.