xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: "Daniel P. Smith" <dpsmith@apertussolutions.com>
Cc: sstabellini@kernel.org, julien@xen.org,
	Volodymyr_Babchuk@epam.com, andrew.cooper3@citrix.com,
	george.dunlap@citrix.com, iwj@xenproject.org, wl@xen.org,
	roger.pau@citrix.com, tamas@tklengyel.com, tim@xen.org,
	jgross@suse.com, aisaila@bitdefender.com,
	ppircalabu@bitdefender.com, dfaggioli@suse.com, paul@xen.org,
	kevin.tian@intel.com, dgdegra@tycho.nsa.gov,
	adam.schwalm@starlab.io, scott.davis@starlab.io,
	xen-devel@lists.xenproject.org
Subject: Re: [RFC PATCH 04/10] xsm: convert rewrite privilege check function
Date: Fri, 18 Jun 2021 16:14:44 +0200	[thread overview]
Message-ID: <bd1f80e9-5d1c-beb5-f255-4351a5f64955@suse.com> (raw)
In-Reply-To: <20210514205437.13661-5-dpsmith@apertussolutions.com>

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



  reply	other threads:[~2021-06-18 14:15 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-14 20:54 [RFC PATCH 00/10] xsm: introducing domain roles Daniel P. Smith
2021-05-14 20:54 ` [RFC PATCH 01/10] headers: introduce new default privilege model Daniel P. Smith
2021-06-18 13:56   ` Jan Beulich
2021-05-14 20:54 ` [RFC PATCH 02/10] control domain: refactor is_control_domain Daniel P. Smith
2021-06-18 14:02   ` Jan Beulich
2021-05-14 20:54 ` [RFC PATCH 03/10] xenstore: migrate to default privilege model Daniel P. Smith
2021-05-14 20:54 ` [RFC PATCH 04/10] xsm: convert rewrite privilege check function Daniel P. Smith
2021-06-18 14:14   ` Jan Beulich [this message]
2021-05-14 20:54 ` [RFC PATCH 05/10] hardware domain: convert to domain roles Daniel P. Smith
2021-06-18 14:47   ` Jan Beulich
2021-05-14 20:54 ` [RFC PATCH 06/10] xsm-roles: covert the dummy system to roles Daniel P. Smith
2021-05-14 20:54 ` [RFC PATCH 07/10] xsm-roles: adjusting core xsm Daniel P. Smith
2021-05-14 20:54 ` [RFC PATCH 08/10] xsm-silo: convert silo over to domain roles Daniel P. Smith
2021-07-08 13:17   ` Jan Beulich
2021-05-14 20:54 ` [RFC PATCH 09/10] xsm-flask: clean up for domain roles conversion Daniel P. Smith
2021-05-14 20:54 ` [RFC PATCH 10/10] common/Kconfig: updating Kconfig for domain roles Daniel P. Smith

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=bd1f80e9-5d1c-beb5-f255-4351a5f64955@suse.com \
    --to=jbeulich@suse.com \
    --cc=Volodymyr_Babchuk@epam.com \
    --cc=adam.schwalm@starlab.io \
    --cc=aisaila@bitdefender.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=dfaggioli@suse.com \
    --cc=dgdegra@tycho.nsa.gov \
    --cc=dpsmith@apertussolutions.com \
    --cc=george.dunlap@citrix.com \
    --cc=iwj@xenproject.org \
    --cc=jgross@suse.com \
    --cc=julien@xen.org \
    --cc=kevin.tian@intel.com \
    --cc=paul@xen.org \
    --cc=ppircalabu@bitdefender.com \
    --cc=roger.pau@citrix.com \
    --cc=scott.davis@starlab.io \
    --cc=sstabellini@kernel.org \
    --cc=tamas@tklengyel.com \
    --cc=tim@xen.org \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    --subject='Re: [RFC PATCH 04/10] xsm: convert rewrite privilege check function' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).