Re: [RFC PATCH 04/10] xsm: convert rewrite privilege check function

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.05.2021 22:54, Daniel P. Smith wrote:

In the title, did you mean just one of "convert" or "rewrite", or
did you omit some punctuation?

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -475,6 +475,12 @@ struct domain
>  #define XSM_XENSTORE  (1U<<31) /* Xenstore: domain that can do privileged 
> operations on xenstore */
>  #define CLASSIC_DOM0_PRIVS (XSM_PLAT_CTRL | XSM_DOM_BUILD | XSM_DOM_SUPER | \
>               XSM_DEV_EMUL | XSM_HW_CTRL | XSM_HW_SUPER | XSM_XENSTORE)
> +/* Any access for which XSM_DEV_EMUL is the restriction, XSM_DOM_SUPER is an 
> override */
> +#define DEV_EMU_PRIVS (XSM_DOM_SUPER | XSM_DEV_EMUL)
> +/* Anytime there is an XSM_TARGET check, XSM_SELF also applies, and 
> XSM_DOM_SUPER is an override */
> +#define TARGET_PRIVS (XSM_TARGET | XSM_SELF | XSM_DOM_SUPER)
> +/* Anytime there is an XSM_XENSTORE check, XSM_DOM_SUPER is an override */
> +#define XENSTORE_PRIVS (XSM_XENSTORE | XSM_DOM_SUPER)

I think all of these want to *start* with a common prefix, e.g.
XSM_PRIVS_*. But of course especially these "override" remarks in
the comments again show that for now it is unclear what the
individual bits really mean, and hence whether these combinations
all make sense.

> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -65,37 +65,48 @@ void __xsm_action_mismatch_detected(void);
>  #define XSM_INLINE always_inline
>  #define XSM_DEFAULT_ARG xsm_default_t action,
>  #define XSM_DEFAULT_VOID xsm_default_t action
> -#define XSM_ASSERT_ACTION(def) LINKER_BUG_ON(def != action)
> +#define XSM_ASSERT_ACTION(def) LINKER_BUG_ON((def) != action)
>  
>  #endif /* CONFIG_XSM */
>  
>  static always_inline int xsm_default_action(
>      xsm_default_t action, struct domain *src, struct domain *target)
>  {
> -    switch ( action ) {
> -    case XSM_HOOK:
> +    /* TODO: these three if's could be squashed into one, decreasing
> +     *       the readability/logical reason-ability but may decrease the
> +     *       number of spectre gadgets
> +     */

Seeing this remark, I'm particularly puzzled by you dropping all
evaluate_nospec().

> +    if ( action & XSM_NONE )
>          return 0;
> -    case XSM_TARGET:
> -        if ( evaluate_nospec(src == target) )
> -        {
> -            return 0;
> -    case XSM_XS_PRIV:
> -            if ( evaluate_nospec(is_xenstore_domain(src)) )
> -                return 0;
> -        }
> -        /* fall through */
> -    case XSM_DM_PRIV:
> -        if ( target && evaluate_nospec(src->target == target) )
> -            return 0;
> -        /* fall through */
> -    case XSM_PRIV:
> -        if ( is_control_domain(src) )
> -            return 0;
> -        return -EPERM;
> -    default:
> -        LINKER_BUG_ON(1);
> -        return -EPERM;
> -    }
> +
> +    if ( (action & XSM_SELF) && ((!target) || (src == target)) )
> +        return 0;
> +
> +    if ( (action & XSM_TARGET) && ((target) && (src->target == target)) )
> +        return 0;

This is an inline function - no need to parenthesize individual
identifiers (also again below). Similarly no need to parenthesize
!target.

> +    /* XSM_DEV_EMUL is the only domain role with a condition, i.e. the
> +     * role only applies to a domain's target.
> +     */
> +    if ( (action & XSM_DEV_EMUL) && (src->xsm_roles & XSM_DEV_EMUL)
> +        && (target) && (src->target == target) )
> +        return 0;
> +
> +    /* Mask out SELF, TARGET, and DEV_EMUL as they have been handled */
> +    action &= ~(XSM_SELF & XSM_TARGET & XSM_DEV_EMUL);
> +
> +    /* Checks if the domain has one of the remaining roles set on it:
> +     *      XSM_PLAT_CTRL
> +     *      XSM_DOM_BUILD
> +     *      XSM_DOM_SUPER
> +     *      XSM_HW_CTRL
> +     *      XSM_HW_SUPER
> +     *      XSM_XENSTORE
> +     */
> +    if (src->xsm_roles & action)

There are style issues here again. I'm not going to mention such any
further. As to the comment, I'm seeing the risk of it ending up stale
the moment yet another role gets added. IOW I'm not convinced you
should enumerate the remaining ones here.

Jan