Re: [PATCH] XSM/domctl: Fix permission checks on XEN_DOMCTL_createdomain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.07.2024 18:26, Andrew Cooper wrote:
> The XSM checks for XEN_DOMCTL_createdomain are problematic.  There's a split
> between xsm_domctl() called early, and flask_domain_create() called quite late
> during domain construction.
> 
> All XSM implementations except Flask have a simple IS_PRIV check in
> xsm_domctl(), and operate as expected when an unprivileged domain tries to
> make a hypercall.
> 
> Flask however foregoes any action in xsm_domctl() and defers everything,
> including the simple "is current permitted to create a a domain" check, to

Nit: Double "a".

> flask_domain_create().
> 
> As a conseqeuence, when XSM Flask is active, and irrespective of the policy
> loaded, all domains irrespective of privilege can:
> 
>  * Mutate the global 'rover' variable, used to track the next free domid.
>    Therefore, all domains can cause a domid wraparound, and combined with a
>    volentary reboot, choose their own domid.
> 
>  * Cause a reasonable amount of a domain to be constructed before ultimately
>    failing for permission reasons, including the use of settings outside of
>    supported limits.
> 
> In order to remedate this, pass the ssidref into xsm_domctl() and at least
> check that the calling domain privileged enough to create domains.
> 
> This issue has not been assigned an XSA, because Flask is experimental and not
> security supported.
> 
> Reported-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

However, a remark and a nit:

> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -162,7 +162,7 @@ static XSM_INLINE int cf_check xsm_set_target(
>  }
>  
>  static XSM_INLINE int cf_check xsm_domctl(
> -    XSM_DEFAULT_ARG struct domain *d, int cmd)
> +    XSM_DEFAULT_ARG struct domain *d, int cmd, uint32_t ssidref)

Might be a reasonable thing to also convert type of "cmd" here and elsewhere,
as you're touching all relevant places anyway: The struct field passed in is
uint32_t, so the caller needlessly does a signed-ness conversion.

> @@ -248,9 +248,9 @@ static inline int xsm_set_target(
>      return alternative_call(xsm_ops.set_target, d, e);
>  }
>  
> -static inline int xsm_domctl(xsm_default_t def, struct domain *d, int cmd)
> +static inline int xsm_domctl(xsm_default_t def, struct domain *d, int cmd, 
> uint32_t ssidref)

This line is getting a little too long now.

Jan