| Message ID | 1465483638-9489-13-git-send-email-dgdegra@tycho.nsa.gov (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/06/16 15:47, Daniel De Graaf wrote: > Since FLASK is the only implementation of XSM hooks in Xen, using an > iterated initcall dispatch for setup is overly complex. Change this to > a direct function call to a globally visible function; if additional XSM > hooks are added in the future, a switching mechanism will be needed > regardless, and that can be placed in xsm_core.c. > > Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks for fixing the menuconfig bug; I was about to submit a fix myself. This is one case where ordering things alphabetically in the file not the highest priority.
>>> On 09.06.16 at 16:47, <dgdegra@tycho.nsa.gov> wrote: > Since FLASK is the only implementation of XSM hooks in Xen, using an > iterated initcall dispatch for setup is overly complex. Change this to > a direct function call to a globally visible function; if additional XSM > hooks are added in the future, a switching mechanism will be needed > regardless, and that can be placed in xsm_core.c. > > Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> While I agree with the Kconfig change, it not being mentioned at all in the description is confusing. Does it need to be part of this patch? Or can it rather be a separate one with a proper description? Jan
On 06/09/2016 12:11 PM, Jan Beulich wrote: >>>> On 09.06.16 at 16:47, <dgdegra@tycho.nsa.gov> wrote: >> Since FLASK is the only implementation of XSM hooks in Xen, using an >> iterated initcall dispatch for setup is overly complex. Change this to >> a direct function call to a globally visible function; if additional XSM >> hooks are added in the future, a switching mechanism will be needed >> regardless, and that can be placed in xsm_core.c. >> >> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> > > While I agree with the Kconfig change, it not being mentioned at > all in the description is confusing. Does it need to be part of this > patch? Or can it rather be a separate one with a proper description? > > Jan It does not need to be part of this patch; it just ended up here because the other Kconfig changes that I made were not useful and got dropped.
On Thu, Jun 09, 2016 at 10:47:15AM -0400, Daniel De Graaf wrote: > Since FLASK is the only implementation of XSM hooks in Xen, using an > iterated initcall dispatch for setup is overly complex. Change this to As such, should the Kconfig file enable the FLASK by default? Or make the XSM entry have the configuration for FLASK? Or perhaps make the FLASK be the visible one and select XSM? > a direct function call to a globally visible function; if additional XSM > hooks are added in the future, a switching mechanism will be needed > regardless, and that can be placed in xsm_core.c. > +config FLASK > + bool "FLux Advanced Security Kernel support" > + default y > + depends on XSM So this would be 'select XSM' ? > + ---help--- > + Enables FLASK (FLux Advanced Security Kernel) as the access control > + mechanism used by the XSM framework. This provides a mandatory access > + control framework by which security enforcement, isolation, and > + auditing can be achieved with fine granular control via a security > + policy. > + > + If unsure, say Y. > + > +config FLASK_AVC_STATS > + def_bool y > + depends on FLASK > + ---help--- > + Maintain statistics on the access vector cache > + > # Enable schedulers > menu "Schedulers" > visible if EXPERT = "y" > diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c > index 634ec98..cb2fdb6 100644 > --- a/xen/xsm/xsm_core.c > +++ b/xen/xsm/xsm_core.c > @@ -38,13 +38,7 @@ static inline int verify(struct xsm_operations *ops) > > static void __init do_xsm_initcalls(void) > { > - xsm_initcall_t *call; > - call = __xsm_initcall_start; > - while ( call < __xsm_initcall_end ) > - { > - (*call) (); > - call++; > - } > + flask_init(); Could you delete do_xsm_initcalls() and make xsm_init() call flask_init() ?
On 06/17/2016 11:50 AM, Konrad Rzeszutek Wilk wrote: > On Thu, Jun 09, 2016 at 10:47:15AM -0400, Daniel De Graaf wrote: >> Since FLASK is the only implementation of XSM hooks in Xen, using an >> iterated initcall dispatch for setup is overly complex. Change this to > > As such, should the Kconfig file enable the FLASK by default? > Or make the XSM entry have the configuration for FLASK? > > Or perhaps make the FLASK be the visible one and select XSM? XSM has previously been the configuration option to enable. If XSM is enabled (by choice), FLASK will then be enabled by default. Logically, FLASK is an implementation of XSM, and while it would be possible to swap them, this would probably need to be done by hiding the XSM option from the user. >> a direct function call to a globally visible function; if additional XSM >> hooks are added in the future, a switching mechanism will be needed >> regardless, and that can be placed in xsm_core.c. > >> +config FLASK >> + bool "FLux Advanced Security Kernel support" >> + default y >> + depends on XSM > > So this would be 'select XSM' ? >> + ---help--- >> + Enables FLASK (FLux Advanced Security Kernel) as the access control >> + mechanism used by the XSM framework. This provides a mandatory access >> + control framework by which security enforcement, isolation, and >> + auditing can be achieved with fine granular control via a security >> + policy. >> + >> + If unsure, say Y. >> + >> +config FLASK_AVC_STATS >> + def_bool y >> + depends on FLASK >> + ---help--- >> + Maintain statistics on the access vector cache >> + >> # Enable schedulers >> menu "Schedulers" >> visible if EXPERT = "y" >> diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c >> index 634ec98..cb2fdb6 100644 >> --- a/xen/xsm/xsm_core.c >> +++ b/xen/xsm/xsm_core.c >> @@ -38,13 +38,7 @@ static inline int verify(struct xsm_operations *ops) >> >> static void __init do_xsm_initcalls(void) >> { >> - xsm_initcall_t *call; >> - call = __xsm_initcall_start; >> - while ( call < __xsm_initcall_end ) >> - { >> - (*call) (); >> - call++; >> - } >> + flask_init(); > > Could you delete do_xsm_initcalls() and make xsm_init() call flask_init() ? Sure.
On Fri, Jun 17, 2016 at 01:04:01PM -0400, Daniel De Graaf wrote: > On 06/17/2016 11:50 AM, Konrad Rzeszutek Wilk wrote: > >On Thu, Jun 09, 2016 at 10:47:15AM -0400, Daniel De Graaf wrote: > >>Since FLASK is the only implementation of XSM hooks in Xen, using an > >>iterated initcall dispatch for setup is overly complex. Change this to > > > >As such, should the Kconfig file enable the FLASK by default? > >Or make the XSM entry have the configuration for FLASK? > > > >Or perhaps make the FLASK be the visible one and select XSM? > > XSM has previously been the configuration option to enable. If XSM is > enabled (by choice), FLASK will then be enabled by default. Ah, OK. I need to check, but could you disable FLASK and still have XSM enabled? > > Logically, FLASK is an implementation of XSM, and while it would be > possible to swap them, this would probably need to be done by hiding the > XSM option from the user. Right. > > >>a direct function call to a globally visible function; if additional XSM > >>hooks are added in the future, a switching mechanism will be needed > >>regardless, and that can be placed in xsm_core.c. > >
On 06/17/2016 01:14 PM, Konrad Rzeszutek Wilk wrote: > On Fri, Jun 17, 2016 at 01:04:01PM -0400, Daniel De Graaf wrote: >> On 06/17/2016 11:50 AM, Konrad Rzeszutek Wilk wrote: >>> On Thu, Jun 09, 2016 at 10:47:15AM -0400, Daniel De Graaf wrote: >>>> Since FLASK is the only implementation of XSM hooks in Xen, using an >>>> iterated initcall dispatch for setup is overly complex. Change this to >>> >>> As such, should the Kconfig file enable the FLASK by default? >>> Or make the XSM entry have the configuration for FLASK? >>> >>> Or perhaps make the FLASK be the visible one and select XSM? >> >> XSM has previously been the configuration option to enable. If XSM is >> enabled (by choice), FLASK will then be enabled by default. > > Ah, OK. I need to check, but could you disable FLASK and still have XSM enabled? Yes, but it won't do anything except slow down the hypervisor a bit. It's the same behavior as enabling flask and passing "flask=disabled" on the hypervisor command line. If someone wanted to write another XSM implementation (or resurrect the old ACM module), this would be the starting point for that.
On Fri, Jun 17, 2016 at 01:18:47PM -0400, Daniel De Graaf wrote: > On 06/17/2016 01:14 PM, Konrad Rzeszutek Wilk wrote: > >On Fri, Jun 17, 2016 at 01:04:01PM -0400, Daniel De Graaf wrote: > >>On 06/17/2016 11:50 AM, Konrad Rzeszutek Wilk wrote: > >>>On Thu, Jun 09, 2016 at 10:47:15AM -0400, Daniel De Graaf wrote: > >>>>Since FLASK is the only implementation of XSM hooks in Xen, using an > >>>>iterated initcall dispatch for setup is overly complex. Change this to > >>> > >>>As such, should the Kconfig file enable the FLASK by default? > >>>Or make the XSM entry have the configuration for FLASK? > >>> > >>>Or perhaps make the FLASK be the visible one and select XSM? > >> > >>XSM has previously been the configuration option to enable. If XSM is > >>enabled (by choice), FLASK will then be enabled by default. > > > >Ah, OK. I need to check, but could you disable FLASK and still have XSM enabled? > > Yes, but it won't do anything except slow down the hypervisor a bit. > It's the same behavior as enabling flask and passing "flask=disabled" on > the hypervisor command line. Right. Would it make sense to squash XSM and FLASK together in the Kconfig? > > If someone wanted to write another XSM implementation (or resurrect the > old ACM module), this would be the starting point for that. >
On 06/17/2016 01:21 PM, Konrad Rzeszutek Wilk wrote: > On Fri, Jun 17, 2016 at 01:18:47PM -0400, Daniel De Graaf wrote: >> On 06/17/2016 01:14 PM, Konrad Rzeszutek Wilk wrote: >>> On Fri, Jun 17, 2016 at 01:04:01PM -0400, Daniel De Graaf wrote: >>>> On 06/17/2016 11:50 AM, Konrad Rzeszutek Wilk wrote: >>>>> On Thu, Jun 09, 2016 at 10:47:15AM -0400, Daniel De Graaf wrote: >>>>>> Since FLASK is the only implementation of XSM hooks in Xen, using an >>>>>> iterated initcall dispatch for setup is overly complex. Change this to >>>>> >>>>> As such, should the Kconfig file enable the FLASK by default? >>>>> Or make the XSM entry have the configuration for FLASK? >>>>> >>>>> Or perhaps make the FLASK be the visible one and select XSM? >>>> >>>> XSM has previously been the configuration option to enable. If XSM is >>>> enabled (by choice), FLASK will then be enabled by default. >>> >>> Ah, OK. I need to check, but could you disable FLASK and still have XSM enabled? >> >> Yes, but it won't do anything except slow down the hypervisor a bit. >> It's the same behavior as enabling flask and passing "flask=disabled" on >> the hypervisor command line. > > Right. Would it make sense to squash XSM and FLASK together in the Kconfig? When making the Kconifg changes, I originally tried making a choice option for the XSM provider (like for scheduler), with FLASK and dummy being the two choices. I discarded that because it didn't add anything useful over the binary option. However, thinking about it, if it doesn't make sense to disable FLASK then hiding the option would simplify the configuration. It removes the grouping of flask-only options, but since there's only one that isn't all that important.
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index 76982b2..8320381 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -162,11 +162,6 @@ SECTIONS *(.initcall1.init) __initcall_end = .; } :text - .xsm_initcall.init : { - __xsm_initcall_start = .; - *(.xsm_initcall.init) - __xsm_initcall_end = .; - } :text __init_end_efi = .; . = ALIGN(STACK_SIZE); __init_end = .; diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index a43b29d..dcbb8fe 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -190,11 +190,6 @@ SECTIONS *(.initcall1.init) __initcall_end = .; } :text - .xsm_initcall.init : { - __xsm_initcall_start = .; - *(.xsm_initcall.init) - __xsm_initcall_end = .; - } :text . = ALIGN(PAGE_SIZE); __init_end = .; diff --git a/xen/common/Kconfig b/xen/common/Kconfig index cd59574..b8f1800 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -11,24 +11,6 @@ config COMPAT config CORE_PARKING bool -config FLASK - bool "FLux Advanced Security Kernel support" - default y - depends on XSM - ---help--- - Enables the FLASK (FLux Advanced Security Kernel) support which - provides a mandatory access control framework by which security - enforcement, isolation, and auditing can be achieved with fine - granular control via a security policy. - - If unsure, say N. - -config FLASK_AVC_STATS - def_bool y - depends on FLASK - ---help--- - Maintain statistics on the access vector cache - # Select HAS_DEVICE_TREE if device tree is supported config HAS_DEVICE_TREE bool @@ -137,6 +119,25 @@ config XSM If unsure, say N. +config FLASK + bool "FLux Advanced Security Kernel support" + default y + depends on XSM + ---help--- + Enables FLASK (FLux Advanced Security Kernel) as the access control + mechanism used by the XSM framework. This provides a mandatory access + control framework by which security enforcement, isolation, and + auditing can be achieved with fine granular control via a security + policy. + + If unsure, say Y. + +config FLASK_AVC_STATS + def_bool y + depends on FLASK + ---help--- + Maintain statistics on the access vector cache + # Enable schedulers menu "Schedulers" visible if EXPERT = "y" diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index 8ed8ee5..0d525ec 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -46,14 +46,6 @@ typedef enum xsm_default xsm_default_t; extern char *policy_buffer; extern u32 policy_size; -typedef void (*xsm_initcall_t)(void); - -extern xsm_initcall_t __xsm_initcall_start[], __xsm_initcall_end[]; - -#define xsm_initcall(fn) \ - static xsm_initcall_t __initcall_##fn \ - __used_section(".xsm_initcall.init") = fn - struct xsm_operations { void (*security_domaininfo) (struct domain *d, struct xen_domctl_getdomaininfo *info); @@ -763,6 +755,14 @@ extern int unregister_xsm(struct xsm_operations *ops); extern struct xsm_operations dummy_xsm_ops; extern void xsm_fixup_ops(struct xsm_operations *ops); +#ifdef CONFIG_FLASK +extern void flask_init(void); +#else +static inline void flask_init(void) +{ +} +#endif + #else /* CONFIG_XSM */ #include <xsm/dummy.h> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index 3ab3fbf..596ac0a 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -1816,7 +1816,7 @@ static struct xsm_operations flask_ops = { .xen_version = flask_xen_version, }; -static __init void flask_init(void) +__init void flask_init(void) { int ret = -ENOENT; @@ -1859,8 +1859,6 @@ static __init void flask_init(void) printk(XENLOG_INFO "Flask: Starting in permissive mode.\n"); } -xsm_initcall(flask_init); - /* * Local variables: * mode: C diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c index 634ec98..cb2fdb6 100644 --- a/xen/xsm/xsm_core.c +++ b/xen/xsm/xsm_core.c @@ -38,13 +38,7 @@ static inline int verify(struct xsm_operations *ops) static void __init do_xsm_initcalls(void) { - xsm_initcall_t *call; - call = __xsm_initcall_start; - while ( call < __xsm_initcall_end ) - { - (*call) (); - call++; - } + flask_init(); } static int __init xsm_core_init(void)
Since FLASK is the only implementation of XSM hooks in Xen, using an iterated initcall dispatch for setup is overly complex. Change this to a direct function call to a globally visible function; if additional XSM hooks are added in the future, a switching mechanism will be needed regardless, and that can be placed in xsm_core.c. Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> --- xen/arch/arm/xen.lds.S | 5 ----- xen/arch/x86/xen.lds.S | 5 ----- xen/common/Kconfig | 37 +++++++++++++++++++------------------ xen/include/xsm/xsm.h | 16 ++++++++-------- xen/xsm/flask/hooks.c | 4 +--- xen/xsm/xsm_core.c | 8 +------- 6 files changed, 29 insertions(+), 46 deletions(-)