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

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

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

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

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

v3: make copies of dummy functions to avoid indirect call.

---
 docs/misc/xen-command-line.markdown |   5 +-
 xen/common/Kconfig                  |  12 +++
 xen/include/xsm/xsm.h               |   6 ++
 xen/xsm/Makefile                    |   1 +
 xen/xsm/silo.c                      | 123 ++++++++++++++++++++++++++++
 xen/xsm/xsm_core.c                  |   9 ++
 6 files changed, 155 insertions(+), 1 deletion(-)
 create mode 100644 xen/xsm/silo.c

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 6a3c0e71c7..e0a9b4d268 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
-> `= default | flask`
+> `= default | flask | silo`
 
 > Default: `default`
 
@@ -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 1a6d6281c1..2fe668ba5a 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -154,6 +154,18 @@ 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.
+
 config LATE_HWDOM
 	bool "Dedicated hardware domain"
 	default n
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/silo.c b/xen/xsm/silo.c
new file mode 100644
index 0000000000..020b0c8e94
--- /dev/null
+++ b/xen/xsm/silo.c
@@ -0,0 +1,123 @@
+/******************************************************************************
+ * 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/>.
+ */
+
+#include <xen/sched.h>
+#include <xsm/xsm.h>
+
+struct xsm_operations silo_xsm_ops;
+
+static int (*dummy_evtchn_unbound) (struct domain *, struct evtchn *, domid_t);
+static int (*dummy_evtchn_interdomain) (struct domain *, struct evtchn *,
+                                        struct domain *, struct evtchn *);
+static int (*dummy_grant_mapref) (struct domain *, struct domain *, uint32_t);
+static int (*dummy_grant_transfer) (struct domain *, struct domain *);
+static int (*dummy_grant_copy) (struct domain *, struct domain *);
+
+/*
+ * 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 = dummy_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 dummy_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 dummy_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 dummy_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 dummy_grant_copy(d1, d2);
+    return -EPERM;
+}
+
+void __init silo_init(void)
+{
+    printk("Initialising XSM SILO mode\n");
+
+    dummy_evtchn_unbound = dummy_xsm_ops.evtchn_unbound;
+    dummy_evtchn_interdomain = dummy_xsm_ops.evtchn_interdomain;
+    dummy_grant_mapref = dummy_xsm_ops.grant_mapref;
+    dummy_grant_transfer = dummy_xsm_ops.grant_transfer;
+    dummy_grant_copy = dummy_xsm_ops.grant_copy;
+
+    silo_xsm_ops = dummy_xsm_ops;
+
+    silo_xsm_ops.evtchn_unbound = silo_evtchn_unbound;
+    silo_xsm_ops.evtchn_interdomain = silo_evtchn_interdomain;
+    silo_xsm_ops.grant_mapref = silo_grant_mapref;
+    silo_xsm_ops.grant_transfer = silo_grant_transfer;
+    silo_xsm_ops.grant_copy = silo_grant_copy;
+
+    xsm_ops = &silo_xsm_ops;
+}
+
+/*
+ * 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 658af40c6e..58409eb0c7 100644
--- a/xen/xsm/xsm_core.c
+++ b/xen/xsm/xsm_core.c
@@ -34,6 +34,7 @@ struct xsm_operations *xsm_ops;
 enum xsm_bootparam {
     XSM_BOOTPARAM_DUMMY,
     XSM_BOOTPARAM_FLASK,
+    XSM_BOOTPARAM_SILO,
 };
 
 static enum xsm_bootparam __initdata xsm_bootparam = XSM_BOOTPARAM_DUMMY;
@@ -47,6 +48,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);
@@ -93,6 +98,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: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM

[Daniel De Graaf]

On 09/28/2018 04:18 AM, Xin Li wrote:
> 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>
>
> v3: make copies of dummy functions to avoid indirect call.

This still makes indirect calls.  You will need to #include xsm/dummy.h,
and a third case in line 44's #ifdef CONFIG_XSM / #else may be required
to adjust XSM_INLINE to avoid compiler warnings.

The function xsm_fixup_ops() will already set the unused parts of your
xsm_operations struct to the dummy functions; you don't need to do that
in silo_init by copying dummy_xsm_ops.  You should use register_xsm to
assign to xsm_ops instead of doing it yourself.

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

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

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

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

v2
To further discuss:
1) is the new Kconfig option XSM_SILO necessary?
we can handle SILO similar as DUMMY, using exsting CONFIG_XSM.

2) explain "unmediated communication channel"

3) is it OK to use the indirect call dummy_xsm_ops.evtchn_unbound?

---
 docs/misc/xen-command-line.markdown |   5 +-
 xen/common/Kconfig                  |  12 ++++
 xen/include/xsm/xsm.h               |   6 ++
 xen/xsm/Makefile                    |   1 +
 xen/xsm/silo.c                      | 102 ++++++++++++++++++++++++++++
 xen/xsm/xsm_core.c                  |   9 +++
 6 files changed, 134 insertions(+), 1 deletion(-)
 create mode 100644 xen/xsm/silo.c

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 7ca34aa273..6bbd67b436 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -866,7 +866,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`
 
@@ -877,6 +877,9 @@ the hypervisor was compiled with XSM support.
   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 068c3206a1..1f36dfcc5f 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -143,6 +143,18 @@ 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.
+
 config LATE_HWDOM
 	bool "Dedicated hardware domain"
 	default n
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 70e7a6849f..11518e5bd6 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -733,6 +733,12 @@ extern const unsigned char xsm_init_flask_policy[];
 extern const unsigned int xsm_init_flask_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/silo.c b/xen/xsm/silo.c
new file mode 100644
index 0000000000..d4416f3a95
--- /dev/null
+++ b/xen/xsm/silo.c
@@ -0,0 +1,102 @@
+/******************************************************************************
+ * 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/>.
+ */
+
+#include <xen/sched.h>
+#include <xsm/xsm.h>
+
+struct xsm_operations silo_xsm_ops;
+
+/*
+ * Check if inter-domain communication is allowed.
+ * Return true when pass check.
+ */
+static bool silo_mode_dom_check(struct domain *ldom, struct domain *rdom)
+{
+    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_id(id2);
+    if ( d2 != NULL && silo_mode_dom_check(d1, d2) )
+        rc = dummy_xsm_ops.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 dummy_xsm_ops.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 dummy_xsm_ops.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 dummy_xsm_ops.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 dummy_xsm_ops.grant_copy(d1, d2);
+    return -EPERM;
+}
+
+void __init silo_init(void)
+{
+    printk("Initialising XSM SILO mode\n");
+
+    silo_xsm_ops = dummy_xsm_ops;
+
+    silo_xsm_ops.evtchn_unbound = silo_evtchn_unbound;
+    silo_xsm_ops.evtchn_interdomain = silo_evtchn_interdomain;
+    silo_xsm_ops.grant_mapref = silo_grant_mapref;
+    silo_xsm_ops.grant_transfer = silo_grant_transfer;
+    silo_xsm_ops.grant_copy = silo_grant_copy;
+
+    xsm_ops = &silo_xsm_ops;
+}
+
+/*
+ * 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 d4668edad7..89d5fd3b2e 100644
--- a/xen/xsm/xsm_core.c
+++ b/xen/xsm/xsm_core.c
@@ -34,6 +34,7 @@ struct xsm_operations *xsm_ops;
 enum xsm_bootparam {
     XSM_BOOTPARAM_DUMMY,
     XSM_BOOTPARAM_FLASK,
+    XSM_BOOTPARAM_SILO,
 };
 
 static enum xsm_bootparam __initdata xsm_bootparam = XSM_BOOTPARAM_DUMMY;
@@ -46,6 +47,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
         rc = -EINVAL;
@@ -91,6 +96,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: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM

[Jan Beulich]

>>> On 03.07.18 at 03:26, <talons.lee@gmail.com> wrote:
> v2
> To further discuss:
> 1) is the new Kconfig option XSM_SILO necessary?
> we can handle SILO similar as DUMMY, using exsting CONFIG_XSM.
> 
> 2) explain "unmediated communication channel"

I'm confused: As said in the reply to patch 1, this section is generally
expected to contain information on what has changed from the prior
version. I take it the above item instead related to the "To further
discuss" sub-heading.

> 3) is it OK to use the indirect call dummy_xsm_ops.evtchn_unbound?

I'm not convinced it was worthwhile to send v2 with all of these still
open.

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

const (three times altogether)

> +    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_id(id2);
> +    if ( d2 != NULL && silo_mode_dom_check(d1, d2) )

Blank line please between declaration(s) and statement(s). And
const on the local variable declaration again.

Also, is DOMID_SELF really not allowed here for id2? I don't think
so, looking at e.g. evtchn_alloc_unbound().

> +static int silo_grant_copy(struct domain *d1, struct domain *d2)
> +{
> +    if ( silo_mode_dom_check(d1, d2) )
> +        return dummy_xsm_ops.grant_copy(d1, d2);
> +    return -EPERM;
> +}

I know transitive grants are a bad child, but they shouldn't be left out
altogether in deciding what SILO mode is going to mean. In fact it looks
to me as if there was no XSM check at all for the second half of a
transitive grant copy's domain handling (the recursive
acquire_grant_for_copy() call), which would mean that the fencing
SILO mode looks to mean to establish wouldn't be complete. Daniel?

Speaking of completeness: What about TMEM? Isn't one of the two
pool types also meant to allow page sharing between domains?

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

[Xin Li (Talons)]

Hello Jan,

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, July 3, 2018 3:34 PM
> To: Xin Li <talons.lee@gmail.com>; Daniel de Graaf <dgdegra@tycho.nsa.gov>
> 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>; Tim
> (Xen.org) <tim@xen.org>
> Subject: Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
> 
> >>> On 03.07.18 at 03:26, <talons.lee@gmail.com> wrote:
> > v2
> > To further discuss:
> > 1) is the new Kconfig option XSM_SILO necessary?
> > we can handle SILO similar as DUMMY, using exsting CONFIG_XSM.
> >
> > 2) explain "unmediated communication channel"
> 
> I'm confused: As said in the reply to patch 1, this section is generally expected
> to contain information on what has changed from the prior version. I take it the
> above item instead related to the "To further discuss" sub-heading.
> 
> > 3) is it OK to use the indirect call dummy_xsm_ops.evtchn_unbound?
> 
> I'm not convinced it was worthwhile to send v2 with all of these still open.

OK.

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

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);
}
> 
> > +    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_id(id2);
> > +    if ( d2 != NULL && silo_mode_dom_check(d1, d2) )
> 
> Blank line please between declaration(s) and statement(s). And const on the
> local variable declaration again.
> 
> Also, is DOMID_SELF really not allowed here for id2? I don't think so, looking at
> e.g. evtchn_alloc_unbound().

static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn,
                               domid_t id2)
{   
    int rc = -EPERM;
    struct domain *d2;
    
    if ( id2 == DOMID_SELF )
        id2 = current->domain->domain_id;
    d2 = rcu_lock_domain_by_id(id2);
    if ( d2 != NULL && silo_mode_dom_check(d1, d2) )
        rc = dummy_xsm_ops.evtchn_unbound(d1, chn, id2);
    rcu_unlock_domain(d2);
    
    return rc;
}

> 
> > +static int silo_grant_copy(struct domain *d1, struct domain *d2) {
> > +    if ( silo_mode_dom_check(d1, d2) )
> > +        return dummy_xsm_ops.grant_copy(d1, d2);
> > +    return -EPERM;
> > +}
> 
> I know transitive grants are a bad child, but they shouldn't be left out
> altogether in deciding what SILO mode is going to mean. In fact it looks to me
> as if there was no XSM check at all for the second half of a transitive grant
> copy's domain handling (the recursive
> acquire_grant_for_copy() call), which would mean that the fencing SILO mode
> looks to mean to establish wouldn't be complete. Daniel?
> 
> Speaking of completeness: What about TMEM? Isn't one of the two pool types
> also meant to allow page sharing between domains?
> 
> 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 03.07.18 at 11:07, <xin.li@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Tuesday, July 3, 2018 3:34 PM
>> >>> On 03.07.18 at 03:26, <talons.lee@gmail.com> wrote:
>> > +    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_id(id2);
>> > +    if ( d2 != NULL && silo_mode_dom_check(d1, d2) )
>> 
>> Blank line please between declaration(s) and statement(s). And const on the
>> local variable declaration again.
>> 
>> Also, is DOMID_SELF really not allowed here for id2? I don't think so, looking at
>> e.g. evtchn_alloc_unbound().
> 
> static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn,
>                                domid_t id2)
> {   
>     int rc = -EPERM;
>     struct domain *d2;
>     
>     if ( id2 == DOMID_SELF )
>         id2 = current->domain->domain_id;
>     d2 = rcu_lock_domain_by_id(id2);

No - simply call rcu_lock_domain_by_any_id().

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

[Xin Li (Talons)]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, July 3, 2018 6:16 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 2/2] xen/xsm: Add new SILO mode for XSM
> 
> >>> On 03.07.18 at 11:07, <xin.li@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Tuesday, July 3, 2018 3:34 PM
> >> >>> On 03.07.18 at 03:26, <talons.lee@gmail.com> wrote:
> >> > +    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_id(id2);
> >> > +    if ( d2 != NULL && silo_mode_dom_check(d1, d2) )
> >>
> >> Blank line please between declaration(s) and statement(s). And const
> >> on the local variable declaration again.
> >>
> >> Also, is DOMID_SELF really not allowed here for id2? I don't think
> >> so, looking at e.g. evtchn_alloc_unbound().
> >
> > static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn,
> >                                domid_t id2)
> > {
> >     int rc = -EPERM;
> >     struct domain *d2;
> >
> >     if ( id2 == DOMID_SELF )
> >         id2 = current->domain->domain_id;
> >     d2 = rcu_lock_domain_by_id(id2);
> 
> No - simply call rcu_lock_domain_by_any_id().

Sure.

+    int rc = -EPERM;
+    struct domain *d2 = rcu_lock_domain_by_any_id(id2);
+ 
+    if ( d2 != NULL && silo_mode_dom_check(d1, d2) )

> 
> Jan
> 


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

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

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

[Xin Li]

When SILO is enabled, there would be no page-sharing 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>
---
 docs/misc/xen-command-line.markdown |   3 +
 xen/common/Kconfig                  |  11 +++
 xen/include/xsm/xsm.h               |   6 ++
 xen/xsm/Makefile                    |   1 +
 xen/xsm/silo.c                      | 106 ++++++++++++++++++++++++++++
 xen/xsm/xsm_core.c                  |   9 +++
 6 files changed, 136 insertions(+)
 create mode 100644 xen/xsm/silo.c

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 7c689b8225..454de11c3d 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -877,6 +877,9 @@ the hypervisor was compiled with XSM support.
   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 068c3206a1..f3f8e5afbc 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -143,6 +143,17 @@ 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 will deny any unmediated communication channels between unprivileged
+	  VMs.
+
+	  If unsure, say Y.
+
 config LATE_HWDOM
 	bool "Dedicated hardware domain"
 	default n
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 70e7a6849f..11518e5bd6 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -733,6 +733,12 @@ extern const unsigned char xsm_init_flask_policy[];
 extern const unsigned int xsm_init_flask_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/silo.c b/xen/xsm/silo.c
new file mode 100644
index 0000000000..cac22432da
--- /dev/null
+++ b/xen/xsm/silo.c
@@ -0,0 +1,106 @@
+/******************************************************************************
+ * xsm/silo.c
+ *
+ * SILO module for XSM(Xen Security Modules)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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/>.
+ *
+ * Copyright (c) 2018 Citrix Systems Ltd.
+ */
+
+#include <xen/sched.h>
+#include <xsm/xsm.h>
+
+struct xsm_operations silo_xsm_ops;
+
+/*
+ * check if inter-domain communication is allowed
+ * return true when pass check
+ */
+static bool silo_mode_dom_check(domid_t ldom, domid_t rdom)
+{
+    domid_t hd_dom = hardware_domain->domain_id;
+    domid_t cur_dom = current->domain->domain_id;
+
+    if ( ldom == DOMID_SELF )
+        ldom = cur_dom;
+    if ( rdom == DOMID_SELF )
+        rdom = cur_dom;
+
+    return (hd_dom == cur_dom || hd_dom == ldom || hd_dom == rdom ||
+            ldom == rdom);
+}
+
+static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn,
+                               domid_t id2)
+{
+    if ( silo_mode_dom_check(d1->domain_id, id2) )
+        return dummy_xsm_ops.evtchn_unbound(d1, chn, id2);
+    return -EPERM;
+}
+
+static int silo_evtchn_interdomain(struct domain *d1, struct evtchn *chan1,
+                                   struct domain *d2, struct evtchn *chan2)
+{
+    if ( silo_mode_dom_check(d1->domain_id, d2->domain_id) )
+        return dummy_xsm_ops.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->domain_id, d2->domain_id) )
+        return dummy_xsm_ops.grant_mapref(d1, d2, flags);
+    return -EPERM;
+}
+
+static int silo_grant_transfer(struct domain *d1, struct domain *d2)
+{
+    if ( silo_mode_dom_check(d1->domain_id, d2->domain_id) )
+        return dummy_xsm_ops.grant_transfer(d1, d2);
+    return -EPERM;
+}
+
+static int silo_grant_copy(struct domain *d1, struct domain *d2)
+{
+    if ( silo_mode_dom_check(d1->domain_id, d2->domain_id) )
+        return dummy_xsm_ops.grant_copy(d1, d2);
+    return -EPERM;
+}
+
+void __init silo_init(void)
+{
+    printk("Initialising XSM SILO mode");
+
+    silo_xsm_ops = dummy_xsm_ops;
+
+    silo_xsm_ops.evtchn_unbound = silo_evtchn_unbound;
+    silo_xsm_ops.evtchn_interdomain = silo_evtchn_interdomain;
+    silo_xsm_ops.grant_mapref = silo_grant_mapref;
+    silo_xsm_ops.grant_transfer = silo_grant_transfer;
+    silo_xsm_ops.grant_copy = silo_grant_copy;
+
+    xsm_ops = &silo_xsm_ops;
+}
+
+/*
+ * 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 e002200578..7842f6dd44 100644
--- a/xen/xsm/xsm_core.c
+++ b/xen/xsm/xsm_core.c
@@ -34,6 +34,7 @@ struct xsm_operations *xsm_ops;
 enum xsm_bootparam {
     XSM_BOOTPARAM_DUMMY,
     XSM_BOOTPARAM_FLASK,
+    XSM_BOOTPARAM_SILO,
     XSM_BOOTPARAM_INVALID,
 };
 
@@ -46,6 +47,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
         xsm_bootparam = XSM_BOOTPARAM_INVALID;
@@ -92,6 +97,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");
     }
-- 
2.18.0


_______________________________________________
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

[Andrew Cooper]

On 29/06/18 10:28, Xin Li wrote:
> +void __init silo_init(void)
> +{
> +    printk("Initialising XSM SILO mode");

You need a newline at the end of printk here.

Otherwise, everything else looks in order.

~Andrew

_______________________________________________
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)]

Done.

Best regards

Xin(Talons) Li


> -----Original Message-----
> From: Andrew Cooper
> Sent: Friday, June 29, 2018 5:52 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 2/2] xen/xsm: Add new SILO mode for XSM
> 
> On 29/06/18 10:28, Xin Li wrote:
> > +void __init silo_init(void)
> > +{
> > +    printk("Initialising XSM SILO mode");
> 
> You need a newline at the end of printk here.
> 
> Otherwise, everything else looks in order.
> 
> ~Andrew
_______________________________________________
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.06.18 at 11:28, <talons.lee@gmail.com> wrote:
> When SILO is enabled, there would be no page-sharing between
> unprivileged VMs (no grant tables or event channels).

What is the relation between page sharing and event channels?

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -143,6 +143,17 @@ 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 will deny any unmediated communication channels between unprivileged
> +	  VMs.
> +
> +	  If unsure, say Y.

It would be helpful to clarify here that this is not the default mode of
operation. In fact, another Kconfig (choice) might be useful to have to
select the built-in default. In fact "deny any" suggests that this is what
is going to happen regardless of command line options. At the very
least I think this wants to be "This will allow to deny any ..." or "In this
mode, any ... will by denied".

Andrew, the chosen name here may underline the relevance of my
comment regarding XSM_FLASK vs just FLASK, albeit things are
unclear/ambiguous if I also take into account the code further down.
The descriptions above make it sound as if this was an override to
whatever access control mechanism was in place (dummy or flask
currently). Code below suggests though that this is meant to be a
clone of dummy, with just some minimal adjustments. I guess it's
rather the description that needs adjustment, but the alternative
of being a global override even in FLASK mode certainly exists.

Furthermore it is unclear here what an "unmediated communication
channel" is, and what "mediated communication channels" (if any)
are still available in this new mode.

> --- /dev/null
> +++ b/xen/xsm/silo.c
> @@ -0,0 +1,106 @@
> +/******************************************************************************
> + * xsm/silo.c
> + *
> + * SILO module for XSM(Xen Security Modules)
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that 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/>.
> + *
> + * Copyright (c) 2018 Citrix Systems Ltd.
> + */
> +
> +#include <xen/sched.h>
> +#include <xsm/xsm.h>
> +
> +struct xsm_operations silo_xsm_ops;
> +
> +/*
> + * check if inter-domain communication is allowed
> + * return true when pass check
> + */

Uppercase first letter please, and I'd prefer if you also put a full stop here.

> +static bool silo_mode_dom_check(domid_t ldom, domid_t rdom)
> +{
> +    domid_t hd_dom = hardware_domain->domain_id;

I don't think you mean the hardware domain here, but the control domain
(of which in theory there may be multiple).

> +    domid_t cur_dom = current->domain->domain_id;
> +
> +    if ( ldom == DOMID_SELF )
> +        ldom = cur_dom;
> +    if ( rdom == DOMID_SELF )
> +        rdom = cur_dom;
> +
> +    return (hd_dom == cur_dom || hd_dom == ldom || hd_dom == rdom ||
> +            ldom == rdom);
> +}
> +
> +static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn,
> +                               domid_t id2)
> +{
> +    if ( silo_mode_dom_check(d1->domain_id, id2) )
> +        return dummy_xsm_ops.evtchn_unbound(d1, chn, id2);

Urgh. Why is this not xsm_evtchn_unbound() from dummy.h? It would be
really nice to avoid such extra indirect calls here.

Furthermore, this hook is called in two contexts. Is the above really
appropriate also in the alloc_unbound_xen_event_channel() case?

> +static int silo_grant_mapref(struct domain *d1, struct domain *d2,
> +                             uint32_t flags)
> +{
> +    if ( silo_mode_dom_check(d1->domain_id, d2->domain_id) )
> +        return dummy_xsm_ops.grant_mapref(d1, d2, flags);
> +    return -EPERM;
> +}

What about the unmap counterpart?

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

[Xin Li (Talons)]

Hello Jan,

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, June 29, 2018 6:36 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 2/2] xen/xsm: Add new SILO mode for XSM
> 
> >>> On 29.06.18 at 11:28, <talons.lee@gmail.com> wrote:
> > When SILO is enabled, there would be no page-sharing between
> > unprivileged VMs (no grant tables or event channels).
> 
> What is the relation between page sharing and event channels?

They are the two mechanisms exist for inter-domain communication,
And we want to block them in SILO mode.

> 
> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -143,6 +143,17 @@ 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 will deny any unmediated communication channels between
> unprivileged
> > +	  VMs.
> > +
> > +	  If unsure, say Y.
> 
> It would be helpful to clarify here that this is not the default mode of operation.
> In fact, another Kconfig (choice) might be useful to have to select the built-in
> default. In fact "deny any" suggests that this is what is going to happen
> regardless of command line options. At the very least I think this wants to be
> "This will allow to deny any ..." or "In this mode, any ... will by denied".
> 
> Andrew, the chosen name here may underline the relevance of my comment
> regarding XSM_FLASK vs just FLASK, albeit things are unclear/ambiguous if I
> also take into account the code further down.
> The descriptions above make it sound as if this was an override to whatever
> access control mechanism was in place (dummy or flask currently). Code below
> suggests though that this is meant to be a clone of dummy, with just some
> minimal adjustments. I guess it's rather the description that needs adjustment,
> but the alternative of being a global override even in FLASK mode certainly
> exists.
> 
> Furthermore it is unclear here what an "unmediated communication channel"
> is, and what "mediated communication channels" (if any) are still available in
> this new mode.

Change to:

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.

> 
> > --- /dev/null
> > +++ b/xen/xsm/silo.c
> > @@ -0,0 +1,106 @@
> >
> +/***************************************************************
> *****
> > +**********
> > + * xsm/silo.c
> > + *
> > + * SILO module for XSM(Xen Security Modules)
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License as published
> > +by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that 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/>.
> > + *
> > + * Copyright (c) 2018 Citrix Systems Ltd.
> > + */
> > +
> > +#include <xen/sched.h>
> > +#include <xsm/xsm.h>
> > +
> > +struct xsm_operations silo_xsm_ops;
> > +
> > +/*
> > + * check if inter-domain communication is allowed
> > + * return true when pass check
> > + */
> 
> Uppercase first letter please, and I'd prefer if you also put a full stop here.

