[PATCH 1/2] xen/xsm: Introduce new boot parameter xsm

[PATCH 1/2] xen/xsm: Introduce new boot parameter xsm

[Xin Li]

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

[PATCH 2/2] xen/xsm: Add new SILO mode for XSM

[Xin Li]

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

Re: [Non-DoD Source] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm

[DeGraaf, Daniel G]

> 
> 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

Re: [Non-DoD Source] [PATCH 2/2] xen/xsm: Add new SILO mode for XSM

[DeGraaf, Daniel G]

> 
> 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

Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm

[Jan Beulich]

>>> 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

Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM

[Jan Beulich]

>>> 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

Re: [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm

[Xin Li (Talons)]

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

Re: [Non-DoD Source] [PATCH 1/2] xen/xsm: Introduce new boot parameter xsm

[Xin Li (Talons)]

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

Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM

[Xin Li (Talons)]

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

Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM

[Jan Beulich]

>>> 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