Re: [PATCH 2/2] flask: create unified "flask=" boot parameter

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 06/03/15 21:43, Daniel De Graaf wrote:
> This unifies the flask_enforcing and flask_enabled boot parameters into
> a single parameter with additional states.  Defined options are:
>
>  force - require policy to be loaded at boot time and enforce it
>  late - bootloader policy is not used; later loadpolicy is enforcing
>  permissive - a missing or broken policy does not panic
>  disabled - revert to dummy (no XSM) policy.  Was flask_enabled=0
>
> The default mode remains "permissive" and the flask_enforcing boot
> parameter is retained for compatibility.  If flask_enforcing=1 is
> specified and flask= is not, the bootloader policy will be loaded in
> enforcing mode if present, but errors will disable access controls until
> a successful loadpolicy instead of causing a panic at boot.
>
> Suggested-by: Julien Grall <julien.grall@linaro.org>
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  docs/man/xl.pod.1                   |  4 ++--
>  docs/misc/xen-command-line.markdown | 30 +++++++++++++++++++++++++-----
>  docs/misc/xsm-flask.txt             | 29 -----------------------------
>  xen/xsm/flask/flask_op.c            | 34 ++++++++++++++++++++++++++++++----
>  xen/xsm/flask/hooks.c               | 22 +++++++++++++---------
>  xen/xsm/flask/include/security.h    |  8 +++++++-
>  6 files changed, 77 insertions(+), 50 deletions(-)
>
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index 6b89ba8..48b8f98 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -1441,8 +1441,8 @@ Determine if the FLASK security module is loaded and enforcing its policy.
>  =item B<setenforce> I<1|0|Enforcing|Permissive>
>  
>  Enable or disable enforcing of the FLASK access controls. The default is
> -permissive and can be changed using the flask_enforcing option on the
> -hypervisor's command line.
> +permissive, but this can be changed to enforcing by specifying "flask=enforcing"
> +or "flask=late" on the hypervisor's command line.
>  
>  =item B<loadpolicy> I<policy-file>
>  
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index 9b458e1..505617b 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -626,11 +626,31 @@ hardware domain is architecture dependent.
>  Note that specifying zero as domU value means zero, while for dom0 it means
>  to use the default.
>  
> -### flask\_enabled
> -> `= <integer>`
> -
> -### flask\_enforcing
> -> `= <integer>`
> +### flask
> +> `= permissive | force | enforcing | late | disabled`
> +
> +> Default: `permissive`
> +
> +Specify how the FLASK security server should be configured.  This option is only
> +available if the hypervisor was compiled with XSM support (which can be enabled
> +by setting XSM\_ENABLE = y in .config).
> +
> +* `permissive`: This is intended for development and is not suitable for use
> +  with untrusted guests.  If a policy is provided by the bootloader, it will be
> +  loaded; errors will be reported to the ring buffer but will not prevent
> +  booting.  The policy can be changed to enforcing mode using "xl setenforce".
> +* `force` or `enforcing`: This requires a security policy to be provided by the
> +  bootloader and will enter enforcing mode prior to the creation of domain 0.
> +  If a valid policy is not provided, the hypervisor will not continue booting.
> +* `late`: This disables loading of the security policy from the bootloader.
> +  FLASK will be enabled but will not enforce access controls until a policy is
> +  loaded by a domain using "xl loadpolicy".  Once a policy is loaded, FLASK will
> +  run in enforcing mode unless "xl setenforce" has changed that setting.
> +* `disabled`: This causes the XSM framework to revert to the dummy module.  The
> +  dummy module provides the same security policy as is used when compiling the
> +  hypervisor without support for XSM.  The xsm\_op hypercall can also be used to
> +  switch to this mode after boot, but there is no way to re-enable FLASK once
> +  the dummy module is loaded.
>  
>  ### font
>  > `= <height>` where height is `8x8 | 8x14 | 8x16`
> diff --git a/docs/misc/xsm-flask.txt b/docs/misc/xsm-flask.txt
> index 9559028..ab05913 100644
> --- a/docs/misc/xsm-flask.txt
> +++ b/docs/misc/xsm-flask.txt
> @@ -396,32 +396,3 @@ the ranges being denied to more easily determine what resources are required.
>  When running in permissive mode, only the first denial of a given
>  source/destination is printed to the log, so labeling devices using this method
>  may require multiple passes to find all required ranges.
> -
> -Additional notes on XSM:FLASK
> ------------------------------
> -
> -1) xen command line parameters
> -
> -	a) flask_enforcing
> -	
> -	The default value for flask_enforcing is '0'.  This parameter causes the 
> -	platform to boot in permissive mode which means that the policy is loaded 
> -	but not enforced.  This mode is often helpful for developing new systems 
> -	and policies as the policy violations are reported on the xen console and 
> -	may be viewed in dom0 through 'xl dmesg'.
> -	
> -	To boot the platform into enforcing mode, which means that the policy is
> -	loaded and enforced, append 'flask_enforcing=1' on the grub line.
> -	
> -	This parameter may also be changed through the flask hypercall.
> -	
> -	b) flask_enabled
> -	
> -	The default value for flask_enabled is '1'.  This parameter causes the
> -	platform to enable the FLASK security module under the XSM framework.
> -	The parameter may be enabled/disabled only once per boot.  If the parameter
> -	is set to '0', only a reboot can re-enable flask.  When flask_enabled is '0'
> -	the DUMMY module is enforced.
> -
> -	This parameter may also be changed through the flask hypercall.  But may
> -	only be performed once per boot.
> diff --git a/xen/xsm/flask/flask_op.c b/xen/xsm/flask/flask_op.c
> index 0e89360..8db9b1e 100644
> --- a/xen/xsm/flask/flask_op.c
> +++ b/xen/xsm/flask/flask_op.c
> @@ -24,11 +24,12 @@
>  #define _copy_to_guest copy_to_guest
>  #define _copy_from_guest copy_from_guest
>  
> -int flask_enforcing = 0;
> -integer_param("flask_enforcing", flask_enforcing);
> +int __read_mostly flask_bootparam = FLASK_BOOTPARAM_DEFAULT;