OK. Done.

 
> > +static bool silo_mode_dom_check(domid_t ldom, domid_t rdom) {
> > +    domid_t hd_dom = hardware_domain->domain_id;
> 
> I don't think you mean the hardware domain here, but the control domain (of
> which in theory there may be multiple).

I mean the one and only dom0.

> 
> > +    domid_t cur_dom = current->domain->domain_id;
> > +
> > +    if ( ldom == DOMID_SELF )
> > +        ldom = cur_dom;
> > +    if ( rdom == DOMID_SELF )
> > +        rdom = cur_dom;
> > +
> > +    return (hd_dom == cur_dom || hd_dom == ldom || hd_dom == rdom ||
> > +            ldom == rdom);
> > +}
> > +
> > +static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn,
> > +                               domid_t id2) {
> > +    if ( silo_mode_dom_check(d1->domain_id, id2) )
> > +        return dummy_xsm_ops.evtchn_unbound(d1, chn, id2);
> 
> Urgh. Why is this not xsm_evtchn_unbound() from dummy.h? It would be really
> nice to avoid such extra indirect calls here.

This makes it clearer that we are calling the counterpart of dummy ops(overriding).
This indirect calls should not introduce any runtime penalty.

> 
> Furthermore, this hook is called in two contexts. Is the above really appropriate
> also in the alloc_unbound_xen_event_channel() case?
> 
> > +static int silo_grant_mapref(struct domain *d1, struct domain *d2,
> > +                             uint32_t flags) {
> > +    if ( silo_mode_dom_check(d1->domain_id, d2->domain_id) )
> > +        return dummy_xsm_ops.grant_mapref(d1, d2, flags);
> > +    return -EPERM;
> > +}
> 
> What about the unmap counterpart?

This is unnecessary, since we are blocking it from setting up,
Those calling unmap must pass the check of maping.

> 
> Jan


Best regards

Xin(Talons) Li

_______________________________________________
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 02.07.18 at 08:57, <xin.li@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Friday, June 29, 2018 6:36 PM
>> >>> On 29.06.18 at 11:28, <talons.lee@gmail.com> wrote:
>> > When SILO is enabled, there would be no page-sharing between
>> > unprivileged VMs (no grant tables or event channels).
>> 
>> What is the relation between page sharing and event channels?
> 
> They are the two mechanisms exist for inter-domain communication,
> And we want to block them in SILO mode.

I understand this, but it doesn't answer my question. I agree that
grant tables are a means to share pages, but the wording looks odd
to me wrt event channels.

>> > --- a/xen/common/Kconfig
>> > +++ b/xen/common/Kconfig
>> > @@ -143,6 +143,17 @@ 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 will deny any unmediated communication channels between
>> unprivileged
>> > +	  VMs.
>> > +
>> > +	  If unsure, say Y.
>> 
>> It would be helpful to clarify here that this is not the default mode of operation.
>> In fact, another Kconfig (choice) might be useful to have to select the built-in
>> default. In fact "deny any" suggests that this is what is going to happen
>> regardless of command line options. At the very least I think this wants to be
>> "This will allow to deny any ..." or "In this mode, any ... will by denied".
>> 
>> Andrew, the chosen name here may underline the relevance of my comment
>> regarding XSM_FLASK vs just FLASK, albeit things are unclear/ambiguous if I
>> also take into account the code further down.
>> The descriptions above make it sound as if this was an override to whatever
>> access control mechanism was in place (dummy or flask currently). Code below
>> suggests though that this is meant to be a clone of dummy, with just some
>> minimal adjustments. I guess it's rather the description that needs adjustment,
>> but the alternative of being a global override even in FLASK mode certainly
>> exists.
>> 
>> Furthermore it is unclear here what an "unmediated communication channel"
>> is, and what "mediated communication channels" (if any) are still available in
>> this new mode.
> 
> Change to:
> 
> 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.

With s/module/mode/ this is an improvement, but continues to leave open
in particular what an "unmediated communication channel" is.

Btw, thinking about it again - do we need a Kconfig option here in the first
place, when the mode isn't the default, and it's not a whole lot of code
that gets added?

>> > +static bool silo_mode_dom_check(domid_t ldom, domid_t rdom) {
>> > +    domid_t hd_dom = hardware_domain->domain_id;
>> 
>> I don't think you mean the hardware domain here, but the control domain (of
>> which in theory there may be multiple).
> 
> I mean the one and only dom0.

No, for the purpose here you don't mean Dom0, which just happens to
be both hardware and (the only) control domain in most setups. From
a security pov though you need to distinguish all of these.

>> > +    domid_t cur_dom = current->domain->domain_id;
>> > +
>> > +    if ( ldom == DOMID_SELF )
>> > +        ldom = cur_dom;
>> > +    if ( rdom == DOMID_SELF )
>> > +        rdom = cur_dom;
>> > +
>> > +    return (hd_dom == cur_dom || hd_dom == ldom || hd_dom == rdom ||
>> > +            ldom == rdom);
>> > +}
>> > +
>> > +static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn,
>> > +                               domid_t id2) {
>> > +    if ( silo_mode_dom_check(d1->domain_id, id2) )
>> > +        return dummy_xsm_ops.evtchn_unbound(d1, chn, id2);
>> 
>> Urgh. Why is this not xsm_evtchn_unbound() from dummy.h? It would be really
>> nice to avoid such extra indirect calls here.
> 
> This makes it clearer that we are calling the counterpart of dummy 
> ops(overriding).

Yes, but the same level of clarity could be achieved when naming the
function in dummy.h dummy_evtchn_unbound() (aliased to
xsm_evtchn_unbound() for satisfying needs elsewhere).

> This indirect calls should not introduce any runtime penalty.

How does it not, when indirect calls are more expensive than direct
ones already without the Spectre v2 mitigations?

>> Furthermore, this hook is called in two contexts. Is the above really 
> appropriate
>> also in the alloc_unbound_xen_event_channel() case?
>> 
>> > +static int silo_grant_mapref(struct domain *d1, struct domain *d2,
>> > +                             uint32_t flags) {
>> > +    if ( silo_mode_dom_check(d1->domain_id, d2->domain_id) )
>> > +        return dummy_xsm_ops.grant_mapref(d1, d2, flags);
>> > +    return -EPERM;
>> > +}
>> 
>> What about the unmap counterpart?
> 
> This is unnecessary, since we are blocking it from setting up,
> Those calling unmap must pass the check of maping.

Taking that position, the unmap XSM hook's existence is questionable
altogether.

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

[Xin Li (Talons)]

Hello Jan,

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, July 2, 2018 3:29 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 2/2] xen/xsm: Add new SILO mode for XSM
> 
> >>> On 02.07.18 at 08:57, <xin.li@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Friday, June 29, 2018 6:36 PM
> >> >>> On 29.06.18 at 11:28, <talons.lee@gmail.com> wrote:
> >> > When SILO is enabled, there would be no page-sharing between
> >> > unprivileged VMs (no grant tables or event channels).
> >>
> >> What is the relation between page sharing and event channels?
> >
> > They are the two mechanisms exist for inter-domain communication, And
> > we want to block them in SILO mode.
> 
> I understand this, but it doesn't answer my question. I agree that grant tables
> are a means to share pages, but the wording looks odd to me wrt event
> channels.
Do you mean add " or event notifications",
When SILO is enabled, there would be no page-sharing or event notifications
between unprivileged VMs (no grant tables or event channels).

> 
> >> > --- a/xen/common/Kconfig
> >> > +++ b/xen/common/Kconfig
> >> > @@ -143,6 +143,17 @@ 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 will deny any unmediated communication channels between
> >> unprivileged
> >> > +	  VMs.
> >> > +
> >> > +	  If unsure, say Y.
> >>
> >> It would be helpful to clarify here that this is not the default mode of
> operation.
> >> In fact, another Kconfig (choice) might be useful to have to select
> >> the built-in default. In fact "deny any" suggests that this is what
> >> is going to happen regardless of command line options. At the very
> >> least I think this wants to be "This will allow to deny any ..." or "In this mode,
> any ... will by denied".
> >>
> >> Andrew, the chosen name here may underline the relevance of my
> >> comment regarding XSM_FLASK vs just FLASK, albeit things are
> >> unclear/ambiguous if I also take into account the code further down.
> >> The descriptions above make it sound as if this was an override to
> >> whatever access control mechanism was in place (dummy or flask
> >> currently). Code below suggests though that this is meant to be a
> >> clone of dummy, with just some minimal adjustments. I guess it's
> >> rather the description that needs adjustment, but the alternative of
> >> being a global override even in FLASK mode certainly exists.
> >>
> >> Furthermore it is unclear here what an "unmediated communication
> channel"
> >> is, and what "mediated communication channels" (if any) are still
> >> available in this new mode.
> >
> > Change to:
> >
> > 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.
> 
> With s/module/mode/ this is an improvement, but continues to leave open in
> particular what an "unmediated communication channel" is.
This can't prevent side-channel attack.

> 
> Btw, thinking about it again - do we need a Kconfig option here in the first
> place, when the mode isn't the default, and it's not a whole lot of code that gets
> added?
The existing XSM code use Kconfig,
I just want to follow the similar style for new module.
And yes, we can handle it in CONFIG_XSM like dummy.
Which way is better?

