Re: [Xen-devel] [PATCH] flask: change default state to enforcing

[Anshul Makkar <anshul.makkar@xxxxxxxxxx>]

-----Original Message-----
From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of Konrad 
Rzeszutek Wilk
Sent: 10 March 2016 19:12
To: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>; Ian Jackson 
<Ian.Jackson@xxxxxxxxxx>; jbeulich@xxxxxxxx
Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; cardoe@xxxxxxxxxx; Andrew Cooper 
<Andrew.Cooper3@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] flask: change default state to enforcing

On Thu, Mar 10, 2016 at 01:30:29PM -0500, Daniel De Graaf wrote:

I've added Ian and Jan on the email as scripts/get_maintainer.pl spits out 
their names (Oddly not yours?)
> The previous default of "permissive" is meant for developing or 
> debugging a disaggregated system.  However, this default makes it too 
> easy to accidentally boot a machine in this state, which does not 
> place any restrictions on guests.  This is not suitable for normal 
> systems because any guest can perform any operation (including 
> operations like rebooting the machine, kexec, and reading or writing 
> another domain's memory).
> 
> This change will cause the boot to fail if you do not specify an XSM 
> policy during boot; if you need to load a policy from dom0, use the 
> "flask=late" boot parameter.
> 
> Originally by Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; modified 
> to also change the default value of flask_enforcing so that the policy 
> is not still in permissive mode.  This also removes the (no longer
> documented) command line argument directly changing that variable 
> since it has been superseded by the flask= parameter.
> 

Reviwed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> .. however:

> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> ---
> 
>  docs/misc/xen-command-line.markdown |  2 +-
>  docs/misc/xsm-flask.txt             | 12 ++++++------
>  xen/xsm/flask/flask_op.c            |  8 +++++---
>  3 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.markdown 
> b/docs/misc/xen-command-line.markdown
> index ca77e3b..9e77f8a 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -662,7 +662,7 @@ to use the default.
>  ### flask
>  > `= permissive | enforcing | late | disabled`
>  
> -> Default: `permissive`
> +> Default: `enforcing`
>  
>  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 diff --git a/docs/misc/xsm-flask.txt 
> b/docs/misc/xsm-flask.txt index fb2fe9f..00a2b13 100644
> --- a/docs/misc/xsm-flask.txt
> +++ b/docs/misc/xsm-flask.txt
> @@ -283,12 +283,12 @@ for passthrough, run:
>  
>  This command must be rerun on each boot or after any policy reload.
>  
> -The example policy was only tested with simple domain creation and 
> may be -missing rules allowing accesses by dom0 or domU when a number 
> of hypervisor -features are used. When first loading or writing a 
> policy, you should run FLASK -in permissive mode (the default) and 
> check the Xen logs (xl dmesg) for AVC -denials before using it in 
> enforcing mode (flask_enforcing=1 on the command -line, or xl setenforce).
> +When first loading or writing a policy, you should run FLASK in 
> +permissive mode (flask=permissive on the command line) and check the 
> +Xen logs (xl dmesg) for AVC denials before using it in enforcing mode 
> +(the default value of the boot parameter, which can also be changed 
> +using xl setenforce).  When using the default types for domains 
> +(domU_t), the example policy shipped with Xen should allow the same 
> operations on or between domains as when not using FLASK.
>  
>  
>  MLS/MCS policy
> diff --git a/xen/xsm/flask/flask_op.c b/xen/xsm/flask/flask_op.c index 
> f4f5dd1..cdb462c 100644
> --- a/xen/xsm/flask/flask_op.c
> +++ b/xen/xsm/flask/flask_op.c
> @@ -25,12 +25,11 @@
>  #define _copy_to_guest copy_to_guest
>  #define _copy_from_guest copy_from_guest
>  
> -enum flask_bootparam_t __read_mostly flask_bootparam = 
> FLASK_BOOTPARAM_PERMISSIVE;
> +enum flask_bootparam_t __read_mostly flask_bootparam = 
> +FLASK_BOOTPARAM_ENFORCING;
>  static void parse_flask_param(char *s);  custom_param("flask", 
> parse_flask_param);
>  
> -bool_t __read_mostly flask_enforcing = 0; 
> -boolean_param("flask_enforcing", flask_enforcing);
> +bool_t __read_mostly flask_enforcing = 1;

Since you set that to the default value should the parse_flask_param 
'flask_enforcing = 1' for the 'enforcing' and 'late' be removed?

(If you agree, the committer could do it).

>  
>  #define MAX_POLICY_SIZE 0x4000000
>  
> @@ -76,7 +75,10 @@ static void __init parse_flask_param(char *s)
>      else if ( !strcmp(s, "disabled") )
>          flask_bootparam = FLASK_BOOTPARAM_DISABLED;
>      else if ( !strcmp(s, "permissive") )
> +    {
> +        flask_enforcing = 0;
>          flask_bootparam = FLASK_BOOTPARAM_PERMISSIVE;
> +    }
>      else
>          flask_bootparam = FLASK_BOOTPARAM_INVALID;  }
> --
> 2.5.0
> 

There is no need to explicitly set flask_enforcing=0.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel