[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/6] xsm: refactor xsm_ops handling


  • To: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Fri, 18 Jun 2021 12:34:54 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=AH60yatl15MpGt996L4jLcBhtfAOcyMFE8gReOfSrQY=; b=buXyCD4QbhV2BqDb5YWdHDB8YapfdL7uEbTEPIdo054cwbvs6UtC8fZv+Gv+YE9Bg/y2JYmSa8M2Yt/SCSGBTesWQ7c0Kp+pOuFcR3SvopY1AgrlEWFIUNHnRM/gf0c+5m/TQ4lVm0JqTMv7eTjDfMCX8oM4V/b92Q+5Y2OiyzJrGOYa877eMTEBfGtg9qSZkJ52uNtgZMiOUhWTDRYZY/FR5tLRTDF2ST4iorqpr6W6IIOWoabW/RdtPxSHeXVE8c4f+ejJkdVipCyyZcjd0pXcZjKxbs8xQqxHCz0szJTtjKgDl1AfXIQGksAItX0Aj64bri9HNDwfe8kOqDOaUQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=G6TxloC/ZUdNwxGwQwBq/eyjJ5R76GktaZxZ94lQWn1O6biQl6Mw+qaiIVbKO4BAF8v3+ZMEtTe2lbx6p34EPE8zj0cezvSOlbPY4hkHoz4P4HaeEjojhpazWnzfQfGNkAq2NFxzBCF7mO+3iQKnbXJ21d21l0fyoKCmwRdg6ZnX/FrUHrHoWwYIPNTO6ICIAh77D4xyVJnhsZa4QBP06pEHH5UPJtH4xmr5xraz/F2aUfkmMUdglJTyBjy9nzGhPkkhmyfTjV7HMc+7epv3tvlFWILi0EQGJIOoFFlEXeQM82GB5FWf1kQT7h6WdFBpcWSQ1eaQnf8bMg0Ig5dHuA==
  • Authentication-results: esa2.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Tamas K Lengyel <tamas@xxxxxxxxxxxxx>, Tim Deegan <tim@xxxxxxx>, Juergen Gross <jgross@xxxxxxxx>, Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>, Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>, Dario Faggioli <dfaggioli@xxxxxxxx>, Paul Durrant <paul@xxxxxxx>, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>, <persaur@xxxxxxxxx>, <christopher.w.clark@xxxxxxxxx>, <adam.schwalm@xxxxxxxxxx>, <scott.davis@xxxxxxxxxx>
  • Delivery-date: Fri, 18 Jun 2021 11:35:15 +0000
  • Ironport-hdrordr: A9a23:hVQU7KNtUwZPSsBcT1j155DYdb4zR+YMi2TDiHofdfUFSKClfp 6V8cjztSWUtN4QMEtQ/OxoS5PwPk80kqQFnbX5XI3SITUO3VHHEGgM1/qb/9SNIVyYygcZ79 YbT0EcMqyCMbEZt7eC3ODQKb9Jq7PmgcPY8Ns2jU0dKT2CA5sQnzuRYTzrdHGeKjM2Z6bRWK Dsnfau8FGbCAUqh4mAdzc4t6+pnayDqLvWJTo9QzI34giHij2lrJb8Dhijxx8bFxdC260r/2 TpmxHwovzLiYD69jbsk0voq7hGktrozdVOQOSKl8guMz3pziKlfp5oVbGutC085Muv9FEput /RpApIBbUz11rhOkWO5Tf90Qjp1zgjr1X4z0WDvHflqcvlABonFston+tiA17kwntlmOs5/L NA3mqfuZYSJwjHhj7B69/BUAwvvlaooEAljfUYgxVkIMkjgYdq3MsiFX5uYdE99HqQ0vF/LA AuNrCe2B9uSyLfU5iD1VMfmOBFNx8Ib2W7qktrgL3Z79EZpgEj86O0rPZv1kvoz6hNPKWs0d 60eJiApIs+OvP+UpgNctvpYfHHRlAlEii8f157HzzcZeo60iX22uDKCfMOlbuXRKA=
  • Ironport-sdr: 7kbziORytCmX9ZIiPgWVDm1MuO8W1aTKqfkr2UmycGUI5dpraZruIWhLHayxpaau2AxmA5TxP4 4Y2TGHyh4Ij6pv9uQkzoyAgQCmYbD3Tui8yWJukoVOpKA23akDpR1V0RD8mqF2p9Di6VvulQt5 mfxG+X0oZwArGrKWDCZew4G+TR8leGabSrBh6VXX/GocIwh4AlvpBgcUjUL8/AGL5CF1VDq5rV LMqPwxUVHSpaQfdVgbg6ERaVjHLoBCQz4fwxNwRhEe1Nh6kPKIxnqkrhqTKm+HV+4gcSAWvfgX KQ8=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 18/06/2021 00:39, Daniel P. Smith wrote:
> The assignment and setup of xsm_ops structure was refactored to make it a
> one-time assignment. The calling of the xsm_ops were refactored to use the
> alternate_call framework to reduce the need for retpolines.
>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>

I think the commit message needs a little more explanation for anyone
doing code archaeology.

AFAICT, the current infrastructure has some (incomplete?) support for
Flask to unload itself as the security engine, which doesn't sounds like
a clever thing in general.

What we do here is make a semantic change to say that the security
engine (Dummy, Flask or SILO) gets chosen once during boot, and is
immutable thereafter.  This is better from a security standpoint (no
accidentally unloading Flask at runtime), and allows for the use of the
alternative_vcall() infrastructure to drop all the function pointer calls.