> 
> >> > +static bool silo_mode_dom_check(domid_t ldom, domid_t rdom) {
> >> > +    domid_t hd_dom = hardware_domain->domain_id;
> >>
> >> I don't think you mean the hardware domain here, but the control
> >> domain (of which in theory there may be multiple).
> >
> > I mean the one and only dom0.
> 
> No, for the purpose here you don't mean Dom0, which just happens to be both
> hardware and (the only) control domain in most setups. From a security pov
> though you need to distinguish all of these.

Yes! thanks.
I will use 
is_control_domain(d)
instead of comparing the hardware domain id.

This comment is misleading then:
/* Is this guest fully privileged (aka dom0)? */
   bool             is_privileged;

> >> > +    domid_t cur_dom = current->domain->domain_id;
> >> > +
> >> > +    if ( ldom == DOMID_SELF )
> >> > +        ldom = cur_dom;
> >> > +    if ( rdom == DOMID_SELF )
> >> > +        rdom = cur_dom;
> >> > +
> >> > +    return (hd_dom == cur_dom || hd_dom == ldom || hd_dom == rdom
> ||
> >> > +            ldom == rdom);
> >> > +}
> >> > +
> >> > +static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn,
> >> > +                               domid_t id2) {
> >> > +    if ( silo_mode_dom_check(d1->domain_id, id2) )
> >> > +        return dummy_xsm_ops.evtchn_unbound(d1, chn, id2);
> >>
> >> Urgh. Why is this not xsm_evtchn_unbound() from dummy.h? It would be
> >> really nice to avoid such extra indirect calls here.
> >
> > This makes it clearer that we are calling the counterpart of dummy
> > ops(overriding).
> 
> Yes, but the same level of clarity could be achieved when naming the function
> in dummy.h dummy_evtchn_unbound() (aliased to
> xsm_evtchn_unbound() for satisfying needs elsewhere).
> 
> > This indirect calls should not introduce any runtime penalty.
> 
> How does it not, when indirect calls are more expensive than direct ones
> already without the Spectre v2 mitigations?
I only mean it's not runtime binding.

And I ran some performance test before, seems no performance penalty.

The names in dummy.h are the same as xsm.h.
If I call xsm_evtchn_unbound, that's from xsm.h,
It probably call silo_evtchn_unbound, ends up in a loop.

So I may have to rename all the functions in dummy.h,
And remove static...

is it necessary?

> 
> >> Furthermore, this hook is called in two contexts. Is the above really
> > appropriate
> >> also in the alloc_unbound_xen_event_channel() case?
> >>
> >> > +static int silo_grant_mapref(struct domain *d1, struct domain *d2,
> >> > +                             uint32_t flags) {
> >> > +    if ( silo_mode_dom_check(d1->domain_id, d2->domain_id) )
> >> > +        return dummy_xsm_ops.grant_mapref(d1, d2, flags);
> >> > +    return -EPERM;
> >> > +}
> >>
> >> What about the unmap counterpart?
> >
> > This is unnecessary, since we are blocking it from setting up, Those
> > calling unmap must pass the check of maping.
> 
> Taking that position, the unmap XSM hook's existence is questionable
> altogether.

I think that's for completeness.
We can override it only when necessary.
But this change don't have to include that part.

> 
> Jan


Best regards

Xin(Talons) Li

_______________________________________________
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 02.07.18 at 11:22, <xin.li@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Monday, July 2, 2018 3:29 PM
>> >>> On 02.07.18 at 08:57, <xin.li@citrix.com> wrote:
>> >> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> Sent: Friday, June 29, 2018 6:36 PM
>> >> >>> On 29.06.18 at 11:28, <talons.lee@gmail.com> wrote:
>> >> > When SILO is enabled, there would be no page-sharing between
>> >> > unprivileged VMs (no grant tables or event channels).
>> >>
>> >> What is the relation between page sharing and event channels?
>> >
>> > They are the two mechanisms exist for inter-domain communication, And
>> > we want to block them in SILO mode.
>> 
>> I understand this, but it doesn't answer my question. I agree that grant tables
>> are a means to share pages, but the wording looks odd to me wrt event
>> channels.
> Do you mean add " or event notifications",
> When SILO is enabled, there would be no page-sharing or event notifications
> between unprivileged VMs (no grant tables or event channels).

Yes, that's one way to clarify things.

>> > Change to:
>> >
>> > 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.
>> 
>> With s/module/mode/ this is an improvement, but continues to leave open in
>> particular what an "unmediated communication channel" is.
> This can't prevent side-channel attack.

???

>> Btw, thinking about it again - do we need a Kconfig option here in the first
>> place, when the mode isn't the default, and it's not a whole lot of code 
> that gets
>> added?
> The existing XSM code use Kconfig,
> I just want to follow the similar style for new module.
> And yes, we can handle it in CONFIG_XSM like dummy.
> Which way is better?

Daniel, Andrew?

>> >> > +static bool silo_mode_dom_check(domid_t ldom, domid_t rdom) {
>> >> > +    domid_t hd_dom = hardware_domain->domain_id;
>> >>
>> >> I don't think you mean the hardware domain here, but the control
>> >> domain (of which in theory there may be multiple).
>> >
>> > I mean the one and only dom0.
>> 
>> No, for the purpose here you don't mean Dom0, which just happens to be both
>> hardware and (the only) control domain in most setups. From a security pov
>> though you need to distinguish all of these.
> 
> Yes! thanks.
> I will use 
> is_control_domain(d)
> instead of comparing the hardware domain id.
> 
> This comment is misleading then:
> /* Is this guest fully privileged (aka dom0)? */
>    bool             is_privileged;

Yes, but it's likely going to remain that way until further disaggregation
work would happen.

>> >> > +    domid_t cur_dom = current->domain->domain_id;
>> >> > +
>> >> > +    if ( ldom == DOMID_SELF )
>> >> > +        ldom = cur_dom;
>> >> > +    if ( rdom == DOMID_SELF )
>> >> > +        rdom = cur_dom;
>> >> > +
>> >> > +    return (hd_dom == cur_dom || hd_dom == ldom || hd_dom == rdom
>> ||
>> >> > +            ldom == rdom);
>> >> > +}
>> >> > +
>> >> > +static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn,
>> >> > +                               domid_t id2) {
>> >> > +    if ( silo_mode_dom_check(d1->domain_id, id2) )
>> >> > +        return dummy_xsm_ops.evtchn_unbound(d1, chn, id2);
>> >>
>> >> Urgh. Why is this not xsm_evtchn_unbound() from dummy.h? It would be
>> >> really nice to avoid such extra indirect calls here.
>> >
>> > This makes it clearer that we are calling the counterpart of dummy
>> > ops(overriding).
>> 
>> Yes, but the same level of clarity could be achieved when naming the function
>> in dummy.h dummy_evtchn_unbound() (aliased to
>> xsm_evtchn_unbound() for satisfying needs elsewhere).
>> 
>> > This indirect calls should not introduce any runtime penalty.
>> 
>> How does it not, when indirect calls are more expensive than direct ones
>> already without the Spectre v2 mitigations?
> I only mean it's not runtime binding.
> 
> And I ran some performance test before, seems no performance penalty.

Sure, these paths aren't normally performance critical. But by doing what
you do we'd have a bad precedent, and if someone later cloned your
solution into something that does sit on a performance critical path, we'd
have a problem.

> The names in dummy.h are the same as xsm.h.
> If I call xsm_evtchn_unbound, that's from xsm.h,
> It probably call silo_evtchn_unbound, ends up in a loop.
> 
> So I may have to rename all the functions in dummy.h,

Note how I've said "naming the function in dummy.h
dummy_evtchn_unbound() (aliased to xsm_evtchn_unbound()
for satisfying needs elsewhere)."

> And remove static...

I don't think so, no.

> is it necessary?

It should imo be at least considered. But Daniel as the maintainer may
have something to say here as well...

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

[Xin Li (Talons)]

Hi
    I think the main questions here are:
    1. Do we need a separated KConfig option for SILO
    2. Can we use indirect call like "dummy_xsm_ops.grant_copy"
    Any suggestion?

Best regards

Xin(Talons) Li

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, July 2, 2018 5:39 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 2/2] xen/xsm: Add new SILO mode for XSM
> 
> >>> On 02.07.18 at 11:22, <xin.li@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Monday, July 2, 2018 3:29 PM
> >> >>> On 02.07.18 at 08:57, <xin.li@citrix.com> wrote:
> >> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >> Sent: Friday, June 29, 2018 6:36 PM
> >> >> >>> On 29.06.18 at 11:28, <talons.lee@gmail.com> wrote:
> >> >> > When SILO is enabled, there would be no page-sharing between
> >> >> > unprivileged VMs (no grant tables or event channels).
> >> >>
> >> >> What is the relation between page sharing and event channels?
> >> >
> >> > They are the two mechanisms exist for inter-domain communication,
> >> > And we want to block them in SILO mode.
> >>
> >> I understand this, but it doesn't answer my question. I agree that
> >> grant tables are a means to share pages, but the wording looks odd to
> >> me wrt event channels.
> > Do you mean add " or event notifications", When SILO is enabled, there
> > would be no page-sharing or event notifications between unprivileged
> > VMs (no grant tables or event channels).
> 
> Yes, that's one way to clarify things.
> 
> >> > Change to:
> >> >
> >> > 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.
> >>
> >> With s/module/mode/ this is an improvement, but continues to leave
> >> open in particular what an "unmediated communication channel" is.
> > This can't prevent side-channel attack.
> 
> ???
> 
> >> Btw, thinking about it again - do we need a Kconfig option here in
> >> the first place, when the mode isn't the default, and it's not a
> >> whole lot of code
> > that gets
> >> added?
> > The existing XSM code use Kconfig,
> > I just want to follow the similar style for new module.
> > And yes, we can handle it in CONFIG_XSM like dummy.
> > Which way is better?
> 
> Daniel, Andrew?
> 
> >> >> > +static bool silo_mode_dom_check(domid_t ldom, domid_t rdom) {
> >> >> > +    domid_t hd_dom = hardware_domain->domain_id;
> >> >>
> >> >> I don't think you mean the hardware domain here, but the control
> >> >> domain (of which in theory there may be multiple).
> >> >
> >> > I mean the one and only dom0.
> >>
> >> No, for the purpose here you don't mean Dom0, which just happens to
> >> be both hardware and (the only) control domain in most setups. From a
> >> security pov though you need to distinguish all of these.
> >
> > Yes! thanks.
> > I will use
> > is_control_domain(d)
> > instead of comparing the hardware domain id.
> >
> > This comment is misleading then:
> > /* Is this guest fully privileged (aka dom0)? */
> >    bool             is_privileged;
> 
> Yes, but it's likely going to remain that way until further disaggregation work
> would happen.
> 
> >> >> > +    domid_t cur_dom = current->domain->domain_id;
> >> >> > +
> >> >> > +    if ( ldom == DOMID_SELF )
> >> >> > +        ldom = cur_dom;
> >> >> > +    if ( rdom == DOMID_SELF )
> >> >> > +        rdom = cur_dom;
> >> >> > +
> >> >> > +    return (hd_dom == cur_dom || hd_dom == ldom || hd_dom ==
> >> >> > + rdom
> >> ||
> >> >> > +            ldom == rdom);
> >> >> > +}
> >> >> > +
> >> >> > +static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn,
> >> >> > +                               domid_t id2) {
> >> >> > +    if ( silo_mode_dom_check(d1->domain_id, id2) )
> >> >> > +        return dummy_xsm_ops.evtchn_unbound(d1, chn, id2);
> >> >>
> >> >> Urgh. Why is this not xsm_evtchn_unbound() from dummy.h? It would
> >> >> be really nice to avoid such extra indirect calls here.
> >> >
> >> > This makes it clearer that we are calling the counterpart of dummy
> >> > ops(overriding).
> >>
> >> Yes, but the same level of clarity could be achieved when naming the
> >> function in dummy.h dummy_evtchn_unbound() (aliased to
> >> xsm_evtchn_unbound() for satisfying needs elsewhere).
> >>
> >> > This indirect calls should not introduce any runtime penalty.
> >>
> >> How does it not, when indirect calls are more expensive than direct
> >> ones already without the Spectre v2 mitigations?
> > I only mean it's not runtime binding.
> >
> > And I ran some performance test before, seems no performance penalty.
> 
> Sure, these paths aren't normally performance critical. But by doing what you
> do we'd have a bad precedent, and if someone later cloned your solution into
> something that does sit on a performance critical path, we'd have a problem.
> 
> > The names in dummy.h are the same as xsm.h.
> > If I call xsm_evtchn_unbound, that's from xsm.h, It probably call
> > silo_evtchn_unbound, ends up in a loop.
> >
> > So I may have to rename all the functions in dummy.h,
> 
> Note how I've said "naming the function in dummy.h
> dummy_evtchn_unbound() (aliased to xsm_evtchn_unbound() for satisfying
> needs elsewhere)."
> 
> > And remove static...
> 
> I don't think so, no.
> 
> > is it necessary?
> 
> It should imo be at least considered. But Daniel as the maintainer may have
> something to say here as well...
> 
> 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 23.07.18 at 12:45, <xin.li@citrix.com> wrote:
>     I think the main questions here are:
>     1. Do we need a separated KConfig option for SILO
>     2. Can we use indirect call like "dummy_xsm_ops.grant_copy"
>     Any suggestion?

I'm afraid the addressing of your mail is misleading: I've voiced my
opinion, so I'm unlikely the one you've meant to be on the To list.

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

[Xin Li (Talons)]

 Hi Daniel,

     I think the main questions here are:
     1. Do we need a separated KConfig option for SILO
     2. Can we use indirect call like "dummy_xsm_ops.grant_copy"
     Any suggestion?
 
Best regards
 
Xin(Talons) Li
 
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@suse.com]
> > Sent: Monday, July 2, 2018 5:39 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 2/2] xen/xsm: Add new SILO mode for XSM
> >
> > >>> On 02.07.18 at 11:22, <xin.li@citrix.com> wrote:
> > >> From: Jan Beulich [mailto:JBeulich@suse.com]
> > >> Sent: Monday, July 2, 2018 3:29 PM
> > >> >>> On 02.07.18 at 08:57, <xin.li@citrix.com> wrote:
> > >> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> > >> >> Sent: Friday, June 29, 2018 6:36 PM
> > >> >> >>> On 29.06.18 at 11:28, <talons.lee@gmail.com> wrote:
> > >> >> > When SILO is enabled, there would be no page-sharing between
> > >> >> > unprivileged VMs (no grant tables or event channels).
> > >> >>
> > >> >> What is the relation between page sharing and event channels?
> > >> >
> > >> > They are the two mechanisms exist for inter-domain communication,
> > >> > And we want to block them in SILO mode.
> > >>
> > >> I understand this, but it doesn't answer my question. I agree that
> > >> grant tables are a means to share pages, but the wording looks odd
> > >> to me wrt event channels.
> > > Do you mean add " or event notifications", When SILO is enabled,
> > > there would be no page-sharing or event notifications between
> > > unprivileged VMs (no grant tables or event channels).
> >
> > Yes, that's one way to clarify things.
> >
> > >> > Change to:
> > >> >
> > >> > 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.
> > >>
> > >> With s/module/mode/ this is an improvement, but continues to leave
> > >> open in particular what an "unmediated communication channel" is.
> > > This can't prevent side-channel attack.
> >
> > ???
> >
> > >> Btw, thinking about it again - do we need a Kconfig option here in
> > >> the first place, when the mode isn't the default, and it's not a
> > >> whole lot of code
> > > that gets
> > >> added?
> > > The existing XSM code use Kconfig,
> > > I just want to follow the similar style for new module.
> > > And yes, we can handle it in CONFIG_XSM like dummy.
> > > Which way is better?
> >
> > Daniel, Andrew?
> >
> > >> >> > +static bool silo_mode_dom_check(domid_t ldom, domid_t rdom) {
> > >> >> > +    domid_t hd_dom = hardware_domain->domain_id;
> > >> >>
> > >> >> I don't think you mean the hardware domain here, but the control
> > >> >> domain (of which in theory there may be multiple).
> > >> >
> > >> > I mean the one and only dom0.
> > >>
> > >> No, for the purpose here you don't mean Dom0, which just happens to
> > >> be both hardware and (the only) control domain in most setups. From
> > >> a security pov though you need to distinguish all of these.
> > >
> > > Yes! thanks.
> > > I will use
> > > is_control_domain(d)
> > > instead of comparing the hardware domain id.
> > >
> > > This comment is misleading then:
> > > /* Is this guest fully privileged (aka dom0)? */
> > >    bool             is_privileged;
> >
> > Yes, but it's likely going to remain that way until further
> > disaggregation work would happen.
> >
> > >> >> > +    domid_t cur_dom = current->domain->domain_id;
> > >> >> > +
> > >> >> > +    if ( ldom == DOMID_SELF )
> > >> >> > +        ldom = cur_dom;
> > >> >> > +    if ( rdom == DOMID_SELF )
> > >> >> > +        rdom = cur_dom;
> > >> >> > +
> > >> >> > +    return (hd_dom == cur_dom || hd_dom == ldom || hd_dom ==
> > >> >> > + rdom
> > >> ||
> > >> >> > +            ldom == rdom);
> > >> >> > +}
> > >> >> > +
> > >> >> > +static int silo_evtchn_unbound(struct domain *d1, struct evtchn *chn,
> > >> >> > +                               domid_t id2) {
> > >> >> > +    if ( silo_mode_dom_check(d1->domain_id, id2) )
> > >> >> > +        return dummy_xsm_ops.evtchn_unbound(d1, chn, id2);
> > >> >>
> > >> >> Urgh. Why is this not xsm_evtchn_unbound() from dummy.h? It
> > >> >> would be really nice to avoid such extra indirect calls here.
> > >> >
> > >> > This makes it clearer that we are calling the counterpart of
> > >> > dummy ops(overriding).
> > >>
> > >> Yes, but the same level of clarity could be achieved when naming
> > >> the function in dummy.h dummy_evtchn_unbound() (aliased to
> > >> xsm_evtchn_unbound() for satisfying needs elsewhere).
> > >>
> > >> > This indirect calls should not introduce any runtime penalty.
> > >>
> > >> How does it not, when indirect calls are more expensive than direct
> > >> ones already without the Spectre v2 mitigations?
> > > I only mean it's not runtime binding.
> > >
> > > And I ran some performance test before, seems no performance penalty.
> >
> > Sure, these paths aren't normally performance critical. But by doing
> > what you do we'd have a bad precedent, and if someone later cloned
> > your solution into something that does sit on a performance critical path,
> we'd have a problem.
> >
> > > The names in dummy.h are the same as xsm.h.
> > > If I call xsm_evtchn_unbound, that's from xsm.h, It probably call
> > > silo_evtchn_unbound, ends up in a loop.
> > >
> > > So I may have to rename all the functions in dummy.h,
> >
> > Note how I've said "naming the function in dummy.h
> > dummy_evtchn_unbound() (aliased to xsm_evtchn_unbound() for satisfying
> > needs elsewhere)."
> >
> > > And remove static...
> >
> > I don't think so, no.
> >
> > > is it necessary?
> >
> > It should imo be at least considered. But Daniel as the maintainer may
> > have something to say here as well...
> >
> > 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

[Daniel De Graaf]

On 07/24/2018 04:18 AM, Xin Li (Talons) wrote:
>   Hi Daniel,
> 
>       I think the main questions here are:
>       1. Do we need a separated KConfig option for SILO

Yes; I made comments on your patch doing so

>       2. Can we use indirect call like "dummy_xsm_ops.grant_copy"
>       Any suggestion?

It would be nice to avoid it.  Making a global dummy_xsm_grant_copy()
function would be better, though the best location of the prototype and
code isn't obvious.

_______________________________________________
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

[Julien Grall]

Hello,

On 29/06/18 10:28, Xin Li wrote:
> diff --git a/xen/xsm/silo.c b/xen/xsm/silo.c
> new file mode 100644
> index 0000000000..cac22432da
> --- /dev/null
> +++ b/xen/xsm/silo.c
> @@ -0,0 +1,106 @@
> +/******************************************************************************
> + * xsm/silo.c
> + *
> + * SILO module for XSM(Xen Security Modules)
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.

Xen hypervisor is licensed as GPLv2 only. However, this header is 
allowing GPLv2+. Can you please update the header accordingly?

Cheers,

-- 
Julien Grall

_______________________________________________
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)]

OK.
Change to:

/******************************************************************************
 * 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/>.
 */

Best regards

Xin(Talons) Li

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: Friday, June 29, 2018 9:22 PM
> To: Xin Li <talons.lee@gmail.com>; xen-devel@lists.xen.org
> Cc: Sergey Dyasli <sergey.dyasli@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Xin Li (Talons) <xin.li@citrix.com>; Tim (Xen.org)
> <tim@xen.org>; Jan Beulich <JBeulich@suse.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Ming Lu <ming.lu@citrix.com>; Daniel De
> Graaf <dgdegra@tycho.nsa.gov>
> Subject: Re: [Xen-devel] [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
> 
> Hello,
> 
> On 29/06/18 10:28, Xin Li wrote:
> > diff --git a/xen/xsm/silo.c b/xen/xsm/silo.c new file mode 100644
> > index 0000000000..cac22432da
> > --- /dev/null
> > +++ b/xen/xsm/silo.c
> > @@ -0,0 +1,106 @@
> >
> +/***************************************************************
> *****
> > +**********
> > + * xsm/silo.c
> > + *
> > + * SILO module for XSM(Xen Security Modules)
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License as published
> > +by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> 
> Xen hypervisor is licensed as GPLv2 only. However, this header is allowing
> GPLv2+. Can you please update the header accordingly?
> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel