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

[Jan Beulich]

>>> On 08.10.18 at 09:49,  wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: Tuesday, October 2, 2018 5:33 PM
>> 
>> >>> On 29.09.18 at 11:22,  wrote:
>> > --- a/xen/xsm/dummy.c
>> > +++ b/xen/xsm/dummy.c
>> > @@ -11,7 +11,6 @@
>> >   */
>> >
>> >  #define XSM_NO_WRAPPERS
>> > -#define XSM_INLINE /* */
>> >  #include 
>> 
>> 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: [Xen-devel] [PATCH 2/2] xen/xsm: Add new SILO mode for XSM

[Xin Li (Talons)]

Thanks Jan.

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Tuesday, October 2, 2018 5:33 PM
> To: Xin Li 
> Cc: Andrew Cooper ; Ming Lu
> ; Sergey Dyasli ; Wei Liu
> ; Xin Li (Talons) ; George Dunlap
> ; Stefano Stabellini ; xen-
> de...@lists.xen.org; Konrad Rzeszutek Wilk ; Daniel
> de Graaf ; Tim (Xen.org) 
> Subject: Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
> 
> >>> On 29.09.18 at 11:22,  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 
> 
> 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 .
> > + */
> > +#define XSM_NO_WRAPPERS
> > +
> > +#include 
> 
> 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: [Xen-devel] [PATCH 2/2] xen/xsm: Add new SILO mode for XSM

[Jan Beulich]

>>> On 29.09.18 at 11:22,  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 

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 .
> + */
> +#define XSM_NO_WRAPPERS
> +
> +#include 

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

[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 

---
CC: Daniel De Graaf 
CC: George Dunlap 
CC: Jan Beulich 
CC: Konrad Rzeszutek Wilk 
CC: Stefano Stabellini 
CC: Tim Deegan 
CC: Wei Liu 
CC: Sergey Dyasli 
CC: Andrew Cooper 
CC: Ming Lu 

v4:
1. 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 
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 
 
 struct xsm_operations dummy_xsm_ops;
diff --git a/xen/xsm/silo.c b/xen/xsm/silo.c
new file mode 100644
index 00..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 

Re: [Xen-devel] [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 

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

[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 

---
CC: Daniel De Graaf 
CC: George Dunlap 
CC: Jan Beulich 
CC: Konrad Rzeszutek Wilk 
CC: Stefano Stabellini 
CC: Tim Deegan 
CC: Wei Liu 
CC: Sergey Dyasli 
CC: Andrew Cooper 
CC: Ming Lu 

v3: 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 
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 00..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 .
+ */
+
+#include 
+#include 
+
+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) || 

Re: [Xen-devel] [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: [Xen-devel] [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:jbeul...@suse.com]
> > Sent: Monday, July 2, 2018 5:39 PM
> > To: Xin Li (Talons) ; Xin Li 
> > Cc: Andrew Cooper ; George Dunlap
> > ; Ming Lu ; Sergey
> > Dyasli ; Wei Liu ;
> > Stefano Stabellini ; xen-de...@lists.xen.org;
> > Konrad Rzeszutek Wilk ; Daniel de Graaf
> > ; Tim
> > (Xen.org) 
> > Subject: RE: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
> >
> > >>> On 02.07.18 at 11:22,  wrote:
> > >> From: Jan Beulich [mailto:jbeul...@suse.com]
> > >> Sent: Monday, July 2, 2018 3:29 PM
> > >> >>> On 02.07.18 at 08:57,  wrote:
> > >> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> > >> >> Sent: Friday, June 29, 2018 6:36 PM
> > >> >> >>> On 29.06.18 at 11:28,  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
> > >> 

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

[Jan Beulich]

>>> On 23.07.18 at 12:45,  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: [Xen-devel] [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:jbeul...@suse.com]
> Sent: Monday, July 2, 2018 5:39 PM
> To: Xin Li (Talons) ; Xin Li 
> Cc: Andrew Cooper ; George Dunlap
> ; Ming Lu ; Sergey Dyasli
> ; Wei Liu ; Stefano Stabellini
> ; xen-de...@lists.xen.org; Konrad Rzeszutek Wilk
> ; Daniel de Graaf ; Tim
> (Xen.org) 
> Subject: RE: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
> 
> >>> On 02.07.18 at 11:22,  wrote:
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: Monday, July 2, 2018 3:29 PM
> >> >>> On 02.07.18 at 08:57,  wrote:
> >> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> >> Sent: Friday, June 29, 2018 6:36 PM
> >> >> >>> On 29.06.18 at 11:28,  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 

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

[Xin Li (Talons)]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Tuesday, July 3, 2018 6:16 PM
> To: Xin Li (Talons) ; Xin Li 
> Cc: Andrew Cooper ; George Dunlap
> ; Ming Lu ; Sergey Dyasli
> ; Wei Liu ; Stefano Stabellini
> ; xen-de...@lists.xen.org; Konrad Rzeszutek Wilk
> ; Daniel de Graaf ; Tim
> (Xen.org) 
> Subject: RE: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
> 
> >>> On 03.07.18 at 11:07,  wrote:
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: Tuesday, July 3, 2018 3:34 PM
> >> >>> On 03.07.18 at 03:26,  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

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

[Jan Beulich]

>>> On 03.07.18 at 11:07,  wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: Tuesday, July 3, 2018 3:34 PM
>> >>> On 03.07.18 at 03:26,  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: [Xen-devel] [PATCH 2/2] xen/xsm: Add new SILO mode for XSM

[Xin Li (Talons)]

Hello Jan,

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Tuesday, July 3, 2018 3:34 PM
> To: Xin Li ; Daniel de Graaf 
> Cc: Andrew Cooper ; Ming Lu
> ; Sergey Dyasli ; Wei Liu
> ; Xin Li (Talons) ; George Dunlap
> ; Stefano Stabellini ; xen-
> de...@lists.xen.org; Konrad Rzeszutek Wilk ; Tim
> (Xen.org) 
> Subject: Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
> 
> >>> On 03.07.18 at 03:26,  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: [Xen-devel] [PATCH 2/2] xen/xsm: Add new SILO mode for XSM

[Jan Beulich]

>>> On 03.07.18 at 03:26,  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

[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 

---
CC: Daniel De Graaf 
CC: George Dunlap 
CC: Jan Beulich 
CC: Konrad Rzeszutek Wilk 
CC: Stefano Stabellini 
CC: Tim Deegan 
CC: Wei Liu 
CC: Sergey Dyasli 
CC: Andrew Cooper 
CC: Ming Lu 

v2
To further discuss:
1) is 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 
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 00..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 .
+ */
+
+#include 
+#include 
+
+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 = 

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

[Jan Beulich]

>>> On 02.07.18 at 11:22,  wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: Monday, July 2, 2018 3:29 PM
>> >>> On 02.07.18 at 08:57,  wrote:
>> >> From: Jan Beulich [mailto:jbeul...@suse.com]
>> >> Sent: Friday, June 29, 2018 6:36 PM
>> >> >>> On 29.06.18 at 11:28,  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 

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

[Xin Li (Talons)]

Hello Jan,

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Monday, July 2, 2018 3:29 PM
> To: Xin Li (Talons) ; Xin Li 
> Cc: Andrew Cooper ; George Dunlap
> ; Ming Lu ; Sergey Dyasli
> ; Wei Liu ; Stefano Stabellini
> ; xen-de...@lists.xen.org; Konrad Rzeszutek Wilk
> ; Daniel de Graaf ; Tim
> (Xen.org) 
> Subject: RE: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
> 
> >>> On 02.07.18 at 08:57,  wrote:
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: Friday, June 29, 2018 6:36 PM
> >> >>> On 29.06.18 at 11:28,  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;
> 

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

[Jan Beulich]

>>> On 02.07.18 at 08:57,  wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: Friday, June 29, 2018 6:36 PM
>> >>> On 29.06.18 at 11:28,  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) 

Re: [Xen-devel] [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.gr...@arm.com]
> Sent: Friday, June 29, 2018 9:22 PM
> To: Xin Li ; xen-de...@lists.xen.org
> Cc: Sergey Dyasli ; Stefano Stabellini
> ; Wei Liu ; George Dunlap
> ; Xin Li (Talons) ; Tim (Xen.org)
> ; Jan Beulich ; Andrew Cooper
> ; Ming Lu ; Daniel De
> Graaf 
> 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 00..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: [Xen-devel] [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 ; xen-de...@lists.xen.org
> Cc: Xin Li (Talons) ; Daniel De Graaf
> ; George Dunlap ; Jan
> Beulich ; Konrad Rzeszutek Wilk
> ; Stefano Stabellini ; Tim
> (Xen.org) ; Wei Liu ; Sergey Dyasli
> ; Ming Lu 
> Subject: Re: [PATCH 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: [Xen-devel] [PATCH 2/2] xen/xsm: Add new SILO mode for XSM

[Xin Li (Talons)]

Hello Jan,

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Friday, June 29, 2018 6:36 PM
> To: Andrew Cooper ; Xin Li
> 
> Cc: Ming Lu ; Sergey Dyasli ;
> Wei Liu ; Xin Li (Talons) ; George
> Dunlap ; Stefano Stabellini
> ; xen-de...@lists.xen.org; Konrad Rzeszutek Wilk
> ; Daniel de Graaf ; Tim
> (Xen.org) 
> Subject: Re: [PATCH 2/2] xen/xsm: Add new SILO mode for XSM
> 
> >>> On 29.06.18 at 11:28,  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 .
> > + *
> > + * Copyright (c) 2018 Citrix Systems Ltd.
> > + */
> > +
> > +#include 
> > +#include 
> > +
> > +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 

Re: [Xen-devel] [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 00..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: [Xen-devel] [PATCH 2/2] xen/xsm: Add new SILO mode for XSM

[Jan Beulich]

>>> On 29.06.18 at 11:28,  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 .
> + *
> + * Copyright (c) 2018 Citrix Systems Ltd.
> + */
> +
> +#include 
> +#include 
> +
> +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: [Xen-devel] [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

[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 

---
CC: Daniel De Graaf 
CC: George Dunlap 
CC: Jan Beulich 
CC: Konrad Rzeszutek Wilk 
CC: Stefano Stabellini 
CC: Tim Deegan 
CC: Wei Liu 
CC: Sergey Dyasli 
CC: Andrew Cooper 
CC: Ming Lu 
---
 docs/misc/xen-command-line.markdown |   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 
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 00..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 .
+ *
+ * Copyright (c) 2018 Citrix Systems Ltd.
+ */
+
+#include 
+#include 
+
+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 (