Does that about sum things up?

> diff --git a/xen/xsm/flask/flask_op.c b/xen/xsm/flask/flask_op.c
> index 01e52138a1..df9fcc1d6d 100644
> --- a/xen/xsm/flask/flask_op.c
> +++ b/xen/xsm/flask/flask_op.c
> @@ -225,26 +225,7 @@ static int flask_security_sid(struct 
> xen_flask_sid_context *arg)
>  
>  static int flask_disable(void)
>  {
> -    static int flask_disabled = 0;
> -
> -    if ( ss_initialized )
> -    {
> -        /* Not permitted after initial policy load. */
> -        return -EINVAL;
> -    }
> -
> -    if ( flask_disabled )
> -    {
> -        /* Only do this once. */
> -        return -EINVAL;
> -    }
> -
> -    printk("Flask:  Disabled at runtime.\n");
> -
> -    flask_disabled = 1;
> -
> -    /* Reset xsm_ops to the original module. */
> -    xsm_ops = &dummy_xsm_ops;
> +    printk("Flask:  Disabling is not supported.\n");

Judging by this, should this patch be split up more?

I think you want to remove FLASK_DISABLE (and this function too - just
return -EOPNOTSUPP in the parent) as a separate explained change (as it
is a logical change in how Flask works).

The second patch wants to be the rest, which changes the indirection of
xsm_ops and converts to vcall().  This is a fairly mechanical change
without semantic changes.

I'm unsure if you want a 3rd patch in the middle, separating the
xsm_core_init() juggling, with the "switch to using vcall()".  It might
be a good idea for more easily demonstrating the changes, but I'll leave
it to your judgement.

> diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
> index 5eab21e1b1..acc1af7166 100644
> --- a/xen/xsm/xsm_core.c
> +++ b/xen/xsm/xsm_core.c
>  static int __init xsm_core_init(const void *policy_buffer, size_t 
> policy_size)
>  {
>  #ifdef CONFIG_XSM_FLASK_POLICY
> @@ -87,17 +86,22 @@ static int __init xsm_core_init(const void 
> *policy_buffer, size_t policy_size)
>      }
>  #endif
>  
> -    if ( verify(&dummy_xsm_ops) )
> +    if ( xsm_ops_registered != XSM_OPS_UNREGISTERED )
>      {
> -        printk(XENLOG_ERR "Could not verify dummy_xsm_ops structure\n");
> +        printk(XENLOG_ERR
> +            "Could not init XSM, xsm_ops register already attempted\n");

Indentation.

>          return -EIO;
>      }
>  
> -    xsm_ops = &dummy_xsm_ops;
> +    /* install the dummy ops as default to ensure ops
> +     * are defined if requested policy fails init
> +     */
> +    xsm_fixup_ops(&xsm_ops);

/* Comment style. */

or

/*
 * Multi-
 * line comment style.
 */

>      switch ( xsm_bootparam )
>      {
>      case XSM_BOOTPARAM_DUMMY:
> +        xsm_ops_registered = XSM_OPS_REGISTERED;
>          break;
>  
>      case XSM_BOOTPARAM_FLASK:
> @@ -113,6 +117,9 @@ static int __init xsm_core_init(const void 
> *policy_buffer, size_t policy_size)
>          break;
>      }
>  
> +    if ( xsm_ops_registered != XSM_OPS_REGISTERED )
> +        xsm_ops_registered = XSM_OPS_REG_FAILED;
> +
>      return 0;
>  }
>  
> @@ -197,16 +204,21 @@ bool __init has_xsm_magic(paddr_t start)
>  
>  int __init register_xsm(struct xsm_operations *ops)
>  {
> -    if ( verify(ops) )
> +    if ( xsm_ops_registered != XSM_OPS_UNREGISTERED )
> +        return -EAGAIN;

I know you moved this around the function, but it really isn't -EAGAIN
material any more.  It's "too late - nope".

-EEXIST is probably best for "I'm never going to tolerate another call".

> +
> +    if ( !ops )
>      {
> -        printk(XENLOG_ERR "Could not verify xsm_operations structure\n");
> +        xsm_ops_registered = XSM_OPS_REG_FAILED;
> +        printk(XENLOG_ERR "Invalid xsm_operations structure registered\n");
>          return -EINVAL;

Honestly, I'd be half tempted to declare register_xsm() with
__nonnull(0) and let the compiler reject any attempt to pass a NULL ops
pointer.

Both callers pass a pointer to a static singleton objects.

>      }
>  
> -    if ( xsm_ops != &dummy_xsm_ops )
> -        return -EAGAIN;
> +    /* use dummy ops for any empty ops */
> +    xsm_fixup_ops(ops);

Isn't this redundant with the call in xsm_core_init(), seeing as
register_xsm() must be nested within the switch statement?

> -    xsm_ops = ops;
> +    xsm_ops = *ops;
> +    xsm_ops_registered = XSM_OPS_REGISTERED;
>  
>      return 0;
>  }

Having got to the end, the xsm_core_init() vs register_xsm() dynamic is
quite weird.

I think it would result in clearer code to have init_{flask,silo}()
return pointers to their struct xsm_operations *, and let
xsm_core_init() do the copy in to xsm_ops.  This reduces the scope of
xsm_ops_state to this function only, and gets rid of at least one
runtime panic() call which is dead code.

If you were to go with this approach, you'd definitely want to split
into the 3-patch approach.

~Andrew




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.