Re: [Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
Thanks Jan.
> >>> On 29.09.18 at 11:22, wrote:
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -899,6 +899,19 @@ hardware domain is architecture dependent.
> > Note that specifying zero as domU value means zero, while for dom0 it
> > means to use the default.
> >
> > +### xsm
> > +> `= dummy | flask`
> > +
> > +> Default: `dummy`
> > +
> > +Specify which XSM module should be enabled. This option is only
> > +available if the hypervisor was compiled with XSM support.
> > +
> > +* `dummy`: this is the default choice. Basic restriction for common
> > +deployment
> > + (the dummy module) will be applied. it's also used when XSM is compiled
> out.
>
> "It's" (upper case I).
>
OK. Done.
> > +* `flask`: this is the policy based access control. To choose this,
> > +the
> > + separated option in kconfig must also be enabled.
>
> Perhaps better "separate" (but I'm not a native speaker)?
>
I prefer separated, as I checked here:
https://english.stackexchange.com/questions/13144/separated-versus-separate
But generally both are OK I think.
> > @@ -154,6 +154,17 @@ config XSM_FLASK_POLICY
> >
> > If unsure, say Y.
> >
> > +choice
> > + prompt "Default XSM implementation"
> > + depends on XSM
> > + default XSM_FLASK_DEFAULT if XSM_FLASK
> > + default XSM_DUMMY_DEFAULT
> > + config XSM_DUMMY_DEFAULT
> > + bool "Match non-XSM behavior"
> > + config XSM_FLASK_DEFAULT
> > + bool "FLux Advanced Security Kernel" if XSM_FLASK endchoice
>
> Personally I'd prefer XSM_DEFAULT_*; not sure what others think.
>
I would like to keep this style, because I see
"
default "credit" if SCHED_CREDIT_DEFAULT
default "credit2" if SCHED_CREDIT2_DEFAULT
" in config file.
> > --- a/xen/xsm/xsm_core.c
> > +++ b/xen/xsm/xsm_core.c
> > @@ -31,6 +31,37 @@
> >
> > struct xsm_operations *xsm_ops;
> >
> > +enum xsm_bootparam {
> > +XSM_BOOTPARAM_DUMMY,
> > +XSM_BOOTPARAM_FLASK,
>
> Considering the #ifdef-s further down, should this perhaps also be put in
> "#ifdef
> CONFIG_XSM_FLASK", to avoid it mistakenly getting used when the code was
> compiled out?
>
I would like to avoid #ifdef here, because I prefer the same enum value in
different config.
> > +};
> > +
> > +static enum xsm_bootparam __initdata xsm_bootparam = #ifdef
> > +CONFIG_XSM_FLASK_DEFAULT
> > +XSM_BOOTPARAM_FLASK;
> > +#else
> > +XSM_BOOTPARAM_DUMMY;
> > +#endif
> > +
> > +static int __init parse_xsm_param(const char *s) {
> > +int rc = 0;
> > +
> > +if ( !strcmp(s, "dummy") )
> > +xsm_bootparam = XSM_BOOTPARAM_DUMMY; #ifdef
> CONFIG_XSM_FLASK
> > +else if ( !strcmp(s, "flask") )
> > +xsm_bootparam = XSM_BOOTPARAM_FLASK; #endif
> > +else {
>
> Style (brace goes on its own line).
OK. Done.
>
> > +printk("XSM: can't parse boot parameter xsm=%s\n", s);
>
> Again, not being a native speaker, "can't parse" sounds odd. Why not just
> "unknown" or "unknown or unsupported"?
OK. Done.
>
> > @@ -57,7 +88,20 @@ static int __init xsm_core_init(const void
> *policy_buffer, size_t policy_size)
> > }
> >
> > xsm_ops = _xsm_ops;
> > -flask_init(policy_buffer, policy_size);
> > +
> > +switch ( xsm_bootparam )
> > +{
> > +case XSM_BOOTPARAM_DUMMY:
> > +break;
> > +
> > +case XSM_BOOTPARAM_FLASK:
> > +flask_init(policy_buffer, policy_size);
> > +break;
> > +
> > +default:
> > +printk("XSM: Invalid value for xsm= boot parameter\n");
> > +break;
>
> What situation is this covering, when all enumerators already have their case
> label? Simply ASSERT_UNREACHABLE()?
>
OK. Done.
> Jan
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
>>> On 29.09.18 at 11:22, wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -899,6 +899,19 @@ hardware domain is architecture dependent.
> Note that specifying zero as domU value means zero, while for dom0 it means
> to use the default.
>
> +### xsm
> +> `= dummy | flask`
> +
> +> Default: `dummy`
> +
> +Specify which XSM module should be enabled. This option is only available if
> +the hypervisor was compiled with XSM support.
> +
> +* `dummy`: this is the default choice. Basic restriction for common
> deployment
> + (the dummy module) will be applied. it's also used when XSM is compiled
> out.
"It's" (upper case I).
> +* `flask`: this is the policy based access control. To choose this, the
> + separated option in kconfig must also be enabled.
Perhaps better "separate" (but I'm not a native speaker)?
> @@ -154,6 +154,17 @@ config XSM_FLASK_POLICY
>
> If unsure, say Y.
>
> +choice
> + prompt "Default XSM implementation"
> + depends on XSM
> + default XSM_FLASK_DEFAULT if XSM_FLASK
> + default XSM_DUMMY_DEFAULT
> + config XSM_DUMMY_DEFAULT
> + bool "Match non-XSM behavior"
> + config XSM_FLASK_DEFAULT
> + bool "FLux Advanced Security Kernel" if XSM_FLASK
> +endchoice
Personally I'd prefer XSM_DEFAULT_*; not sure what others think.
> --- a/xen/xsm/xsm_core.c
> +++ b/xen/xsm/xsm_core.c
> @@ -31,6 +31,37 @@
>
> struct xsm_operations *xsm_ops;
>
> +enum xsm_bootparam {
> +XSM_BOOTPARAM_DUMMY,
> +XSM_BOOTPARAM_FLASK,
Considering the #ifdef-s further down, should this perhaps also be
put in "#ifdef CONFIG_XSM_FLASK", to avoid it mistakenly getting
used when the code was compiled out?
> +};
> +
> +static enum xsm_bootparam __initdata xsm_bootparam =
> +#ifdef CONFIG_XSM_FLASK_DEFAULT
> +XSM_BOOTPARAM_FLASK;
> +#else
> +XSM_BOOTPARAM_DUMMY;
> +#endif
> +
> +static int __init parse_xsm_param(const char *s)
> +{
> +int rc = 0;
> +
> +if ( !strcmp(s, "dummy") )
> +xsm_bootparam = XSM_BOOTPARAM_DUMMY;
> +#ifdef CONFIG_XSM_FLASK
> +else if ( !strcmp(s, "flask") )
> +xsm_bootparam = XSM_BOOTPARAM_FLASK;
> +#endif
> +else {
Style (brace goes on its own line).
> +printk("XSM: can't parse boot parameter xsm=%s\n", s);
Again, not being a native speaker, "can't parse" sounds odd. Why
not just "unknown" or "unknown or unsupported"?
> @@ -57,7 +88,20 @@ static int __init xsm_core_init(const void *policy_buffer,
> size_t policy_size)
> }
>
> xsm_ops = _xsm_ops;
> -flask_init(policy_buffer, policy_size);
> +
> +switch ( xsm_bootparam )
> +{
> +case XSM_BOOTPARAM_DUMMY:
> +break;
> +
> +case XSM_BOOTPARAM_FLASK:
> +flask_init(policy_buffer, policy_size);
> +break;
> +
> +default:
> +printk("XSM: Invalid value for xsm= boot parameter\n");
> +break;
What situation is this covering, when all enumerators already have
their case label? Simply ASSERT_UNREACHABLE()?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
Introduce new boot parameter xsm to choose which xsm module is enabled,
and set default to dummy.
Signed-off-by: Xin Li
---
CC: Daniel De Graaf
CC: George Dunlap
CC: Jan Beulich
CC: Konrad Rzeszutek Wilk
CC: Stefano Stabellini
CC: Tim Deegan
CC: Wei Liu
CC: Sergey Dyasli
CC: Andrew Cooper
CC: Ming Lu
v4:
1. change the default XSM boot parameter name from "default" to "dummy".
2. Kconfig, remove XSM_FLASK from EXPERT.
3. Kconfig, add new choice to select the default XSM module.
---
docs/misc/xen-command-line.markdown | 13
xen/common/Kconfig | 13 +++-
xen/xsm/xsm_core.c | 46 -
3 files changed, 70 insertions(+), 2 deletions(-)
diff --git a/docs/misc/xen-command-line.markdown
b/docs/misc/xen-command-line.markdown
index 1ffd586224..cf9924f53f 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -899,6 +899,19 @@ hardware domain is architecture dependent.
Note that specifying zero as domU value means zero, while for dom0 it means
to use the default.
+### xsm
+> `= dummy | flask`
+
+> Default: `dummy`
+
+Specify which XSM module should be enabled. This option is only available if
+the hypervisor was compiled with XSM support.
+
+* `dummy`: this is the default choice. Basic restriction for common deployment
+ (the dummy module) will be applied. it's also used when XSM is compiled out.
+* `flask`: this is the policy based access control. To choose this, the
+ separated option in kconfig must also be enabled.
+
### flask
> `= permissive | enforcing | late | disabled`
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 1a6d6281c1..f802efb625 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -116,7 +116,7 @@ config XSM
config XSM_FLASK
def_bool y
- prompt "FLux Advanced Security Kernel support" if EXPERT = "y"
+ prompt "FLux Advanced Security Kernel support"
depends on XSM
---help---
Enables FLASK (FLux Advanced Security Kernel) as the access control
@@ -154,6 +154,17 @@ config XSM_FLASK_POLICY
If unsure, say Y.
+choice
+ prompt "Default XSM implementation"
+ depends on XSM
+ default XSM_FLASK_DEFAULT if XSM_FLASK
+ default XSM_DUMMY_DEFAULT
+ config XSM_DUMMY_DEFAULT
+ bool "Match non-XSM behavior"
+ config XSM_FLASK_DEFAULT
+ bool "FLux Advanced Security Kernel" if XSM_FLASK
+endchoice
+
config LATE_HWDOM
bool "Dedicated hardware domain"
default n
diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
index 9645e244c3..df284ec463 100644
--- a/xen/xsm/xsm_core.c
+++ b/xen/xsm/xsm_core.c
@@ -31,6 +31,37 @@
struct xsm_operations *xsm_ops;
+enum xsm_bootparam {
+XSM_BOOTPARAM_DUMMY,
+XSM_BOOTPARAM_FLASK,
+};
+
+static enum xsm_bootparam __initdata xsm_bootparam =
+#ifdef CONFIG_XSM_FLASK_DEFAULT
+XSM_BOOTPARAM_FLASK;
+#else
+XSM_BOOTPARAM_DUMMY;
+#endif
+
+static int __init parse_xsm_param(const char *s)
+{
+int rc = 0;
+
+if ( !strcmp(s, "dummy") )
+xsm_bootparam = XSM_BOOTPARAM_DUMMY;
+#ifdef CONFIG_XSM_FLASK
+else if ( !strcmp(s, "flask") )
+xsm_bootparam = XSM_BOOTPARAM_FLASK;
+#endif
+else {
+printk("XSM: can't parse boot parameter xsm=%s\n", s);
+rc = -EINVAL;
+}
+
+return rc;
+}
+custom_param("xsm", parse_xsm_param);
+
static inline int verify(struct xsm_operations *ops)
{
/* verify the security_operations structure exists */
@@ -57,7 +88,20 @@ static int __init xsm_core_init(const void *policy_buffer,
size_t policy_size)
}
xsm_ops = _xsm_ops;
-flask_init(policy_buffer, policy_size);
+
+switch ( xsm_bootparam )
+{
+case XSM_BOOTPARAM_DUMMY:
+break;
+
+case XSM_BOOTPARAM_FLASK:
+flask_init(policy_buffer, policy_size);
+break;
+
+default:
+printk("XSM: Invalid value for xsm= boot parameter\n");
+break;
+}
return 0;
}
--
2.18.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
On 09/28/2018 04:18 AM, Xin Li wrote: Introduce new boot parameter xsm to choose which xsm module is enabled, and set default to dummy. Signed-off-by: Xin Li This changes the default behavior of a hypervisor compiled with XSM+FLASK when booted with no command line arguments from enabling FLASK to enabling the dummy module. I think the default value of the "xsm=" parameter should be settable in Kconfig to allow existing systems to continue working after an upgrade. If not, this new command line argument needs to be mentioned in more locations in the documentation; at least docs/misc/xsm-flask "Setting up FLASK" will need to mention it. I think a mention in the release notes for the next version is also a good idea (in addition to or as part of the note on the new SILO feature), but that's not a part of the patch. Untested Kconfig snippet: choice prompt "Default XSM implementation" default XSM_FLASK_DEFAULT if XSM_FLASK default XSM_SILO_DEFAULT if XSM_SILO default XSM_DUMMY_DEFAULT config XSM_DUMMY_DEFAULT bool "Match non-XSM behavior" config XSM_FLASK_DEFAULT bool "FLux Advanced Security Kernel" if XSM_FLASK config XSM_SILO_DEFAULT bool "SILO" if XSM_SILO endchoice The multiple "default" statements are intended to cause the default to be the chosen enabled system, or dummy if there are no existing Kconfig settings. I also think the question for XSM_FLASK should be removed from EXPERT now that there is a reason to enable XSM without FLASK. The name "default" might end up being misleading in this case; "none", "off", or "dummy" might be better. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
Introduce new boot parameter xsm to choose which xsm module is enabled,
and set default to dummy.
Signed-off-by: Xin Li
---
CC: Daniel De Graaf
CC: George Dunlap
CC: Jan Beulich
CC: Konrad Rzeszutek Wilk
CC: Stefano Stabellini
CC: Tim Deegan
CC: Wei Liu
CC: Sergey Dyasli
CC: Andrew Cooper
CC: Ming Lu
v3: change the default XSM boot parameter name from "dummy" to "default".
---
docs/misc/xen-command-line.markdown | 13 +
xen/xsm/xsm_core.c | 41 -
2 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/docs/misc/xen-command-line.markdown
b/docs/misc/xen-command-line.markdown
index 1ffd586224..6a3c0e71c7 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -899,6 +899,19 @@ hardware domain is architecture dependent.
Note that specifying zero as domU value means zero, while for dom0 it means
to use the default.
+### xsm
+> `= default | flask`
+
+> Default: `default`
+
+Specify which XSM module should be enabled. This option is only available if
+the hypervisor was compiled with XSM support.
+
+* `default`: this is the default choice. Basic restriction for common
deployment
+ (the dummy module) will be applied. it's also used when XSM is compiled out.
+* `flask`: this is the policy based access control. To choose this, the
+ separated option in kconfig must also be enabled.
+
### flask
> `= permissive | enforcing | late | disabled`
diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
index 9645e244c3..658af40c6e 100644
--- a/xen/xsm/xsm_core.c
+++ b/xen/xsm/xsm_core.c
@@ -31,6 +31,32 @@
struct xsm_operations *xsm_ops;
+enum xsm_bootparam {
+XSM_BOOTPARAM_DUMMY,
+XSM_BOOTPARAM_FLASK,
+};
+
+static enum xsm_bootparam __initdata xsm_bootparam = XSM_BOOTPARAM_DUMMY;
+
+static int __init parse_xsm_param(const char *s)
+{
+int rc = 0;
+
+if ( !strcmp(s, "default") )
+xsm_bootparam = XSM_BOOTPARAM_DUMMY;
+#ifdef CONFIG_XSM_FLASK
+else if ( !strcmp(s, "flask") )
+xsm_bootparam = XSM_BOOTPARAM_FLASK;
+#endif
+else {
+printk("XSM: can't parse boot parameter xsm=%s\n", s);
+rc = -EINVAL;
+}
+
+return rc;
+}
+custom_param("xsm", parse_xsm_param);
+
static inline int verify(struct xsm_operations *ops)
{
/* verify the security_operations structure exists */
@@ -57,7 +83,20 @@ static int __init xsm_core_init(const void *policy_buffer,
size_t policy_size)
}
xsm_ops = _xsm_ops;
-flask_init(policy_buffer, policy_size);
+
+switch ( xsm_bootparam )
+{
+case XSM_BOOTPARAM_DUMMY:
+break;
+
+case XSM_BOOTPARAM_FLASK:
+flask_init(policy_buffer, policy_size);
+break;
+
+default:
+printk("XSM: Invalid value for xsm= boot parameter\n");
+break;
+}
return 0;
}
--
2.18.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
On 07/02/2018 09:26 PM, Xin Li wrote: Introduce new boot parameter xsm to choose which xsm module is enabled, and set default to dummy. Signed-off-by: Xin Li This is a change in defaults for the command line: previously, if you compiled Xen with FLASK support, Xen defaulted to using it unless you specified "flask=disabled" on the command line. If we want to model the interface after Linux, there would be a KConfig choice of the default which can be overridden by the command line, so distributions can keep current behavior (including making the default dummy while enabling the other XSM modules). ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
> -Original Message- > From: George Dunlap > Sent: Thursday, July 5, 2018 12:55 AM > To: Jan Beulich > Cc: Xin Li ; Andrew Cooper > ; Ming Lu ; Sergey Dyasli > ; Wei Liu ; Xin Li (Talons) > ; George Dunlap ; Stefano > Stabellini ; xen-devel ; > Konrad Rzeszutek Wilk ; Daniel de Graaf > ; Tim (Xen.org) > Subject: Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm > > > > > On Jul 3, 2018, at 8:12 AM, Jan Beulich wrote: > > > > First of all - please indicate the version also in the subject, i.e. > > here [PATCH v2 1/2] or some such. > > > On 03.07.18 at 03:26, wrote: > >> v2 > >> To further discuss: > >> 1) is "dummy" a good command line option? > >> other choices: basic", "trivial", or "simple" > > > > Indeed, but not limited to the named set. > > I think I’d go with “standard” or “default”. The “default” restrictions are > by no > means “dummy”, “trivial”, or “simple” — they’re carefully thought out for the > most common Xen deployment. "default" seems to be a good choice. It matches the code logic. if ( !strcmp(s, "default") ) xsm_bootparam = XSM_BOOTPARAM_DUMMY; also update " docs/misc/xen-command-line.markdown" > > -George Best regards Xin(Talons) Li ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Tuesday, July 3, 2018 3:12 PM
> To: Xin Li
> Cc: Andrew Cooper ; Ming Lu
> ; Sergey Dyasli ; Wei Liu
> ; Xin Li (Talons) ; George Dunlap
> ; Stefano Stabellini ; xen-
> de...@lists.xen.org; Konrad Rzeszutek Wilk ; Daniel
> de Graaf ; Tim (Xen.org)
> Subject: Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
>
> First of all - please indicate the version also in the subject, i.e. here
> [PATCH v2
> 1/2] or some such.
>
> >>> On 03.07.18 at 03:26, wrote:
> > v2
> > To further discuss:
> > 1) is "dummy" a good command line option?
> > other choices: basic", "trivial", or "simple"
>
> Indeed, but not limited to the named set.
>
> Additionally, please have a brief summary of changes from the prior version
> here.
>
> > +switch ( xsm_bootparam )
> > +{
> > +case XSM_BOOTPARAM_DUMMY:
> > +break;
> > +
> > +case XSM_BOOTPARAM_FLASK:
> > +flask_init(policy_buffer, policy_size);
> > +break;
> > +
> > +default:
> > +printk("XSM: Invalid value for xsm= boot parameter.\n");
>
> As I think I've said before - generally no full stop at the end of log
> messages
> please. I also think that in error messages like this the offending string
> should
> be logged as well. Which points out an issue with the change: Without
> CONFIG_XSM_FLASK (under the current naming as proposed by Andrew; you
> should btw continue to name the dependency of your series on his one until
> that prereq has landed in staging, which you'd ideally do in a 0/2 cover
> letter) would perhaps better result in this error message to be issued, in
> favor
> of or in addition to the command line parsing one.
OK.
Remove this ".",
And in parse_xsm_param, add:
else {
printk("XSM: can't parse boot parameter xsm=%s\n", s);
rc = -EINVAL;
}
>
> Jan
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
First of all - please indicate the version also in the subject, i.e. here
[PATCH v2 1/2] or some such.
>>> On 03.07.18 at 03:26, wrote:
> v2
> To further discuss:
> 1) is "dummy" a good command line option?
> other choices: basic", "trivial", or "simple"
Indeed, but not limited to the named set.
Additionally, please have a brief summary of changes from the prior
version here.
> +switch ( xsm_bootparam )
> +{
> +case XSM_BOOTPARAM_DUMMY:
> +break;
> +
> +case XSM_BOOTPARAM_FLASK:
> +flask_init(policy_buffer, policy_size);
> +break;
> +
> +default:
> +printk("XSM: Invalid value for xsm= boot parameter.\n");
As I think I've said before - generally no full stop at the end of log
messages please. I also think that in error messages like this the
offending string should be logged as well. Which points out an
issue with the change: Without CONFIG_XSM_FLASK (under the
current naming as proposed by Andrew; you should btw continue
to name the dependency of your series on his one until that
prereq has landed in staging, which you'd ideally do in a 0/2 cover
letter) would perhaps better result in this error message to be
issued, in favor of or in addition to the command line parsing one.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
> -Original Message-
> From: Xin Li [mailto:talons@gmail.com]
> Sent: Tuesday, July 3, 2018 9:26 AM
> To: xen-de...@lists.xen.org
> Cc: Xin Li (Talons) ; Daniel De Graaf
> ; George Dunlap ; Jan
> Beulich ; Konrad Rzeszutek Wilk
> ; Stefano Stabellini ; Tim
> (Xen.org) ; Wei Liu ; Sergey Dyasli
> ; Andrew Cooper ;
> Ming Lu
> Subject: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
>
> Introduce new boot parameter xsm to choose which xsm module is enabled,
> and set default to dummy.
>
> Signed-off-by: Xin Li
>
> ---
> CC: Daniel De Graaf
> CC: George Dunlap
> CC: Jan Beulich
> CC: Konrad Rzeszutek Wilk
> CC: Stefano Stabellini
> CC: Tim Deegan
> CC: Wei Liu
> CC: Sergey Dyasli
> CC: Andrew Cooper
> CC: Ming Lu
>
> v2
> To further discuss:
> 1) is "dummy" a good command line option?
> other choices: basic", "trivial", or "simple"
>
> ---
> docs/misc/xen-command-line.markdown | 13 ++
> xen/xsm/xsm_core.c | 39 -
> 2 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-
> command-line.markdown
> index 075e5ea159..7ca34aa273 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -865,6 +865,19 @@ hardware domain is architecture dependent.
> Note that specifying zero as domU value means zero, while for dom0 it means
> to use the default.
>
> +### xsm
> +> `= dummy | flask`
> +
> +> Default: `dummy`
> +
> +Specify which XSM module should be enabled. This option is only
> +available if the hypervisor was compiled with XSM support.
> +
> +* `dummy`: this is the default choice. No special restriction will be
> applied.
> + it's also used when XSM is compiled out.
> +* `flask`: this is the policy based access control. To choose this,
> +the
> + separated option in kconfig must also be enabled.
> +
> ### flask
> > `= permissive | enforcing | late | disabled`
>
> diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c index
> cddcf7aa51..d4668edad7 100644
> --- a/xen/xsm/xsm_core.c
> +++ b/xen/xsm/xsm_core.c
> @@ -31,6 +31,30 @@
>
> struct xsm_operations *xsm_ops;
>
> +enum xsm_bootparam {
> +XSM_BOOTPARAM_DUMMY,
> +XSM_BOOTPARAM_FLASK,
> +};
> +
> +static enum xsm_bootparam __initdata xsm_bootparam = XSM_BOOTPARAM_DUMMY;
New line here.
>+static int __init parse_xsm_param(const char *s)
> {
> +int rc = 0;
> +
> +if ( !strcmp(s, "dummy") )
> +xsm_bootparam = XSM_BOOTPARAM_DUMMY; #ifdef
> CONFIG_XSM_FLASK
> +else if ( !strcmp(s, "flask") )
> +xsm_bootparam = XSM_BOOTPARAM_FLASK; #endif
> +else
> +rc = -EINVAL;
> +
> +return rc;
> +}
No new line here.
> +custom_param("xsm", parse_xsm_param);
> +
> static inline int verify(struct xsm_operations *ops) {
> /* verify the security_operations structure exists */ @@ -57,7 +81,20 @@
> static int __init xsm_core_init(const void *policy_buffer, size_t policy_size)
> }
>
> xsm_ops = _xsm_ops;
> -flask_init(policy_buffer, policy_size);
> +
> +switch ( xsm_bootparam )
> +{
> +case XSM_BOOTPARAM_DUMMY:
> +break;
> +
> +case XSM_BOOTPARAM_FLASK:
> +flask_init(policy_buffer, policy_size);
> +break;
> +
> +default:
> +printk("XSM: Invalid value for xsm= boot parameter.\n");
> +break;
> +}
>
> return 0;
> }
> --
> 2.18.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
Introduce new boot parameter xsm to choose which xsm module is enabled,
and set default to dummy.
Signed-off-by: Xin Li
---
CC: Daniel De Graaf
CC: George Dunlap
CC: Jan Beulich
CC: Konrad Rzeszutek Wilk
CC: Stefano Stabellini
CC: Tim Deegan
CC: Wei Liu
CC: Sergey Dyasli
CC: Andrew Cooper
CC: Ming Lu
v2
To further discuss:
1) is "dummy" a good command line option?
other choices: basic", "trivial", or "simple"
---
docs/misc/xen-command-line.markdown | 13 ++
xen/xsm/xsm_core.c | 39 -
2 files changed, 51 insertions(+), 1 deletion(-)
diff --git a/docs/misc/xen-command-line.markdown
b/docs/misc/xen-command-line.markdown
index 075e5ea159..7ca34aa273 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -865,6 +865,19 @@ hardware domain is architecture dependent.
Note that specifying zero as domU value means zero, while for dom0 it means
to use the default.
+### xsm
+> `= dummy | flask`
+
+> Default: `dummy`
+
+Specify which XSM module should be enabled. This option is only available if
+the hypervisor was compiled with XSM support.
+
+* `dummy`: this is the default choice. No special restriction will be applied.
+ it's also used when XSM is compiled out.
+* `flask`: this is the policy based access control. To choose this, the
+ separated option in kconfig must also be enabled.
+
### flask
> `= permissive | enforcing | late | disabled`
diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
index cddcf7aa51..d4668edad7 100644
--- a/xen/xsm/xsm_core.c
+++ b/xen/xsm/xsm_core.c
@@ -31,6 +31,30 @@
struct xsm_operations *xsm_ops;
+enum xsm_bootparam {
+XSM_BOOTPARAM_DUMMY,
+XSM_BOOTPARAM_FLASK,
+};
+
+static enum xsm_bootparam __initdata xsm_bootparam = XSM_BOOTPARAM_DUMMY;
+static int __init parse_xsm_param(const char *s)
+{
+int rc = 0;
+
+if ( !strcmp(s, "dummy") )
+xsm_bootparam = XSM_BOOTPARAM_DUMMY;
+#ifdef CONFIG_XSM_FLASK
+else if ( !strcmp(s, "flask") )
+xsm_bootparam = XSM_BOOTPARAM_FLASK;
+#endif
+else
+rc = -EINVAL;
+
+return rc;
+}
+
+custom_param("xsm", parse_xsm_param);
+
static inline int verify(struct xsm_operations *ops)
{
/* verify the security_operations structure exists */
@@ -57,7 +81,20 @@ static int __init xsm_core_init(const void *policy_buffer,
size_t policy_size)
}
xsm_ops = _xsm_ops;
-flask_init(policy_buffer, policy_size);
+
+switch ( xsm_bootparam )
+{
+case XSM_BOOTPARAM_DUMMY:
+break;
+
+case XSM_BOOTPARAM_FLASK:
+flask_init(policy_buffer, policy_size);
+break;
+
+default:
+printk("XSM: Invalid value for xsm= boot parameter.\n");
+break;
+}
return 0;
}
--
2.18.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
>>> On 02.07.18 at 10:39, wrote: >> From: Jan Beulich [mailto:jbeul...@suse.com] >> Sent: Monday, July 2, 2018 4:25 PM >> >>> On 02.07.18 at 09:34, wrote: >> >> From: Jan Beulich [mailto:jbeul...@suse.com] >> >> Sent: Friday, June 29, 2018 6:05 PM >> >> >>> On 29.06.18 at 11:47, wrote: >> >> > On 29/06/18 10:28, Xin Li wrote: >> >> >> --- a/docs/misc/xen-command-line.markdown >> >> >> +++ b/docs/misc/xen-command-line.markdown >> >> >> @@ -865,6 +865,19 @@ hardware domain is architecture dependent. >> >> >> Note that specifying zero as domU value means zero, while for >> >> >> dom0 it means to use the default. >> >> >> >> >> >> +### xsm >> >> >> +> `= dummy | silo | flask` >> >> > >> >> > This should be just "dummy | flask" in this patch, and extended >> >> > with silo in the next path. Also, options in this file should be >> >> > sorted alphabetically, so ### xsm should be near the end, rather than >> beside flask. >> >> > >> >> >> + >> >> >> +> Default: `dummy` >> >> >> + >> >> >> +Specify which XSM module should be enabled. This option is only >> >> >> +available if the hypervisor was compiled with XSM support. >> >> >> + >> >> >> +* `dummy`: this is the default choice. No special restriction will be >> applied. >> >> >> + it's also used when XSM is compiled out. >> >> >> +enum xsm_bootparam __read_mostly xsm_bootparam = >> >> >> +XSM_BOOTPARAM_DUMMY; >> >> >> >> So why "dummy" instead of "none" (or one of the boolean false strings)? >> > >> > It seems dummy is not fully stub. (some check by XSM_* classification) >> > So we want to keep this "dummy" check, and override it. >> >> Right, except that "dummy", while a reasonable name internally to the >> implementation, is at least of questionable use/meaning as a part of a >> command line option. > is it better to keep them the same? Well, I'm not entirely sure, hence I've made the original remark in the first place. Command line options should be reasonably meaningful to those potentially having to use / remember them, so there are cases where they're better not in 1:1 relationship with naming in source code. If "none" isn't applicable, how about "basic", "trivial", or "simple"? Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
Hello Jan
> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Monday, July 2, 2018 4:25 PM
> To: Xin Li (Talons) ; Xin Li
> Cc: Andrew Cooper ; George Dunlap
> ; Ming Lu ; Sergey Dyasli
> ; Wei Liu ; Stefano Stabellini
> ; xen-de...@lists.xen.org; Konrad Rzeszutek Wilk
> ; Daniel de Graaf ; Tim
> (Xen.org)
> Subject: RE: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
>
> >>> On 02.07.18 at 09:34, wrote:
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: Friday, June 29, 2018 6:05 PM
> >> >>> On 29.06.18 at 11:47, wrote:
> >> > On 29/06/18 10:28, Xin Li wrote:
> >> >> --- a/docs/misc/xen-command-line.markdown
> >> >> +++ b/docs/misc/xen-command-line.markdown
> >> >> @@ -865,6 +865,19 @@ hardware domain is architecture dependent.
> >> >> Note that specifying zero as domU value means zero, while for
> >> >> dom0 it means to use the default.
> >> >>
> >> >> +### xsm
> >> >> +> `= dummy | silo | flask`
> >> >
> >> > This should be just "dummy | flask" in this patch, and extended
> >> > with silo in the next path. Also, options in this file should be
> >> > sorted alphabetically, so ### xsm should be near the end, rather than
> beside flask.
> >> >
> >> >> +
> >> >> +> Default: `dummy`
> >> >> +
> >> >> +Specify which XSM module should be enabled. This option is only
> >> >> +available if the hypervisor was compiled with XSM support.
> >> >> +
> >> >> +* `dummy`: this is the default choice. No special restriction will be
> applied.
> >> >> + it's also used when XSM is compiled out.
> >> >> +enum xsm_bootparam __read_mostly xsm_bootparam =
> >> >> +XSM_BOOTPARAM_DUMMY;
> >>
> >> So why "dummy" instead of "none" (or one of the boolean false strings)?
> >
> > It seems dummy is not fully stub. (some check by XSM_* classification)
> > So we want to keep this "dummy" check, and override it.
>
> Right, except that "dummy", while a reasonable name internally to the
> implementation, is at least of questionable use/meaning as a part of a
> command line option.
is it better to keep them the same?
>
> >> >> @@ -57,7 +81,20 @@ static int __init xsm_core_init(const void
> >> *policy_buffer, size_t policy_size)
> >> >> }
> >> >>
> >> >> xsm_ops = _xsm_ops;
> >> >> -flask_init(policy_buffer, policy_size);
> >> >> +
> >> >> +switch ( xsm_bootparam )
> >> >> +{
> >> >> +case XSM_BOOTPARAM_DUMMY:
> >> >> +/* empty */
> >>
> >> I'm not sure of the value of this comment.
> > I just want avoid an empty switch case.
>
> Well, it's not empty because of ...
>
> >> >> +break;
>
> ... this (without which it would be a fall-through one).
Oh, OK, remove this comment.
>
> Jan
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
>>> On 02.07.18 at 09:34, wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: Friday, June 29, 2018 6:05 PM
>> >>> On 29.06.18 at 11:47, wrote:
>> > On 29/06/18 10:28, Xin Li wrote:
>> >> --- a/docs/misc/xen-command-line.markdown
>> >> +++ b/docs/misc/xen-command-line.markdown
>> >> @@ -865,6 +865,19 @@ hardware domain is architecture dependent.
>> >> Note that specifying zero as domU value means zero, while for dom0
>> >> it means to use the default.
>> >>
>> >> +### xsm
>> >> +> `= dummy | silo | flask`
>> >
>> > This should be just "dummy | flask" in this patch, and extended with
>> > silo in the next path. Also, options in this file should be sorted
>> > alphabetically, so ### xsm should be near the end, rather than beside
>> > flask.
>> >
>> >> +
>> >> +> Default: `dummy`
>> >> +
>> >> +Specify which XSM module should be enabled. This option is only
>> >> +available if the hypervisor was compiled with XSM support.
>> >> +
>> >> +* `dummy`: this is the default choice. No special restriction will be
>> >> applied.
>> >> + it's also used when XSM is compiled out.
>> >> +enum xsm_bootparam __read_mostly xsm_bootparam =
>> >> +XSM_BOOTPARAM_DUMMY;
>>
>> So why "dummy" instead of "none" (or one of the boolean false strings)?
>
> It seems dummy is not fully stub. (some check by XSM_* classification)
> So we want to keep this "dummy" check, and override it.
Right, except that "dummy", while a reasonable name internally to the
implementation, is at least of questionable use/meaning as a part of a
command line option.
>> >> @@ -57,7 +81,20 @@ static int __init xsm_core_init(const void
>> *policy_buffer, size_t policy_size)
>> >> }
>> >>
>> >> xsm_ops = _xsm_ops;
>> >> -flask_init(policy_buffer, policy_size);
>> >> +
>> >> +switch ( xsm_bootparam )
>> >> +{
>> >> +case XSM_BOOTPARAM_DUMMY:
>> >> +/* empty */
>>
>> I'm not sure of the value of this comment.
> I just want avoid an empty switch case.
Well, it's not empty because of ...
>> >> +break;
... this (without which it would be a fall-through one).
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
Hello Jan,
> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Friday, June 29, 2018 6:05 PM
> To: Andrew Cooper ; Xin Li
>
> Cc: Ming Lu ; Sergey Dyasli ;
> Wei Liu ; Xin Li (Talons) ; George
> Dunlap ; Stefano Stabellini
> ; xen-de...@lists.xen.org; Konrad Rzeszutek Wilk
> ; Daniel de Graaf ; Tim
> (Xen.org)
> Subject: Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
>
> >>> On 29.06.18 at 11:47, wrote:
> > On 29/06/18 10:28, Xin Li wrote:
> >> --- a/docs/misc/xen-command-line.markdown
> >> +++ b/docs/misc/xen-command-line.markdown
> >> @@ -865,6 +865,19 @@ hardware domain is architecture dependent.
> >> Note that specifying zero as domU value means zero, while for dom0
> >> it means to use the default.
> >>
> >> +### xsm
> >> +> `= dummy | silo | flask`
> >
> > This should be just "dummy | flask" in this patch, and extended with
> > silo in the next path. Also, options in this file should be sorted
> > alphabetically, so ### xsm should be near the end, rather than beside flask.
> >
> >> +
> >> +> Default: `dummy`
> >> +
> >> +Specify which XSM module should be enabled. This option is only
> >> +available if the hypervisor was compiled with XSM support.
> >> +
> >> +* `dummy`: this is the default choice. No special restriction will be
> >> applied.
> >> + it's also used when XSM is compiled out.
> >> +enum xsm_bootparam __read_mostly xsm_bootparam =
> >> +XSM_BOOTPARAM_DUMMY;
>
> So why "dummy" instead of "none" (or one of the boolean false strings)?
It seems dummy is not fully stub. (some check by XSM_* classification)
So we want to keep this "dummy" check, and override it.
>
> > This should be __initdata rather than __read_mostly. It is safe to be
> > discarded after boot.
>
> And static.
OK. Done.
>
> >> +static int __init parse_xsm_param(const char *s) {
> >
> > int rc = 0;
> >
> >> +if ( !strcmp(s, "dummy") )
> >> +xsm_bootparam = XSM_BOOTPARAM_DUMMY; #ifdef
> CONFIG_XSM_FLASK
> >> +else if ( !strcmp(s, "flask") )
> >> +xsm_bootparam = XSM_BOOTPARAM_FLASK; #endif
> >> +else
> >> +xsm_bootparam = XSM_BOOTPARAM_INVALID;
> >> +
> >> +return 0;
> >
> > else
> > rc = -EINVAL;
> >
> > return rc;
> >
> > As a result, the core command line infrastructure will inform the user
> > if they passed an unrecognised option.
> >
> > ~Andrew
> >
> >> +}
> >> +
> >> +custom_param("xsm", parse_xsm_param);
>
> Please avoid the blank line above here - in the majority of similar cases, we
> have the custom_param() immediately follow the parsing function.
OK. Done.
>
> >> @@ -57,7 +81,20 @@ static int __init xsm_core_init(const void
> *policy_buffer, size_t policy_size)
> >> }
> >>
> >> xsm_ops = _xsm_ops;
> >> -flask_init(policy_buffer, policy_size);
> >> +
> >> +switch ( xsm_bootparam )
> >> +{
> >> +case XSM_BOOTPARAM_DUMMY:
> >> +/* empty */
>
> I'm not sure of the value of this comment.
I just want avoid an empty switch case.
>
> >> +break;
> >> +
> >> +case XSM_BOOTPARAM_FLASK:
> >> +flask_init(policy_buffer, policy_size);
> >> +break;
> >> +
> >> +default:
> >> +printk("XSM: Invalid value for xsm= boot parameter.\n");
> >> +}
>
> Please don't omit the "break;" here.
Sure, done.
>
> Jan
>
Best regards
Xin(Talons) Li
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
Hell Andrew,
> -Original Message-
> From: Andrew Cooper
> Sent: Friday, June 29, 2018 5:48 PM
> To: Xin Li ; xen-de...@lists.xen.org
> Cc: Xin Li (Talons) ; Daniel De Graaf
> ; George Dunlap ; Jan
> Beulich ; Konrad Rzeszutek Wilk
> ; Stefano Stabellini ; Tim
> (Xen.org) ; Wei Liu ; Sergey Dyasli
> ; Ming Lu
> Subject: Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
>
> On 29/06/18 10:28, Xin Li wrote:
> > Introduce new boot parameter xsm to choose which xsm module is
> > enabled, and set default to dummy.
> >
> > Signed-off-by: Xin Li
>
> As a note for other reviewers, this series is based on top of my XSM Kconfig
> cleanup.
>
> As for this patch, its almost there. Just a few minor issues.
>
> >
> > ---
> > CC: Daniel De Graaf
> > CC: George Dunlap
> > CC: Jan Beulich
> > CC: Konrad Rzeszutek Wilk
> > CC: Stefano Stabellini
> > CC: Tim Deegan
> > CC: Wei Liu
> > CC: Sergey Dyasli
> > CC: Andrew Cooper
> > CC: Ming Lu
> > ---
> > docs/misc/xen-command-line.markdown | 13 ++
> > xen/xsm/xsm_core.c | 39 -
> > 2 files changed, 51 insertions(+), 1 deletion(-)
> >
> > diff --git a/docs/misc/xen-command-line.markdown
> > b/docs/misc/xen-command-line.markdown
> > index 075e5ea159..7c689b8225 100644
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -865,6 +865,19 @@ hardware domain is architecture dependent.
> > Note that specifying zero as domU value means zero, while for dom0 it
> > means to use the default.
> >
> > +### xsm
> > +> `= dummy | silo | flask`
>
> This should be just "dummy | flask" in this patch, and extended with silo in
> the
> next path. Also, options in this file should be sorted alphabetically, so
> ### xsm
> should be near the end, rather than beside flask.
Right. Done.
>
> > +
> > +> Default: `dummy`
> > +
> > +Specify which XSM module should be enabled. This option is only
> > +available if the hypervisor was compiled with XSM support.
> > +
> > +* `dummy`: this is the default choice. No special restriction will be
> > applied.
> > + it's also used when XSM is compiled out.
> > +* `flask`: this is the policy based access control. To choose this,
> > +the
> > + separated option in kconfig must also be enabled.
> > +
> > ### flask
> > > `= permissive | enforcing | late | disabled`
> >
> > diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c index
> > cddcf7aa51..e002200578 100644
> > --- a/xen/xsm/xsm_core.c
> > +++ b/xen/xsm/xsm_core.c
> > @@ -31,6 +31,30 @@
> >
> > struct xsm_operations *xsm_ops;
> >
> > +enum xsm_bootparam {
> > +XSM_BOOTPARAM_DUMMY,
> > +XSM_BOOTPARAM_FLASK,
> > +XSM_BOOTPARAM_INVALID,
>
> I'd drop INVALID (See below for the parsing aspect), as it actually falls
> back to
> DUMMY.
>
> > +};
> > +
> > +enum xsm_bootparam __read_mostly xsm_bootparam =
> XSM_BOOTPARAM_DUMMY;
>
> This should be __initdata rather than __read_mostly. It is safe to be
> discarded
> after boot.
>
> > +
> > +static int __init parse_xsm_param(const char *s) {
>
> int rc = 0;
>
> > +if ( !strcmp(s, "dummy") )
> > +xsm_bootparam = XSM_BOOTPARAM_DUMMY; #ifdef
> CONFIG_XSM_FLASK
> > +else if ( !strcmp(s, "flask") )
> > +xsm_bootparam = XSM_BOOTPARAM_FLASK; #endif
> > +else
> > +xsm_bootparam = XSM_BOOTPARAM_INVALID;
> > +
> > +return 0;
>
> else
> rc = -EINVAL;
>
> return rc;
>
> As a result, the core command line infrastructure will inform the user if they
> passed an unrecognised option.
>
OK. Thank you.
> ~Andrew
>
> > +}
> > +
> > +custom_param("xsm", parse_xsm_param);
> > +
> > static inline int verify(struct xsm_operations *ops) {
> > /* verify the security_operations structure exists */ @@ -57,7
> > +81,20 @@ static int __init xsm_core_init(const void *policy_buffer, size_t
> policy_size)
> > }
> >
> > xsm_ops = _xsm_ops;
> > -flask_init(policy_buffer, policy_size);
> > +
> > +switch ( xsm_bootparam )
> > +{
> > +case XSM_BOOTPARAM_DUMMY:
> > +/* empty */
> > +break;
> > +
> > +case XSM_BOOTPARAM_FLASK:
> > +flask_init(policy_buffer, policy_size);
> > +break;
> > +
> > +default:
> > +printk("XSM: Invalid value for xsm= boot parameter.\n");
> > +}
> >
> > return 0;
> > }
Best regards
Xin(Talons) Li
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
>>> On 29.06.18 at 11:47, wrote:
> On 29/06/18 10:28, Xin Li wrote:
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -865,6 +865,19 @@ hardware domain is architecture dependent.
>> Note that specifying zero as domU value means zero, while for dom0 it means
>> to use the default.
>>
>> +### xsm
>> +> `= dummy | silo | flask`
>
> This should be just "dummy | flask" in this patch, and extended with
> silo in the next path. Also, options in this file should be sorted
> alphabetically, so ### xsm should be near the end, rather than beside flask.
>
>> +
>> +> Default: `dummy`
>> +
>> +Specify which XSM module should be enabled. This option is only available
>> if
>> +the hypervisor was compiled with XSM support.
>> +
>> +* `dummy`: this is the default choice. No special restriction will be
>> applied.
>> + it's also used when XSM is compiled out.
>> +enum xsm_bootparam __read_mostly xsm_bootparam = XSM_BOOTPARAM_DUMMY;
So why "dummy" instead of "none" (or one of the boolean false
strings)?
> This should be __initdata rather than __read_mostly. It is safe to be
> discarded after boot.
And static.
>> +static int __init parse_xsm_param(const char *s)
>> +{
>
> int rc = 0;
>
>> +if ( !strcmp(s, "dummy") )
>> +xsm_bootparam = XSM_BOOTPARAM_DUMMY;
>> +#ifdef CONFIG_XSM_FLASK
>> +else if ( !strcmp(s, "flask") )
>> +xsm_bootparam = XSM_BOOTPARAM_FLASK;
>> +#endif
>> +else
>> +xsm_bootparam = XSM_BOOTPARAM_INVALID;
>> +
>> +return 0;
>
> else
> rc = -EINVAL;
>
> return rc;
>
> As a result, the core command line infrastructure will inform the user
> if they passed an unrecognised option.
>
> ~Andrew
>
>> +}
>> +
>> +custom_param("xsm", parse_xsm_param);
Please avoid the blank line above here - in the majority of similar cases,
we have the custom_param() immediately follow the parsing function.
>> @@ -57,7 +81,20 @@ static int __init xsm_core_init(const void
>> *policy_buffer, size_t policy_size)
>> }
>>
>> xsm_ops = _xsm_ops;
>> -flask_init(policy_buffer, policy_size);
>> +
>> +switch ( xsm_bootparam )
>> +{
>> +case XSM_BOOTPARAM_DUMMY:
>> +/* empty */
I'm not sure of the value of this comment.
>> +break;
>> +
>> +case XSM_BOOTPARAM_FLASK:
>> +flask_init(policy_buffer, policy_size);
>> +break;
>> +
>> +default:
>> +printk("XSM: Invalid value for xsm= boot parameter.\n");
>> +}
Please don't omit the "break;" here.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
Introduce new boot parameter xsm to choose which xsm module is enabled,
and set default to dummy.
Signed-off-by: Xin Li
---
CC: Daniel De Graaf
CC: George Dunlap
CC: Jan Beulich
CC: Konrad Rzeszutek Wilk
CC: Stefano Stabellini
CC: Tim Deegan
CC: Wei Liu
CC: Sergey Dyasli
CC: Andrew Cooper
CC: Ming Lu
---
docs/misc/xen-command-line.markdown | 13 ++
xen/xsm/xsm_core.c | 39 -
2 files changed, 51 insertions(+), 1 deletion(-)
diff --git a/docs/misc/xen-command-line.markdown
b/docs/misc/xen-command-line.markdown
index 075e5ea159..7c689b8225 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -865,6 +865,19 @@ hardware domain is architecture dependent.
Note that specifying zero as domU value means zero, while for dom0 it means
to use the default.
+### xsm
+> `= dummy | silo | flask`
+
+> Default: `dummy`
+
+Specify which XSM module should be enabled. This option is only available if
+the hypervisor was compiled with XSM support.
+
+* `dummy`: this is the default choice. No special restriction will be applied.
+ it's also used when XSM is compiled out.
+* `flask`: this is the policy based access control. To choose this, the
+ separated option in kconfig must also be enabled.
+
### flask
> `= permissive | enforcing | late | disabled`
diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
index cddcf7aa51..e002200578 100644
--- a/xen/xsm/xsm_core.c
+++ b/xen/xsm/xsm_core.c
@@ -31,6 +31,30 @@
struct xsm_operations *xsm_ops;
+enum xsm_bootparam {
+XSM_BOOTPARAM_DUMMY,
+XSM_BOOTPARAM_FLASK,
+XSM_BOOTPARAM_INVALID,
+};
+
+enum xsm_bootparam __read_mostly xsm_bootparam = XSM_BOOTPARAM_DUMMY;
+
+static int __init parse_xsm_param(const char *s)
+{
+if ( !strcmp(s, "dummy") )
+xsm_bootparam = XSM_BOOTPARAM_DUMMY;
+#ifdef CONFIG_XSM_FLASK
+else if ( !strcmp(s, "flask") )
+xsm_bootparam = XSM_BOOTPARAM_FLASK;
+#endif
+else
+xsm_bootparam = XSM_BOOTPARAM_INVALID;
+
+return 0;
+}
+
+custom_param("xsm", parse_xsm_param);
+
static inline int verify(struct xsm_operations *ops)
{
/* verify the security_operations structure exists */
@@ -57,7 +81,20 @@ static int __init xsm_core_init(const void *policy_buffer,
size_t policy_size)
}
xsm_ops = _xsm_ops;
-flask_init(policy_buffer, policy_size);
+
+switch ( xsm_bootparam )
+{
+case XSM_BOOTPARAM_DUMMY:
+/* empty */
+break;
+
+case XSM_BOOTPARAM_FLASK:
+flask_init(policy_buffer, policy_size);
+break;
+
+default:
+printk("XSM: Invalid value for xsm= boot parameter.\n");
+}
return 0;
}
--
2.18.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
On 29/06/18 10:28, Xin Li wrote:
> Introduce new boot parameter xsm to choose which xsm module is enabled,
> and set default to dummy.
>
> Signed-off-by: Xin Li
As a note for other reviewers, this series is based on top of my XSM
Kconfig cleanup.
As for this patch, its almost there. Just a few minor issues.
>
> ---
> CC: Daniel De Graaf
> CC: George Dunlap
> CC: Jan Beulich
> CC: Konrad Rzeszutek Wilk
> CC: Stefano Stabellini
> CC: Tim Deegan
> CC: Wei Liu
> CC: Sergey Dyasli
> CC: Andrew Cooper
> CC: Ming Lu
> ---
> docs/misc/xen-command-line.markdown | 13 ++
> xen/xsm/xsm_core.c | 39 -
> 2 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/docs/misc/xen-command-line.markdown
> b/docs/misc/xen-command-line.markdown
> index 075e5ea159..7c689b8225 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -865,6 +865,19 @@ hardware domain is architecture dependent.
> Note that specifying zero as domU value means zero, while for dom0 it means
> to use the default.
>
> +### xsm
> +> `= dummy | silo | flask`
This should be just "dummy | flask" in this patch, and extended with
silo in the next path. Also, options in this file should be sorted
alphabetically, so ### xsm should be near the end, rather than beside flask.
> +
> +> Default: `dummy`
> +
> +Specify which XSM module should be enabled. This option is only available if
> +the hypervisor was compiled with XSM support.
> +
> +* `dummy`: this is the default choice. No special restriction will be
> applied.
> + it's also used when XSM is compiled out.
> +* `flask`: this is the policy based access control. To choose this, the
> + separated option in kconfig must also be enabled.
> +
> ### flask
> > `= permissive | enforcing | late | disabled`
>
> diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
> index cddcf7aa51..e002200578 100644
> --- a/xen/xsm/xsm_core.c
> +++ b/xen/xsm/xsm_core.c
> @@ -31,6 +31,30 @@
>
> struct xsm_operations *xsm_ops;
>
> +enum xsm_bootparam {
> +XSM_BOOTPARAM_DUMMY,
> +XSM_BOOTPARAM_FLASK,
> +XSM_BOOTPARAM_INVALID,
I'd drop INVALID (See below for the parsing aspect), as it actually
falls back to DUMMY.
> +};
> +
> +enum xsm_bootparam __read_mostly xsm_bootparam = XSM_BOOTPARAM_DUMMY;
This should be __initdata rather than __read_mostly. It is safe to be
discarded after boot.
> +
> +static int __init parse_xsm_param(const char *s)
> +{
int rc = 0;
> +if ( !strcmp(s, "dummy") )
> +xsm_bootparam = XSM_BOOTPARAM_DUMMY;
> +#ifdef CONFIG_XSM_FLASK
> +else if ( !strcmp(s, "flask") )
> +xsm_bootparam = XSM_BOOTPARAM_FLASK;
> +#endif
> +else
> +xsm_bootparam = XSM_BOOTPARAM_INVALID;
> +
> +return 0;
else
rc = -EINVAL;
return rc;
As a result, the core command line infrastructure will inform the user
if they passed an unrecognised option.
~Andrew
> +}
> +
> +custom_param("xsm", parse_xsm_param);
> +
> static inline int verify(struct xsm_operations *ops)
> {
> /* verify the security_operations structure exists */
> @@ -57,7 +81,20 @@ static int __init xsm_core_init(const void *policy_buffer,
> size_t policy_size)
> }
>
> xsm_ops = _xsm_ops;
> -flask_init(policy_buffer, policy_size);
> +
> +switch ( xsm_bootparam )
> +{
> +case XSM_BOOTPARAM_DUMMY:
> +/* empty */
> +break;
> +
> +case XSM_BOOTPARAM_FLASK:
> +flask_init(policy_buffer, policy_size);
> +break;
> +
> +default:
> +printk("XSM: Invalid value for xsm= boot parameter.\n");
> +}
>
> return 0;
> }
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