It would be cleaner for this to be an enum.  The exact numeric values
are not interesting.

> +static void parse_flask_param(char *s);
> +custom_param("flask", parse_flask_param);
>  
> -int flask_enabled = 1;
> -integer_param("flask_enabled", flask_enabled);
> +int __read_mostly flask_enforcing = 0;
> +integer_param("flask_enforcing", flask_enforcing);

It would also be cleaner for flask_enforcing to become a bool_t, seeing
as you area already changing it.

>  
>  #define MAX_POLICY_SIZE 0x4000000
>  
> @@ -60,6 +61,26 @@ extern int ss_initialized;
>  
>  extern struct xsm_operations *original_ops;
>  
> +static void __init parse_flask_param(char *s)
> +{
> +    if ( !strcmp(s, "force") || !strcmp(s, "enforcing") )
> +    {
> +        flask_enforcing = 1;
> +        flask_bootparam = FLASK_BOOTPARAM_FORCE;
> +    }
> +    else if ( !strcmp(s, "late") )
> +    {
> +        flask_enforcing = 1;
> +        flask_bootparam = FLASK_BOOTPARAM_LATELOAD;
> +    }
> +    else if ( !strcmp(s, "disabled") )
> +        flask_bootparam = FLASK_BOOTPARAM_DISABLED;
> +    else if ( !strcmp(s, "permissive") )
> +        flask_bootparam = FLASK_BOOTPARAM_PERMISSIVE;
> +    else
> +        panic("Invalid value specified for flask= boot parameter");

This panic is not useful.  Command line parameters are parsed before the
console is started, which means that the message will not find its way
anywhere useful.

It would be better to have a sentinal unknown value and possibly
panicking in flask_init(), which is much later in boot after the APs
have been brought up.

~Andrew

> +}
> +
>  static int domain_has_security(struct domain *d, u32 perms)
>  {
>      struct domain_security_struct *dsec;
> @@ -502,6 +523,7 @@ static int flask_security_load(struct xen_flask_load *load)
>  {
>      int ret;
>      void *buf = NULL;
> +    bool_t is_reload = ss_initialized;
>  
>      ret = domain_has_security(current->domain, SECURITY__LOAD_POLICY);
>      if ( ret )
> @@ -526,6 +548,10 @@ static int flask_security_load(struct xen_flask_load *load)
>      if ( ret )
>          goto out;
>  
> +    if ( !is_reload )
> +        printk(XENLOG_INFO "Flask: Policy loaded, continuing in %s mode.\n",
> +            flask_enforcing ? "enforcing" : "permissive");
> +
>      xfree(bool_pending_values);
>      bool_pending_values = NULL;
>      ret = 0;
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index dad5deb..00c8843 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -1628,28 +1628,32 @@ static struct xsm_operations flask_ops = {
>  
>  static __init void flask_init(void)
>  {
> -    int ret = 0;
> +    int ret = -ENOENT;
>  
> -    if ( !flask_enabled )
> +    if ( flask_bootparam == FLASK_BOOTPARAM_DISABLED )
>      {
> -        printk("Flask:  Disabled at boot.\n");
> +        printk(XENLOG_INFO "Flask:  Disabled at boot.\n");
>          return;
>      }
>  
> -    printk("Flask:  Initializing.\n");
> -
>      avc_init();
>  
>      original_ops = xsm_ops;
>      if ( register_xsm(&flask_ops) )
>          panic("Flask: Unable to register with XSM");
>  
> -    ret = security_load_policy(policy_buffer, policy_size);
> +    if ( policy_size && flask_bootparam != FLASK_BOOTPARAM_LATELOAD )
> +        ret = security_load_policy(policy_buffer, policy_size);
> +
> +    if ( ret && flask_bootparam == FLASK_BOOTPARAM_FORCE )
> +        panic("Unable to load FLASK policy");
>  
> -    if ( flask_enforcing )
> -        printk("Flask:  Starting in enforcing mode.\n");
> +    if ( ret )
> +        printk(XENLOG_INFO "Flask:  Access controls disabled until policy is loaded.\n");
> +    else if ( flask_enforcing )
> +        printk(XENLOG_INFO "Flask:  Starting in enforcing mode.\n");
>      else
> -        printk("Flask:  Starting in permissive mode.\n");
> +        printk(XENLOG_INFO "Flask:  Starting in permissive mode.\n");
>  }
>  
>  xsm_initcall(flask_init);
> diff --git a/xen/xsm/flask/include/security.h b/xen/xsm/flask/include/security.h
> index dea0143..bd2a4bc 100644
> --- a/xen/xsm/flask/include/security.h
> +++ b/xen/xsm/flask/include/security.h
> @@ -35,7 +35,13 @@
>  #define POLICYDB_VERSION_MIN   POLICYDB_VERSION_BASE
>  #define POLICYDB_VERSION_MAX   POLICYDB_VERSION_BOUNDARY
>  
> -extern int flask_enabled;
> +#define FLASK_BOOTPARAM_DEFAULT    0
> +#define FLASK_BOOTPARAM_PERMISSIVE 0
> +#define FLASK_BOOTPARAM_FORCE      1
> +#define FLASK_BOOTPARAM_LATELOAD   2
> +#define FLASK_BOOTPARAM_DISABLED   3
> +
> +extern int flask_bootparam;
>  extern int flask_mls_enabled;
>  
>  int security_load_policy(void * data, size_t len);