| Message ID | 1465483638-9489-16-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: > diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c > index 4a264c2..6ffccb2 100644 > --- a/xen/xsm/xsm_core.c > +++ b/xen/xsm/xsm_core.c > @@ -36,6 +36,17 @@ static inline int verify(struct xsm_operations *ops) > return 0; > } > > +extern char __xsm_init_policy_start[], __xsm_init_policy_end[]; > + > +static void __init xsm_policy_init(void) > +{ > + if ( policy_size == 0 && __xsm_init_policy_end != __xsm_init_policy_start ) > + { > + policy_buffer = __xsm_init_policy_start; I think this might cause a problem for ARM. xsm_dt_init(void) does ret = xsm_core_init(); xfree(policy_buffer); which might now try freeing a piece of initdata. Even going back to c/s a8f2fb51c19 which introduced this code, I can't spot its justification. It also looks like the entire policy buffer can be const, at which point it can be linked slightly higher in .init.data with the other logically-const data. ~Andrew
>>> On 09.06.16 at 16:47, <dgdegra@tycho.nsa.gov> wrote: > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -132,6 +132,23 @@ config FLASK > > If unsure, say Y. > > +config XSM_POLICY > + bool "Compile Xen with a built-in security policy" > + default y > + depends on XSM > + ---help--- > + This includes a default XSM policy in the hypervisor so that the > + bootloader does not need to load a policy to get sane behavior from an > + XSM-enabled hypervisor. If this is disabled, a policy must be > + provided by the bootloader or by Domain 0. Even if this is enabled, a > + policy provided by the bootloader will override it. > + > + This requires that the SELinux policy compiler (checkpolicy) be > + available when compiling the hypervisor; if this tool is not found, no > + policy will be added. > + > + If unsure, say Y. > + > config FLASK_AVC_STATS > def_bool y > depends on FLASK Placing this between FLASK and FLASK_AVC_STATS will break proper menuconfig representation of the latter afaict. Jan
On 06/09/2016 12:15 PM, Jan Beulich wrote: >>>> On 09.06.16 at 16:47, <dgdegra@tycho.nsa.gov> wrote: >> --- a/xen/common/Kconfig >> +++ b/xen/common/Kconfig >> @@ -132,6 +132,23 @@ config FLASK >> >> If unsure, say Y. >> >> +config XSM_POLICY >> + bool "Compile Xen with a built-in security policy" >> + default y >> + depends on XSM >> + ---help--- >> + This includes a default XSM policy in the hypervisor so that the >> + bootloader does not need to load a policy to get sane behavior from an >> + XSM-enabled hypervisor. If this is disabled, a policy must be >> + provided by the bootloader or by Domain 0. Even if this is enabled, a >> + policy provided by the bootloader will override it. >> + >> + This requires that the SELinux policy compiler (checkpolicy) be >> + available when compiling the hypervisor; if this tool is not found, no >> + policy will be added. >> + >> + If unsure, say Y. >> + >> config FLASK_AVC_STATS >> def_bool y >> depends on FLASK > > Placing this between FLASK and FLASK_AVC_STATS will break proper > menuconfig representation of the latter afaict. > > Jan This option isn't visible in menuconfig. Should I make it visible?
On 06/09/2016 11:30 AM, Andrew Cooper wrote: > On 09/06/16 15:47, Daniel De Graaf wrote: >> diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c >> index 4a264c2..6ffccb2 100644 >> --- a/xen/xsm/xsm_core.c >> +++ b/xen/xsm/xsm_core.c >> @@ -36,6 +36,17 @@ static inline int verify(struct xsm_operations *ops) >> return 0; >> } >> >> +extern char __xsm_init_policy_start[], __xsm_init_policy_end[]; >> + >> +static void __init xsm_policy_init(void) >> +{ >> + if ( policy_size == 0 && __xsm_init_policy_end != __xsm_init_policy_start ) >> + { >> + policy_buffer = __xsm_init_policy_start; > > I think this might cause a problem for ARM. > > xsm_dt_init(void) does > > ret = xsm_core_init(); > xfree(policy_buffer); > > which might now try freeing a piece of initdata. Even going back to c/s > a8f2fb51c19 which introduced this code, I can't spot its justification. The buffer is allocated and populated in xsm_dt_policy_init. > It also looks like the entire policy buffer can be const, at which point > it can be linked slightly higher in .init.data with the other > logically-const data. Sure, although this will require quite a bit of const propagation down into the FLASK security server (or casting it away).
On 6/9/16 11:53 AM, Daniel De Graaf wrote: > On 06/09/2016 12:15 PM, Jan Beulich wrote: >>>>> On 09.06.16 at 16:47, <dgdegra@tycho.nsa.gov> wrote: >>> --- a/xen/common/Kconfig >>> +++ b/xen/common/Kconfig >>> @@ -132,6 +132,23 @@ config FLASK >>> >>> If unsure, say Y. >>> >>> +config XSM_POLICY >>> + bool "Compile Xen with a built-in security policy" >>> + default y >>> + depends on XSM >>> + ---help--- >>> + This includes a default XSM policy in the hypervisor so that the >>> + bootloader does not need to load a policy to get sane behavior >>> from an >>> + XSM-enabled hypervisor. If this is disabled, a policy must be >>> + provided by the bootloader or by Domain 0. Even if this is >>> enabled, a >>> + policy provided by the bootloader will override it. >>> + >>> + This requires that the SELinux policy compiler (checkpolicy) be >>> + available when compiling the hypervisor; if this tool is not >>> found, no >>> + policy will be added. >>> + >>> + If unsure, say Y. >>> + >>> config FLASK_AVC_STATS >>> def_bool y >>> depends on FLASK >> >> Placing this between FLASK and FLASK_AVC_STATS will break proper >> menuconfig representation of the latter afaict. >> >> Jan > > This option isn't visible in menuconfig. Should I make it visible? > I believe I actually had that as an outstanding question to you on the series that introduced that flag.
>>> On 09.06.16 at 18:53, <dgdegra@tycho.nsa.gov> wrote: > On 06/09/2016 12:15 PM, Jan Beulich wrote: >>>>> On 09.06.16 at 16:47, <dgdegra@tycho.nsa.gov> wrote: >>> --- a/xen/common/Kconfig >>> +++ b/xen/common/Kconfig >>> @@ -132,6 +132,23 @@ config FLASK >>> >>> If unsure, say Y. >>> >>> +config XSM_POLICY >>> + bool "Compile Xen with a built-in security policy" >>> + default y >>> + depends on XSM >>> + ---help--- >>> + This includes a default XSM policy in the hypervisor so that the >>> + bootloader does not need to load a policy to get sane behavior from an >>> + XSM-enabled hypervisor. If this is disabled, a policy must be >>> + provided by the bootloader or by Domain 0. Even if this is enabled, a >>> + policy provided by the bootloader will override it. >>> + >>> + This requires that the SELinux policy compiler (checkpolicy) be >>> + available when compiling the hypervisor; if this tool is not found, no >>> + policy will be added. >>> + >>> + If unsure, say Y. >>> + >>> config FLASK_AVC_STATS >>> def_bool y >>> depends on FLASK >> >> Placing this between FLASK and FLASK_AVC_STATS will break proper >> menuconfig representation of the latter afaict. > > This option isn't visible in menuconfig. Should I make it visible? Ah, true. No, it shouldn't become visible now, but since it may become visible eventually I'd still prefer if the ordering was done as if all options having some dependency also had a prompt (reducing future code churn). Jan
>>> On 09.06.16 at 18:58, <dgdegra@tycho.nsa.gov> wrote: > On 06/09/2016 11:30 AM, Andrew Cooper wrote: >> On 09/06/16 15:47, Daniel De Graaf wrote: >>> diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c >>> index 4a264c2..6ffccb2 100644 >>> --- a/xen/xsm/xsm_core.c >>> +++ b/xen/xsm/xsm_core.c >>> @@ -36,6 +36,17 @@ static inline int verify(struct xsm_operations *ops) >>> return 0; >>> } >>> >>> +extern char __xsm_init_policy_start[], __xsm_init_policy_end[]; >>> + >>> +static void __init xsm_policy_init(void) >>> +{ >>> + if ( policy_size == 0 && __xsm_init_policy_end != __xsm_init_policy_start ) >>> + { >>> + policy_buffer = __xsm_init_policy_start; >> >> I think this might cause a problem for ARM. >> >> xsm_dt_init(void) does >> >> ret = xsm_core_init(); >> xfree(policy_buffer); >> >> which might now try freeing a piece of initdata. Even going back to c/s >> a8f2fb51c19 which introduced this code, I can't spot its justification. > > The buffer is allocated and populated in xsm_dt_policy_init. Only if there is an XSM module; I think Andrew refers to the assignment to policy_buffer still visible above, which would remain untouched by xsm_dt_policy_init() if bailing early. >> It also looks like the entire policy buffer can be const, at which point >> it can be linked slightly higher in .init.data with the other >> logically-const data. > > Sure, although this will require quite a bit of const propagation down into > the FLASK security server (or casting it away). No casting away of constness please, except in very special situations (which this one pretty clearly isn't). Jan
On 06/09/2016 05:54 PM, Doug Goldstein wrote: > On 6/9/16 11:53 AM, Daniel De Graaf wrote: >> On 06/09/2016 12:15 PM, Jan Beulich wrote: >>>>>> On 09.06.16 at 16:47, <dgdegra@tycho.nsa.gov> wrote: >>>> --- a/xen/common/Kconfig >>>> +++ b/xen/common/Kconfig >>>> @@ -132,6 +132,23 @@ config FLASK >>>> >>>> If unsure, say Y. >>>> >>>> +config XSM_POLICY >>>> + bool "Compile Xen with a built-in security policy" >>>> + default y >>>> + depends on XSM >>>> + ---help--- >>>> + This includes a default XSM policy in the hypervisor so that the >>>> + bootloader does not need to load a policy to get sane behavior >>>> from an >>>> + XSM-enabled hypervisor. If this is disabled, a policy must be >>>> + provided by the bootloader or by Domain 0. Even if this is >>>> enabled, a >>>> + policy provided by the bootloader will override it. >>>> + >>>> + This requires that the SELinux policy compiler (checkpolicy) be >>>> + available when compiling the hypervisor; if this tool is not >>>> found, no >>>> + policy will be added. >>>> + >>>> + If unsure, say Y. >>>> + >>>> config FLASK_AVC_STATS >>>> def_bool y >>>> depends on FLASK >>> >>> Placing this between FLASK and FLASK_AVC_STATS will break proper >>> menuconfig representation of the latter afaict. >>> >>> Jan >> >> This option isn't visible in menuconfig. Should I make it visible? >> > > I believe I actually had that as an outstanding question to you on the > series that introduced that flag. At the time I didn't see the need for it to be visible. Since it's come up again, I think it should either be made visible (in a distinct patch), but maybe limited to EXPERT=y. Otherwise, it seems like the option and its #ifdefs should be removed: there's no point in having the option if it's not possible to adjust it.
On Thu, Jun 09, 2016 at 10:47:18AM -0400, Daniel De Graaf wrote: > This adds a Kconfig option and support for including the XSM policy from > tools/flask/policy in the hypervisor so that the bootloader does not > need to provide a policy to get sane behavior from an XSM-enabled > hypervisor. The policy provided by the bootloader, if present, will > override the built-in policy. > > Enabling this option only builds the policy if checkpolicy is available > during compilation of the hypervisor; otherwise, it does nothing. The > XSM policy is not moved out of tools because that remains the primary > location for installing and configuring the policy. > > Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Thanks!
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown index fed732c..c85d1dc 100644 --- a/docs/misc/xen-command-line.markdown +++ b/docs/misc/xen-command-line.markdown @@ -704,13 +704,15 @@ enabled by running either: with untrusted guests. If a policy is provided by the bootloader, it will be loaded; errors will be reported to the ring buffer but will not prevent booting. The policy can be changed to enforcing mode using "xl setenforce". -* `enforcing`: This requires a security policy to be provided by the bootloader - and will enter enforcing mode prior to the creation of domain 0. If a valid - policy is not provided, the hypervisor will not continue booting. -* `late`: This disables loading of the security policy from the bootloader. - FLASK will be enabled but will not enforce access controls until a policy is - loaded by a domain using "xl loadpolicy". Once a policy is loaded, FLASK will - run in enforcing mode unless "xl setenforce" has changed that setting. +* `enforcing`: This will cause the security server to enter enforcing mode prior + to the creation of domain 0. If an valid policy is not provided by the + bootloader and no built-in policy is present, the hypervisor will not continue + booting. +* `late`: This disables loading of the built-in security policy or the policy + provided by the bootloader. FLASK will be enabled but will not enforce access + controls until a policy is loaded by a domain using "xl loadpolicy". Once a + policy is loaded, FLASK will run in enforcing mode unless "xl setenforce" has + changed that setting. * `disabled`: This causes the XSM framework to revert to the dummy module. The dummy module provides the same security policy as is used when compiling the hypervisor without support for XSM. The xsm\_op hypercall can also be used to diff --git a/docs/misc/xsm-flask.txt b/docs/misc/xsm-flask.txt index 2f42585..62f15dd 100644 --- a/docs/misc/xsm-flask.txt +++ b/docs/misc/xsm-flask.txt @@ -141,21 +141,21 @@ only type enforcement is used and the user and role are set to system_u and system_r for all domains. The FLASK security framework is mostly configured using a security policy file. -This policy file is not normally generated during the Xen build process because -it relies on the SELinux compiler "checkpolicy"; run - - make -C tools/flask/policy - -to compile the example policy included with Xen. The policy is generated from -definition files under this directory. Most changes to security policy will -involve creating or modifying modules found in tools/flask/policy/modules/. The -modules.conf file there defines what modules are enabled and has short -descriptions of each module. - -The XSM policy file needs to be copied to /boot and loaded as a module by grub. -The exact position of the module does not matter as long as it is after the Xen -kernel; it is normally placed either just above the dom0 kernel or at the end. -Once dom0 is running, the policy can be reloaded using "xl loadpolicy". +It relies on the SELinux compiler "checkpolicy"; if this is available, the +policy will be compiled as part of the tools build. If hypervisor support for a +built-in policy is enabled ("Compile Xen with a built-in security policy"), the +policy will be built during the hypervisor build. + +The policy is generated from definition files in tools/flask/policy. Most +changes to security policy will involve creating or modifying modules found in +tools/flask/policy/modules/. The modules.conf file there defines what modules +are enabled and has short descriptions of each module. + +If not using the built-in policy, the XSM policy file needs to be copied to +/boot and loaded as a module by grub. The exact position and filename of the +module does not matter as long as it is after the Xen kernel; it is normally +placed either just above the dom0 kernel or at the end. Once dom0 is running, +the policy can be reloaded using "xl loadpolicy". The example policy included with Xen demonstrates most of the features of FLASK that can be used without dom0 disaggregation. The main types for domUs are: diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index 8320381..80c2299 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -139,6 +139,10 @@ SECTIONS *(.init.data.rel) *(.init.data.rel.*) + __xsm_init_policy_start = .; + *(.init.xsm_policy) + __xsm_init_policy_end = .; + . = ALIGN(8); __ctors_start = .; *(.init_array) diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index dcbb8fe..9072e1c 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -155,6 +155,11 @@ SECTIONS *(.init.data) *(.init.data.rel) *(.init.data.rel.*) + + __xsm_init_policy_start = .; + *(.init.xsm_policy) + __xsm_init_policy_end = .; + . = ALIGN(4); __trampoline_rel_start = .; *(.trampoline_rel) diff --git a/xen/common/Kconfig b/xen/common/Kconfig index b8f1800..ae489a7 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -132,6 +132,23 @@ config FLASK If unsure, say Y. +config XSM_POLICY + bool "Compile Xen with a built-in security policy" + default y + depends on XSM + ---help--- + This includes a default XSM policy in the hypervisor so that the + bootloader does not need to load a policy to get sane behavior from an + XSM-enabled hypervisor. If this is disabled, a policy must be + provided by the bootloader or by Domain 0. Even if this is enabled, a + policy provided by the bootloader will override it. + + This requires that the SELinux policy compiler (checkpolicy) be + available when compiling the hypervisor; if this tool is not found, no + policy will be added. + + If unsure, say Y. + config FLASK_AVC_STATS def_bool y depends on FLASK diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile index 12fc3a9..eefd37c 100644 --- a/xen/xsm/flask/Makefile +++ b/xen/xsm/flask/Makefile @@ -27,6 +27,23 @@ $(FLASK_H_FILES): $(FLASK_H_DEPEND) $(AV_H_FILES): $(AV_H_DEPEND) $(CONFIG_SHELL) policy/mkaccess_vector.sh $(AWK) $(AV_H_DEPEND) +ifeq ($(CONFIG_XSM_POLICY),y) +HAS_CHECKPOLICY := $(shell checkpolicy -h 2>&1 | grep -q xen && echo y || echo n) + +obj-$(HAS_CHECKPOLICY) += policy.o +endif + +LDFLAGS += --accept-unknown-input-arch + +POLICY_SRC := $(XEN_ROOT)/tools/flask/policy/xenpolicy-$(XEN_FULLVERSION) + +policy.bin: FORCE + $(MAKE) -C $(XEN_ROOT)/tools/flask/policy + cmp -s $(POLICY_SRC) $@ || cp $(POLICY_SRC) $@ + +policy.o: policy.bin + $(OBJCOPY) -S -I binary -O elf64-little --rename-section=.data=.init.xsm_policy $< $@ + .PHONY: clean clean:: rm -f $(ALL_H_FILES) *.o $(DEPS) diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c index 4a264c2..6ffccb2 100644 --- a/xen/xsm/xsm_core.c +++ b/xen/xsm/xsm_core.c @@ -36,6 +36,17 @@ static inline int verify(struct xsm_operations *ops) return 0; } +extern char __xsm_init_policy_start[], __xsm_init_policy_end[]; + +static void __init xsm_policy_init(void) +{ + if ( policy_size == 0 && __xsm_init_policy_end != __xsm_init_policy_start ) + { + policy_buffer = __xsm_init_policy_start; + policy_size = __xsm_init_policy_end - __xsm_init_policy_start; + } +} + static void __init do_xsm_initcalls(void) { flask_init(); @@ -51,6 +62,7 @@ static int __init xsm_core_init(void) } xsm_ops = &dummy_xsm_ops; + xsm_policy_init(); do_xsm_initcalls(); return 0;
This adds a Kconfig option and support for including the XSM policy from tools/flask/policy in the hypervisor so that the bootloader does not need to provide a policy to get sane behavior from an XSM-enabled hypervisor. The policy provided by the bootloader, if present, will override the built-in policy. Enabling this option only builds the policy if checkpolicy is available during compilation of the hypervisor; otherwise, it does nothing. The XSM policy is not moved out of tools because that remains the primary location for installing and configuring the policy. Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> --- docs/misc/xen-command-line.markdown | 16 +++++++++------- docs/misc/xsm-flask.txt | 30 +++++++++++++++--------------- xen/arch/arm/xen.lds.S | 4 ++++ xen/arch/x86/xen.lds.S | 5 +++++ xen/common/Kconfig | 17 +++++++++++++++++ xen/xsm/flask/Makefile | 17 +++++++++++++++++ xen/xsm/xsm_core.c | 12 ++++++++++++ 7 files changed, 79 insertions(+), 22 deletions(-)