All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
@ 2018-09-29  9:22 Xin Li
  2018-09-29  9:22 ` [PATCH 2/2] xen/xsm: Add new SILO mode for XSM Xin Li
                   ` (2 more replies)
  0 siblings, 3 replies; 27+ messages in thread
From: Xin Li @ 2018-09-29  9:22 UTC (permalink / raw)
  To: xen-devel
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, George Dunlap, Xin Li, Tim Deegan,
	Jan Beulich, Andrew Cooper, Ming Lu, Daniel De Graaf

Introduce new boot parameter xsm to choose which xsm module is enabled,
and set default to dummy.

Signed-off-by: Xin Li <xin.li@citrix.com>

---
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ming Lu <ming.lu@citrix.com>

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 = &dummy_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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
  2018-09-29  9:22 [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm Xin Li
@ 2018-09-29  9:22 ` Xin Li
  2018-10-01 15:21   ` [Non-DoD Source] " DeGraaf, Daniel G
  2018-10-02  9:33   ` Jan Beulich
  2018-10-01 15:17 ` [Non-DoD Source] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm DeGraaf, Daniel G
  2018-10-02  9:11 ` Jan Beulich
  2 siblings, 2 replies; 27+ messages in thread
From: Xin Li @ 2018-09-29  9:22 UTC (permalink / raw)
  To: xen-devel
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, George Dunlap, Xin Li, Tim Deegan,
	Jan Beulich, Andrew Cooper, Ming Lu, Daniel De Graaf

When SILO is enabled, there would be no page-sharing or event notifications
between unprivileged VMs (no grant tables or event channels).

Signed-off-by: Xin Li <xin.li@citrix.com>

---
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ming Lu <ming.lu@citrix.com>

v4:
1. include the dummy.h as a copy, and call the dummy module functions to avoid
indirect all.
2. call register_xsm() to setup silo module.

---
 docs/misc/xen-command-line.markdown |   5 +-
 xen/common/Kconfig                  |  15 ++++
 xen/include/xsm/dummy.h             |   5 ++
 xen/include/xsm/xsm.h               |   6 ++
 xen/xsm/Makefile                    |   1 +
 xen/xsm/dummy.c                     |   1 -
 xen/xsm/silo.c                      | 109 ++++++++++++++++++++++++++++
 xen/xsm/xsm_core.c                  |  11 +++
 8 files changed, 151 insertions(+), 2 deletions(-)
 create mode 100644 xen/xsm/silo.c

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index cf9924f53f..1b49fda8fc 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -900,7 +900,7 @@ Note that specifying zero as domU value means zero, while for dom0 it means
 to use the default.
 
 ### xsm
-> `= dummy | flask`
+> `= dummy | flask | silo`
 
 > Default: `dummy`
 
@@ -911,6 +911,9 @@ the hypervisor was compiled with XSM support.
   (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.
+* `silo`: this will deny any unmediated communication channels between
+  unprivileged VMs.  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 f802efb625..ce965fbf17 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -154,15 +154,30 @@ config XSM_FLASK_POLICY
 
 	  If unsure, say Y.
 
+config XSM_SILO
+	def_bool y
+	prompt "SILO support"
+	depends on XSM
+	---help---
+	  Enables SILO as the access control mechanism used by the XSM framework.
+	  This is not the default module, add boot parameter xsm=silo to choose
+	  it. This will deny any unmediated communication channels (grant tables
+	  and event channels) between unprivileged VMs.
+
+	  If unsure, say Y.
+
 choice
 	prompt "Default XSM implementation"
 	depends on XSM
 	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
 
 config LATE_HWDOM
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index b0ac1f66b3..b2bb16c55e 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -48,7 +48,12 @@ void __xsm_action_mismatch_detected(void);
  * There is no xsm_default_t argument available, so the value from the assertion
  * is used to initialize the variable.
  */
+#ifdef CONFIG_XSM_SILO
+#define XSM_INLINE __attribute__ ((unused))
+#else
 #define XSM_INLINE /* */
+#endif
+
 #define XSM_DEFAULT_ARG /* */
 #define XSM_DEFAULT_VOID void
 #define XSM_ASSERT_ACTION(def) xsm_default_t action = def; (void)action
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 3d67962493..3b192b5c31 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -733,6 +733,12 @@ extern const unsigned char xsm_flask_init_policy[];
 extern const unsigned int xsm_flask_init_policy_size;
 #endif
 
+#ifdef CONFIG_XSM_SILO
+extern void silo_init(void);
+#else
+static inline void silo_init(void) {}
+#endif
+
 #else /* CONFIG_XSM */
 
 #include <xsm/dummy.h>
diff --git a/xen/xsm/Makefile b/xen/xsm/Makefile
index 8bb4a24f09..e4d581e065 100644
--- a/xen/xsm/Makefile
+++ b/xen/xsm/Makefile
@@ -1,5 +1,6 @@
 obj-y += xsm_core.o
 obj-$(CONFIG_XSM) += xsm_policy.o
 obj-$(CONFIG_XSM) += dummy.o
+obj-$(CONFIG_XSM_SILO) += silo.o
 
 subdir-$(CONFIG_XSM_FLASK) += flask
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 3290d04527..06a674fad0 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -11,7 +11,6 @@
  */
 
 #define XSM_NO_WRAPPERS
-#define XSM_INLINE /* */
 #include <xsm/dummy.h>
 
 struct xsm_operations dummy_xsm_ops;
diff --git a/xen/xsm/silo.c b/xen/xsm/silo.c
new file mode 100644
index 0000000000..d6ef6abd32
--- /dev/null
+++ b/xen/xsm/silo.c
@@ -0,0 +1,109 @@
+/******************************************************************************
+ * xsm/silo.c
+ *
+ * SILO module for XSM(Xen Security Modules)
+ *
+ * Copyright (c) 2018 Citrix Systems Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+#define XSM_NO_WRAPPERS
+
+#include <xsm/dummy.h>
+
+/*
+ * Check if inter-domain communication is allowed.
+ * Return true when pass check.
+ */
+static bool silo_mode_dom_check(const struct domain *ldom,
+                                const struct domain *rdom)
+{
+    const struct domain *cur_dom = current->domain;
+
+    return (is_control_domain(cur_dom) || is_control_domain(ldom) ||
+            is_control_domain(rdom) || ldom == rdom);
+}
+
+static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn,
+                               domid_t id2)
+{
+    int rc = -EPERM;
+    struct domain *d2 = rcu_lock_domain_by_any_id(id2);
+
+    if ( d2 == NULL )
+        rc = -ESRCH;
+    else
+    {
+        if ( silo_mode_dom_check(d1, d2) )
+            rc = xsm_evtchn_unbound(d1, chn, id2);
+        rcu_unlock_domain(d2);
+    }
+
+    return rc;
+}
+
+static int silo_evtchn_interdomain(struct domain *d1, struct evtchn *chan1,
+                                   struct domain *d2, struct evtchn *chan2)
+{
+    if ( silo_mode_dom_check(d1, d2) )
+        return xsm_evtchn_interdomain(d1, chan1, d2, chan2);
+    return -EPERM;
+}
+
+static int silo_grant_mapref(struct domain *d1, struct domain *d2,
+                             uint32_t flags)
+{
+    if ( silo_mode_dom_check(d1, d2) )
+        return xsm_grant_mapref(d1, d2, flags);
+    return -EPERM;
+}
+
+static int silo_grant_transfer(struct domain *d1, struct domain *d2)
+{
+    if ( silo_mode_dom_check(d1, d2) )
+        return xsm_grant_transfer(d1, d2);
+    return -EPERM;
+}
+
+static int silo_grant_copy(struct domain *d1, struct domain *d2)
+{
+    if ( silo_mode_dom_check(d1, d2) )
+        return xsm_grant_copy(d1, d2);
+    return -EPERM;
+}
+
+static struct xsm_operations silo_xsm_ops = {
+    .evtchn_unbound = silo_evtchn_unbound,
+    .evtchn_interdomain = silo_evtchn_interdomain,
+    .grant_mapref = silo_grant_mapref,
+    .grant_transfer = silo_grant_transfer,
+    .grant_copy = silo_grant_copy,
+};
+
+void __init silo_init(void)
+{
+    printk("Initialising XSM SILO mode\n");
+
+    if ( register_xsm(&silo_xsm_ops) )
+        panic("SILO: Unable to register with XSM\n");
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
index df284ec463..262af5ec9b 100644
--- a/xen/xsm/xsm_core.c
+++ b/xen/xsm/xsm_core.c
@@ -34,11 +34,14 @@ struct xsm_operations *xsm_ops;
 enum xsm_bootparam {
     XSM_BOOTPARAM_DUMMY,
     XSM_BOOTPARAM_FLASK,
+    XSM_BOOTPARAM_SILO,
 };
 
 static enum xsm_bootparam __initdata xsm_bootparam =
 #ifdef CONFIG_XSM_FLASK_DEFAULT
     XSM_BOOTPARAM_FLASK;
+#elif CONFIG_XSM_SILO_DEFAULT
+    XSM_BOOTPARAM_SILO;
 #else
     XSM_BOOTPARAM_DUMMY;
 #endif
@@ -52,6 +55,10 @@ static int __init parse_xsm_param(const char *s)
 #ifdef CONFIG_XSM_FLASK
     else if ( !strcmp(s, "flask") )
         xsm_bootparam = XSM_BOOTPARAM_FLASK;
+#endif
+#ifdef CONFIG_XSM_SILO
+    else if ( !strcmp(s, "silo") )
+        xsm_bootparam = XSM_BOOTPARAM_SILO;
 #endif
     else {
         printk("XSM: can't parse boot parameter xsm=%s\n", s);
@@ -98,6 +105,10 @@ static int __init xsm_core_init(const void *policy_buffer, size_t policy_size)
         flask_init(policy_buffer, policy_size);
         break;
 
+    case XSM_BOOTPARAM_SILO:
+        silo_init();
+        break;
+
     default:
         printk("XSM: Invalid value for xsm= boot parameter\n");
         break;
-- 
2.18.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [Non-DoD Source] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
  2018-09-29  9:22 [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm Xin Li
  2018-09-29  9:22 ` [PATCH 2/2] xen/xsm: Add new SILO mode for XSM Xin Li
@ 2018-10-01 15:17 ` DeGraaf, Daniel G
  2018-10-08  6:32   ` Xin Li (Talons)
  2018-10-02  9:11 ` Jan Beulich
  2 siblings, 1 reply; 27+ messages in thread
From: DeGraaf, Daniel G @ 2018-10-01 15:17 UTC (permalink / raw)
  To: 'Xin Li', xen-devel
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, George Dunlap, Xin Li, Tim Deegan,
	Jan Beulich, Andrew Cooper, Ming Lu, Daniel De Graaf

> 
> Introduce new boot parameter xsm to choose which xsm module is enabled,
> and set default to dummy.
> 
> Signed-off-by: Xin Li <xin.li@citrix.com>

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

It might be useful for the commit message to also reference the new Kconfig option; thanks for adding it.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Non-DoD Source] [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
  2018-09-29  9:22 ` [PATCH 2/2] xen/xsm: Add new SILO mode for XSM Xin Li
@ 2018-10-01 15:21   ` DeGraaf, Daniel G
  2018-10-02  9:33   ` Jan Beulich
  1 sibling, 0 replies; 27+ messages in thread
From: DeGraaf, Daniel G @ 2018-10-01 15:21 UTC (permalink / raw)
  To: 'Xin Li', xen-devel
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, George Dunlap, Xin Li, Tim Deegan,
	Jan Beulich, Andrew Cooper, Ming Lu, Daniel De Graaf

> 
> When SILO is enabled, there would be no page-sharing or event notifications
> between unprivileged VMs (no grant tables or event channels).
> 
> Signed-off-by: Xin Li <xin.li@citrix.com>
> 

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
  2018-09-29  9:22 [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm Xin Li
  2018-09-29  9:22 ` [PATCH 2/2] xen/xsm: Add new SILO mode for XSM Xin Li
  2018-10-01 15:17 ` [Non-DoD Source] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm DeGraaf, Daniel G
@ 2018-10-02  9:11 ` Jan Beulich
  2018-10-08  6:30   ` Xin Li (Talons)
  2 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2018-10-02  9:11 UTC (permalink / raw)
  To: Xin Li
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Tim Deegan,
	xen-devel, Xin Li, Ming Lu, Daniel de Graaf

>>> On 29.09.18 at 11:22, <talons.lee@gmail.com> 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 = &dummy_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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
  2018-09-29  9:22 ` [PATCH 2/2] xen/xsm: Add new SILO mode for XSM Xin Li
  2018-10-01 15:21   ` [Non-DoD Source] " DeGraaf, Daniel G
@ 2018-10-02  9:33   ` Jan Beulich
  2018-10-08  7:49     ` Xin Li (Talons)
  1 sibling, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2018-10-02  9:33 UTC (permalink / raw)
  To: Xin Li
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Tim Deegan,
	xen-devel, Xin Li, Ming Lu, Daniel de Graaf

>>> On 29.09.18 at 11:22, <talons.lee@gmail.com> wrote:
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -48,7 +48,12 @@ void __xsm_action_mismatch_detected(void);
>   * There is no xsm_default_t argument available, so the value from the assertion
>   * is used to initialize the variable.
>   */
> +#ifdef CONFIG_XSM_SILO
> +#define XSM_INLINE __attribute__ ((unused))

Please don't open-code __maybe_unused. Furthermore I question
the dependency on CONFIG_XSM_SILO here: Afaict you want this for
just the single new inclusion site, without affecting any others.

> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -733,6 +733,12 @@ extern const unsigned char xsm_flask_init_policy[];
>  extern const unsigned int xsm_flask_init_policy_size;
>  #endif
>  
> +#ifdef CONFIG_XSM_SILO
> +extern void silo_init(void);
> +#else
> +static inline void silo_init(void) {}
> +#endif

Along the lines of one of my remarks on patch 1, I think you would
better get away without the inline variant, by adding suitable #ifdef-s
to eliminate the risk of wrong use of the new enumerator.

> --- a/xen/xsm/dummy.c
> +++ b/xen/xsm/dummy.c
> @@ -11,7 +11,6 @@
>   */
>  
>  #define XSM_NO_WRAPPERS
> -#define XSM_INLINE /* */
>  #include <xsm/dummy.h>

The change looks unmotivated here, perhaps because of the
questionable CONFIG_XSM_SILO dependency further up. That
said, it looks as if the #define could go away currently, as being
redundant with what dummy.h already has. That would then
better be a small separate prereq patch imo.

> --- /dev/null
> +++ b/xen/xsm/silo.c
> @@ -0,0 +1,109 @@
> +/******************************************************************************
> + * xsm/silo.c
> + *
> + * SILO module for XSM(Xen Security Modules)

Would you mind adding the missing blank here?

> + * Copyright (c) 2018 Citrix Systems Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +#define XSM_NO_WRAPPERS
> +
> +#include <xsm/dummy.h>

Please switch around the blank and the #define lines, matching how
dummy.c is written. This helps readers understand that the #define
is specifically in preparation of the #include.

> +/*
> + * Check if inter-domain communication is allowed.
> + * Return true when pass check.
> + */
> +static bool silo_mode_dom_check(const struct domain *ldom,
> +                                const struct domain *rdom)
> +{
> +    const struct domain *cur_dom = current->domain;

We commonly call such variables currd.

> +    return (is_control_domain(cur_dom) || is_control_domain(ldom) ||
> +            is_control_domain(rdom) || ldom == rdom);
> +}
> +
> +static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn,
> +                               domid_t id2)
> +{
> +    int rc = -EPERM;
> +    struct domain *d2 = rcu_lock_domain_by_any_id(id2);
> +
> +    if ( d2 == NULL )

We generally try to use the shorter !d2 in new code. But it's a
matter of personal preference, I agree.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
  2018-10-02  9:11 ` Jan Beulich
@ 2018-10-08  6:30   ` Xin Li (Talons)
  0 siblings, 0 replies; 27+ messages in thread
From: Xin Li (Talons) @ 2018-10-08  6:30 UTC (permalink / raw)
  To: Jan Beulich, Xin Li
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
	George Dunlap, xen-devel, Ming Lu, Daniel de Graaf

Thanks Jan.

> >>> On 29.09.18 at 11:22, <talons.lee@gmail.com> 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 = &dummy_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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Non-DoD Source] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
  2018-10-01 15:17 ` [Non-DoD Source] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm DeGraaf, Daniel G
@ 2018-10-08  6:32   ` Xin Li (Talons)
  0 siblings, 0 replies; 27+ messages in thread
From: Xin Li (Talons) @ 2018-10-08  6:32 UTC (permalink / raw)
  To: dgdegra, 'Xin Li', xen-devel
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
	George Dunlap, Jan Beulich, Ming Lu

Thanks Daniel.


> -----Original Message-----
> From: DeGraaf, Daniel G [mailto:dgdegra@nsa.gov]
> Sent: Monday, October 1, 2018 11:18 PM
> To: 'Xin Li' <talons.lee@gmail.com>; xen-devel@lists.xen.org
> Cc: Xin Li (Talons) <xin.li@citrix.com>; Daniel De Graaf
> <dgdegra@tycho.nsa.gov>; George Dunlap <George.Dunlap@citrix.com>; Jan
> Beulich <JBeulich@suse.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Tim
> (Xen.org) <tim@xen.org>; Wei Liu <wei.liu2@citrix.com>; Sergey Dyasli
> <sergey.dyasli@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>;
> Ming Lu <ming.lu@citrix.com>
> Subject: RE: [Non-DoD Source] [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 <xin.li@citrix.com>
> 
> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> 
> It might be useful for the commit message to also reference the new Kconfig
> option; thanks for adding it.

OK. Sure.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
  2018-10-02  9:33   ` Jan Beulich
@ 2018-10-08  7:49     ` Xin Li (Talons)
  2018-10-08  8:28       ` Jan Beulich
  0 siblings, 1 reply; 27+ messages in thread
From: Xin Li (Talons) @ 2018-10-08  7:49 UTC (permalink / raw)
  To: Jan Beulich, Xin Li
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
	George Dunlap, xen-devel, Ming Lu, Daniel de Graaf

Thanks Jan.

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, October 2, 2018 5:33 PM
> To: Xin Li <talons.lee@gmail.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Ming Lu
> <ming.lu@citrix.com>; Sergey Dyasli <sergey.dyasli@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Xin Li (Talons) <xin.li@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel@lists.xen.org; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Daniel
> de Graaf <dgdegra@tycho.nsa.gov>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
> 
> >>> On 29.09.18 at 11:22, <talons.lee@gmail.com> wrote:
> > --- a/xen/include/xsm/dummy.h
> > +++ b/xen/include/xsm/dummy.h
> > @@ -48,7 +48,12 @@ void __xsm_action_mismatch_detected(void);
> >   * There is no xsm_default_t argument available, so the value from the
> assertion
> >   * is used to initialize the variable.
> >   */
> > +#ifdef CONFIG_XSM_SILO
> > +#define XSM_INLINE __attribute__ ((unused))
> 
> Please don't open-code __maybe_unused. Furthermore I question the
> dependency on CONFIG_XSM_SILO here: Afaict you want this for just the single
> new inclusion site, without affecting any others.

OK, use __maybe_unused directly and remove this #ifdef.

> 
> > --- a/xen/include/xsm/xsm.h
> > +++ b/xen/include/xsm/xsm.h
> > @@ -733,6 +733,12 @@ extern const unsigned char
> > xsm_flask_init_policy[];  extern const unsigned int
> > xsm_flask_init_policy_size;  #endif
> >
> > +#ifdef CONFIG_XSM_SILO
> > +extern void silo_init(void);
> > +#else
> > +static inline void silo_init(void) {} #endif
> 
> Along the lines of one of my remarks on patch 1, I think you would better get
> away without the inline variant, by adding suitable #ifdef-s to eliminate the risk
> of wrong use of the new enumerator.

I can remove the inline function, and add #idef in xsm_core.c
But, this way does not match the current code style(like flask_init()).
So I would like to keep this way.

> 
> > --- a/xen/xsm/dummy.c
> > +++ b/xen/xsm/dummy.c
> > @@ -11,7 +11,6 @@
> >   */
> >
> >  #define XSM_NO_WRAPPERS
> > -#define XSM_INLINE /* */
> >  #include <xsm/dummy.h>
> 
> The change looks unmotivated here, perhaps because of the questionable
> CONFIG_XSM_SILO dependency further up. That said, it looks as if the #define
> could go away currently, as being redundant with what dummy.h already has.
> That would then better be a small separate prereq patch imo.

This is intended, to avoid compile error, can't refine XSM_INLINE.
Do you mean a separate patch to just remove this line?

> 
> > --- /dev/null
> > +++ b/xen/xsm/silo.c
> > @@ -0,0 +1,109 @@
> >
> +/***************************************************************
> *****
> > +**********
> > + * xsm/silo.c
> > + *
> > + * SILO module for XSM(Xen Security Modules)
> 
> Would you mind adding the missing blank here?

Do you mean add one space after XSM?
* SILO module for XSM (Xen Security Modules)

> 
> > + * Copyright (c) 2018 Citrix Systems Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but
> > +WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY
> > +or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> > +License for
> > + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > +along with
> > + * this program; If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +#define XSM_NO_WRAPPERS
> > +
> > +#include <xsm/dummy.h>
> 
> Please switch around the blank and the #define lines, matching how dummy.c is
> written. This helps readers understand that the #define is specifically in
> preparation of the #include.

OK. Remove the line before #include.

 
> > +/*
> > + * Check if inter-domain communication is allowed.
> > + * Return true when pass check.
> > + */
> > +static bool silo_mode_dom_check(const struct domain *ldom,
> > +                                const struct domain *rdom) {
> > +    const struct domain *cur_dom = current->domain;
> 
> We commonly call such variables currd.

OK. Rename this var.

> 
> > +    return (is_control_domain(cur_dom) || is_control_domain(ldom) ||
> > +            is_control_domain(rdom) || ldom == rdom); }
> > +
> > +static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn,
> > +                               domid_t id2) {
> > +    int rc = -EPERM;
> > +    struct domain *d2 = rcu_lock_domain_by_any_id(id2);
> > +
> > +    if ( d2 == NULL )
> 
> We generally try to use the shorter !d2 in new code. But it's a matter of
> personal preference, I agree.

Thanks. I will just keep it.

> 
> Jan
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
  2018-10-08  7:49     ` Xin Li (Talons)
@ 2018-10-08  8:28       ` Jan Beulich
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2018-10-08  8:28 UTC (permalink / raw)
  To: Xin Li, Xin Li
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, Andrew Cooper, Tim Deegan, george.dunlap,
	xen-devel, Ming Lu, Daniel de Graaf

>>> On 08.10.18 at 09:49, <xin.li@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Tuesday, October 2, 2018 5:33 PM
>> 
>> >>> On 29.09.18 at 11:22, <talons.lee@gmail.com> wrote:
>> > --- a/xen/xsm/dummy.c
>> > +++ b/xen/xsm/dummy.c
>> > @@ -11,7 +11,6 @@
>> >   */
>> >
>> >  #define XSM_NO_WRAPPERS
>> > -#define XSM_INLINE /* */
>> >  #include <xsm/dummy.h>
>> 
>> The change looks unmotivated here, perhaps because of the questionable
>> CONFIG_XSM_SILO dependency further up. That said, it looks as if the #define
>> could go away currently, as being redundant with what dummy.h already has.
>> That would then better be a small separate prereq patch imo.
> 
> This is intended, to avoid compile error, can't refine XSM_INLINE.
> Do you mean a separate patch to just remove this line?

Yes, and then with an explanation of why this line is not needed.

>> > --- /dev/null
>> > +++ b/xen/xsm/silo.c
>> > @@ -0,0 +1,109 @@
>> >
>> +/***************************************************************
>> *****
>> > +**********
>> > + * xsm/silo.c
>> > + *
>> > + * SILO module for XSM(Xen Security Modules)
>> 
>> Would you mind adding the missing blank here?
> 
> Do you mean add one space after XSM?
> * SILO module for XSM (Xen Security Modules)

Yes.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
  2018-09-28  8:18 Xin Li
@ 2018-09-28 17:15 ` Daniel De Graaf
  0 siblings, 0 replies; 27+ messages in thread
From: Daniel De Graaf @ 2018-09-28 17:15 UTC (permalink / raw)
  To: Xin Li, xen-devel
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, George Dunlap, Xin Li, Tim Deegan,
	Jan Beulich, Andrew Cooper, Ming Lu

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 <xin.li@citrix.com>

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
@ 2018-09-28  8:18 Xin Li
  2018-09-28 17:15 ` Daniel De Graaf
  0 siblings, 1 reply; 27+ messages in thread
From: Xin Li @ 2018-09-28  8:18 UTC (permalink / raw)
  To: xen-devel
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, George Dunlap, Xin Li, Tim Deegan,
	Jan Beulich, Andrew Cooper, Ming Lu, Daniel De Graaf

Introduce new boot parameter xsm to choose which xsm module is enabled,
and set default to dummy.

Signed-off-by: Xin Li <xin.li@citrix.com>

---
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ming Lu <ming.lu@citrix.com>

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 = &dummy_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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
  2018-07-03  1:26 Xin Li
  2018-07-03  6:10 ` Xin Li (Talons)
  2018-07-03  7:12 ` Jan Beulich
@ 2018-08-17 19:11 ` Daniel De Graaf
  2 siblings, 0 replies; 27+ messages in thread
From: Daniel De Graaf @ 2018-08-17 19:11 UTC (permalink / raw)
  To: Xin Li, xen-devel
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu, George Dunlap,
	Xin Li, Tim Deegan, Jan Beulich, Andrew Cooper, Ming Lu

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 <xin.li@citrix.com>

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
  2018-07-04 16:54   ` George Dunlap
@ 2018-07-05  1:38     ` Xin Li (Talons)
  0 siblings, 0 replies; 27+ messages in thread
From: Xin Li (Talons) @ 2018-07-05  1:38 UTC (permalink / raw)
  To: George Dunlap, Jan Beulich
  Cc: Xin Li, Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Andrew Cooper, Tim (Xen.org),
	xen-devel, Ming Lu, Daniel de Graaf

> -----Original Message-----
> From: George Dunlap
> Sent: Thursday, July 5, 2018 12:55 AM
> To: Jan Beulich <JBeulich@suse.com>
> Cc: Xin Li <talons.lee@gmail.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Ming Lu <ming.lu@citrix.com>; Sergey Dyasli
> <sergey.dyasli@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Xin Li (Talons)
> <xin.li@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Stefano
> Stabellini <sstabellini@kernel.org>; xen-devel <xen-devel@lists.xen.org>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Daniel de Graaf
> <dgdegra@tycho.nsa.gov>; Tim (Xen.org) <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 <JBeulich@suse.com> 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, <talons.lee@gmail.com> 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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
  2018-07-03  7:12 ` Jan Beulich
  2018-07-03  8:58   ` Xin Li (Talons)
@ 2018-07-04 16:54   ` George Dunlap
  2018-07-05  1:38     ` Xin Li (Talons)
  1 sibling, 1 reply; 27+ messages in thread
From: George Dunlap @ 2018-07-04 16:54 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Xin Li, Sergey Dyasli, Stefano Stabellini, Wei Liu,
	Andrew Cooper, Tim (Xen.org),
	George Dunlap, xen-devel, Xin Li (Talons),
	Ming Lu, Daniel de Graaf



> On Jul 3, 2018, at 8:12 AM, Jan Beulich <JBeulich@suse.com> 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, <talons.lee@gmail.com> 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.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
  2018-07-03  7:12 ` Jan Beulich
@ 2018-07-03  8:58   ` Xin Li (Talons)
  2018-07-04 16:54   ` George Dunlap
  1 sibling, 0 replies; 27+ messages in thread
From: Xin Li (Talons) @ 2018-07-03  8:58 UTC (permalink / raw)
  To: Jan Beulich, Xin Li
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu, Andrew Cooper,
	Tim (Xen.org),
	George Dunlap, xen-devel, Ming Lu, Daniel de Graaf

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, July 3, 2018 3:12 PM
> To: Xin Li <talons.lee@gmail.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Ming Lu
> <ming.lu@citrix.com>; Sergey Dyasli <sergey.dyasli@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Xin Li (Talons) <xin.li@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel@lists.xen.org; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Daniel
> de Graaf <dgdegra@tycho.nsa.gov>; Tim (Xen.org) <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, <talons.lee@gmail.com> 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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
  2018-07-03  1:26 Xin Li
  2018-07-03  6:10 ` Xin Li (Talons)
@ 2018-07-03  7:12 ` Jan Beulich
  2018-07-03  8:58   ` Xin Li (Talons)
  2018-07-04 16:54   ` George Dunlap
  2018-08-17 19:11 ` Daniel De Graaf
  2 siblings, 2 replies; 27+ messages in thread
From: Jan Beulich @ 2018-07-03  7:12 UTC (permalink / raw)
  To: Xin Li
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu, George Dunlap,
	Andrew Cooper, Tim Deegan, xen-devel, Xin Li, Ming Lu,
	Daniel de Graaf

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, <talons.lee@gmail.com> 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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
  2018-07-03  1:26 Xin Li
@ 2018-07-03  6:10 ` Xin Li (Talons)
  2018-07-03  7:12 ` Jan Beulich
  2018-08-17 19:11 ` Daniel De Graaf
  2 siblings, 0 replies; 27+ messages in thread
From: Xin Li (Talons) @ 2018-07-03  6:10 UTC (permalink / raw)
  To: Xin Li, xen-devel
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu, Andrew Cooper,
	Tim (Xen.org),
	George Dunlap, Jan Beulich, Ming Lu, Daniel De Graaf

> -----Original Message-----
> From: Xin Li [mailto:talons.lee@gmail.com]
> Sent: Tuesday, July 3, 2018 9:26 AM
> To: xen-devel@lists.xen.org
> Cc: Xin Li (Talons) <xin.li@citrix.com>; Daniel De Graaf
> <dgdegra@tycho.nsa.gov>; George Dunlap <George.Dunlap@citrix.com>; Jan
> Beulich <JBeulich@suse.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Tim
> (Xen.org) <tim@xen.org>; Wei Liu <wei.liu2@citrix.com>; Sergey Dyasli
> <sergey.dyasli@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>;
> Ming Lu <ming.lu@citrix.com>
> 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 <xin.li@citrix.com>
> 
> ---
> CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Tim Deegan <tim@xen.org>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ming Lu <ming.lu@citrix.com>
> 
> 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 = &dummy_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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
@ 2018-07-03  1:26 Xin Li
  2018-07-03  6:10 ` Xin Li (Talons)
                   ` (2 more replies)
  0 siblings, 3 replies; 27+ messages in thread
From: Xin Li @ 2018-07-03  1:26 UTC (permalink / raw)
  To: xen-devel
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu, George Dunlap,
	Xin Li, Tim Deegan, Jan Beulich, Andrew Cooper, Ming Lu,
	Daniel De Graaf

Introduce new boot parameter xsm to choose which xsm module is enabled,
and set default to dummy.

Signed-off-by: Xin Li <xin.li@citrix.com>

---
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ming Lu <ming.lu@citrix.com>

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 = &dummy_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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
  2018-07-02  8:39         ` Xin Li (Talons)
@ 2018-07-02  9:04           ` Jan Beulich
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2018-07-02  9:04 UTC (permalink / raw)
  To: Xin Li, Xin Li
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu, Andrew Cooper,
	Tim Deegan, george.dunlap, xen-devel, Ming Lu, Daniel de Graaf

>>> On 02.07.18 at 10:39, <xin.li@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Monday, July 2, 2018 4:25 PM
>> >>> On 02.07.18 at 09:34, <xin.li@citrix.com> wrote:
>> >> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> Sent: Friday, June 29, 2018 6:05 PM
>> >> >>> On 29.06.18 at 11:47, <andrew.cooper3@citrix.com> 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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
  2018-07-02  8:24       ` Jan Beulich
@ 2018-07-02  8:39         ` Xin Li (Talons)
  2018-07-02  9:04           ` Jan Beulich
  0 siblings, 1 reply; 27+ messages in thread
From: Xin Li (Talons) @ 2018-07-02  8:39 UTC (permalink / raw)
  To: Jan Beulich, Xin Li
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu, Andrew Cooper,
	Tim (Xen.org),
	George Dunlap, xen-devel, Ming Lu, Daniel de Graaf

Hello Jan


> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, July 2, 2018 4:25 PM
> To: Xin Li (Talons) <xin.li@citrix.com>; Xin Li <talons.lee@gmail.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ming Lu <ming.lu@citrix.com>; Sergey Dyasli
> <sergey.dyasli@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>; xen-devel@lists.xen.org; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Daniel de Graaf <dgdegra@tycho.nsa.gov>; Tim
> (Xen.org) <tim@xen.org>
> Subject: RE: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
> 
> >>> On 02.07.18 at 09:34, <xin.li@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Friday, June 29, 2018 6:05 PM
> >> >>> On 29.06.18 at 11:47, <andrew.cooper3@citrix.com> 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 = &dummy_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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
  2018-07-02  7:34     ` Xin Li (Talons)
@ 2018-07-02  8:24       ` Jan Beulich
  2018-07-02  8:39         ` Xin Li (Talons)
  0 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2018-07-02  8:24 UTC (permalink / raw)
  To: Xin Li, Xin Li
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu, Andrew Cooper,
	Tim Deegan, george.dunlap, xen-devel, Ming Lu, Daniel de Graaf

>>> On 02.07.18 at 09:34, <xin.li@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Friday, June 29, 2018 6:05 PM
>> >>> On 29.06.18 at 11:47, <andrew.cooper3@citrix.com> 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 = &dummy_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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
  2018-06-29 10:04   ` Jan Beulich
@ 2018-07-02  7:34     ` Xin Li (Talons)
  2018-07-02  8:24       ` Jan Beulich
  0 siblings, 1 reply; 27+ messages in thread
From: Xin Li (Talons) @ 2018-07-02  7:34 UTC (permalink / raw)
  To: Jan Beulich, Andrew Cooper, Xin Li
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu, Tim (Xen.org),
	George Dunlap, xen-devel, Ming Lu, Daniel de Graaf

Hello Jan,

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, June 29, 2018 6:05 PM
> To: Andrew Cooper <Andrew.Cooper3@citrix.com>; Xin Li
> <talons.lee@gmail.com>
> Cc: Ming Lu <ming.lu@citrix.com>; Sergey Dyasli <sergey.dyasli@citrix.com>;
> Wei Liu <wei.liu2@citrix.com>; Xin Li (Talons) <xin.li@citrix.com>; George
> Dunlap <George.Dunlap@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>; xen-devel@lists.xen.org; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Daniel de Graaf <dgdegra@tycho.nsa.gov>; Tim
> (Xen.org) <tim@xen.org>
> Subject: Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
> 
> >>> On 29.06.18 at 11:47, <andrew.cooper3@citrix.com> 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 = &dummy_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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
  2018-06-29  9:47 ` Andrew Cooper
  2018-06-29 10:04   ` Jan Beulich
@ 2018-07-02  7:21   ` Xin Li (Talons)
  1 sibling, 0 replies; 27+ messages in thread
From: Xin Li (Talons) @ 2018-07-02  7:21 UTC (permalink / raw)
  To: Andrew Cooper, Xin Li, xen-devel
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu, Tim (Xen.org),
	George Dunlap, Jan Beulich, Ming Lu, Daniel De Graaf

Hell Andrew,

> -----Original Message-----
> From: Andrew Cooper
> Sent: Friday, June 29, 2018 5:48 PM
> To: Xin Li <talons.lee@gmail.com>; xen-devel@lists.xen.org
> Cc: Xin Li (Talons) <xin.li@citrix.com>; Daniel De Graaf
> <dgdegra@tycho.nsa.gov>; George Dunlap <George.Dunlap@citrix.com>; Jan
> Beulich <JBeulich@suse.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Tim
> (Xen.org) <tim@xen.org>; Wei Liu <wei.liu2@citrix.com>; Sergey Dyasli
> <sergey.dyasli@citrix.com>; Ming Lu <ming.lu@citrix.com>
> 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 <xin.li@citrix.com>
> 
> 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 <dgdegra@tycho.nsa.gov>
> > CC: George Dunlap <George.Dunlap@eu.citrix.com>
> > CC: Jan Beulich <JBeulich@suse.com>
> > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > CC: Stefano Stabellini <sstabellini@kernel.org>
> > CC: Tim Deegan <tim@xen.org>
> > CC: Wei Liu <wei.liu2@citrix.com>
> > CC: Sergey Dyasli <sergey.dyasli@citrix.com>
> > CC: Andrew Cooper <andrew.cooper3@citrix.com>
> > CC: Ming Lu <ming.lu@citrix.com>
> > ---
> >  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 = &dummy_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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
  2018-06-29  9:47 ` Andrew Cooper
@ 2018-06-29 10:04   ` Jan Beulich
  2018-07-02  7:34     ` Xin Li (Talons)
  2018-07-02  7:21   ` Xin Li (Talons)
  1 sibling, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2018-06-29 10:04 UTC (permalink / raw)
  To: Andrew Cooper, Xin Li
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu, George Dunlap,
	Xin Li, Tim Deegan, xen-devel, Ming Lu, Daniel de Graaf

>>> On 29.06.18 at 11:47, <andrew.cooper3@citrix.com> 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 = &dummy_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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
  2018-06-29  9:28 Xin Li
@ 2018-06-29  9:47 ` Andrew Cooper
  2018-06-29 10:04   ` Jan Beulich
  2018-07-02  7:21   ` Xin Li (Talons)
  0 siblings, 2 replies; 27+ messages in thread
From: Andrew Cooper @ 2018-06-29  9:47 UTC (permalink / raw)
  To: Xin Li, xen-devel
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu, George Dunlap,
	Xin Li, Tim Deegan, Jan Beulich, Ming Lu, Daniel De Graaf

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 <xin.li@citrix.com>

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 <dgdegra@tycho.nsa.gov>
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Tim Deegan <tim@xen.org>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ming Lu <ming.lu@citrix.com>
> ---
>  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 = &dummy_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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm
@ 2018-06-29  9:28 Xin Li
  2018-06-29  9:47 ` Andrew Cooper
  0 siblings, 1 reply; 27+ messages in thread
From: Xin Li @ 2018-06-29  9:28 UTC (permalink / raw)
  To: xen-devel
  Cc: Sergey Dyasli, Stefano Stabellini, Wei Liu, George Dunlap,
	Xin Li, Tim Deegan, Jan Beulich, Andrew Cooper, Ming Lu,
	Daniel De Graaf

Introduce new boot parameter xsm to choose which xsm module is enabled,
and set default to dummy.

Signed-off-by: Xin Li <xin.li@citrix.com>

---
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ming Lu <ming.lu@citrix.com>
---
 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 = &dummy_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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2018-10-08  8:28 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-29  9:22 [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm Xin Li
2018-09-29  9:22 ` [PATCH 2/2] xen/xsm: Add new SILO mode for XSM Xin Li
2018-10-01 15:21   ` [Non-DoD Source] " DeGraaf, Daniel G
2018-10-02  9:33   ` Jan Beulich
2018-10-08  7:49     ` Xin Li (Talons)
2018-10-08  8:28       ` Jan Beulich
2018-10-01 15:17 ` [Non-DoD Source] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm DeGraaf, Daniel G
2018-10-08  6:32   ` Xin Li (Talons)
2018-10-02  9:11 ` Jan Beulich
2018-10-08  6:30   ` Xin Li (Talons)
  -- strict thread matches above, loose matches on Subject: below --
2018-09-28  8:18 Xin Li
2018-09-28 17:15 ` Daniel De Graaf
2018-07-03  1:26 Xin Li
2018-07-03  6:10 ` Xin Li (Talons)
2018-07-03  7:12 ` Jan Beulich
2018-07-03  8:58   ` Xin Li (Talons)
2018-07-04 16:54   ` George Dunlap
2018-07-05  1:38     ` Xin Li (Talons)
2018-08-17 19:11 ` Daniel De Graaf
2018-06-29  9:28 Xin Li
2018-06-29  9:47 ` Andrew Cooper
2018-06-29 10:04   ` Jan Beulich
2018-07-02  7:34     ` Xin Li (Talons)
2018-07-02  8:24       ` Jan Beulich
2018-07-02  8:39         ` Xin Li (Talons)
2018-07-02  9:04           ` Jan Beulich
2018-07-02  7:21   ` Xin Li (Talons)

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.