diff mbox

[RFC] xsm: add a default policy to .init.data

Message ID 1464015089-25541-1-git-send-email-dgdegra@tycho.nsa.gov (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel De Graaf May 23, 2016, 2:51 p.m. UTC 
This includes the policy in tools/flask/policy in the hypervisor so that
the bootloader does not need to load a policy to get sane behavior from
an XSM-enabled hypervisor.

RFC because this adds a binding between xen's build and the tools build.
The inclusion of policy.o could be made conditional on a Kconfig option
(the code handles omission of the policy properly) to disable it.  ARM
build is also untested.

Moving the entire FLASK policy to live under the hypervisor would also
work, but this loses the ./configure support for detecting checkpolicy.
---
 xen/arch/arm/xen.lds.S |  4 ++++
 xen/arch/x86/xen.lds.S |  5 +++++
 xen/xsm/flask/Makefile | 21 +++++++++++++++++++++
 xen/xsm/xsm_core.c     | 12 ++++++++++++
 4 files changed, 42 insertions(+)

Comments

Wei Liu May 23, 2016, 3:08 p.m. UTC | #1 
On Mon, May 23, 2016 at 10:51:29AM -0400, Daniel De Graaf wrote:
> This includes the policy in tools/flask/policy in the hypervisor so that
> the bootloader does not need to load a policy to get sane behavior from
> an XSM-enabled hypervisor.
> 
> RFC because this adds a binding between xen's build and the tools build.
> The inclusion of policy.o could be made conditional on a Kconfig option
> (the code handles omission of the policy properly) to disable it.  ARM
> build is also untested.
> 
[...]
> +POLICY_SRC := $(XEN_ROOT)/tools/flask/policy/xenpolicy-$(XEN_FULLVERSION)

This (hypervisor build now depends on tools build) needs to be reflected
in Makfile target dependency if we're really going to do it. But I think
it would make sense to just move the policy directory to hypervisor
directory as well, leaving only flask/utils under tools.

Just my 2 cents.

Wei.
Andrew Cooper May 23, 2016, 3:25 p.m. UTC | #2 
On 23/05/16 15:51, Daniel De Graaf wrote:
> diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
> index 634ec98..af1d86f 100644
> --- a/xen/xsm/xsm_core.c
> +++ b/xen/xsm/xsm_core.c
> @@ -47,6 +47,17 @@ static void __init do_xsm_initcalls(void)
>      }
>  }
>  
> +extern char __xsm_init_policy_start[], __xsm_init_policy_end[];
> +
> +static void __init xsm_policy_init(void)
> +{
> +    if ( policy_size == 0 )
> +    {
> +        policy_buffer = __xsm_init_policy_start;
> +        policy_size = __xsm_init_policy_end - __xsm_init_policy_start;
> +    }

Logic like this is slightly problematic if there is no policy.

With these changes, we presumably always expect to have an embedded policy.

It would be cleaner to have a linker ASSERT(__xsm_init_policy_start !=
__xsm_init_policy_end) to guarentee that something is present, at which
point policy_buffer can unilaterally point at __xsm_init_policy_start,
and size can be initialised to __xsm_init_policy_end -
__xsm_init_policy_start.

~Andrew
Daniel De Graaf May 23, 2016, 3:32 p.m. UTC | #3 
On 05/23/2016 11:25 AM, Andrew Cooper wrote:
> On 23/05/16 15:51, Daniel De Graaf wrote:
>> diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
>> index 634ec98..af1d86f 100644
>> --- a/xen/xsm/xsm_core.c
>> +++ b/xen/xsm/xsm_core.c
>> @@ -47,6 +47,17 @@ static void __init do_xsm_initcalls(void)
>>       }
>>   }
>>
>> +extern char __xsm_init_policy_start[], __xsm_init_policy_end[];
>> +
>> +static void __init xsm_policy_init(void)
>> +{
>> +    if ( policy_size == 0 )
>> +    {
>> +        policy_buffer = __xsm_init_policy_start;
>> +        policy_size = __xsm_init_policy_end - __xsm_init_policy_start;
>> +    }
>
> Logic like this is slightly problematic if there is no policy.
>
> With these changes, we presumably always expect to have an embedded policy.

People who already have the policy specified in the bootloader may want
to omit the built-in policy.  I'm not sure that this should be excluded
completely, although this patch doesn't support it (it would require the
Kconfig option I mentioned).

> It would be cleaner to have a linker ASSERT(__xsm_init_policy_start !=
> __xsm_init_policy_end) to guarentee that something is present, at which
> point policy_buffer can unilaterally point at __xsm_init_policy_start,
> and size can be initialised to __xsm_init_policy_end -
> __xsm_init_policy_start.

No, because this would break the ability to specify a policy module in
GRUB.  If there is no built-in policy present, this code will work
correctly: policy_size will remain set to zero, and that is the
condition checked (in flask_init) when the policy itself is used.
Jan Beulich May 23, 2016, 3:34 p.m. UTC | #4 
>>> On 23.05.16 at 16:51, <dgdegra@tycho.nsa.gov> wrote:
> --- a/xen/xsm/flask/Makefile
> +++ b/xen/xsm/flask/Makefile
> @@ -27,6 +27,27 @@ $(FLASK_H_FILES): $(FLASK_H_DEPEND)
>  $(AV_H_FILES): $(AV_H_DEPEND)
>  	$(CONFIG_SHELL) policy/mkaccess_vector.sh $(AWK) $(AV_H_DEPEND)
>  
> +obj-y += policy.o
> +
> +ifeq ($(XEN_TARGET_ARCH),x86_64)
> +    OBJCOPY_ARGS := -I binary -O elf64-x86-64 -B i386:x86-64
> +else ifeq ($(XEN_TARGET_ARCH),arm32)
> +    OBJCOPY_ARGS := -I binary -O elf32-littlearm -B arm
> +else ifeq ($(XEN_TARGET_ARCH),arm64)
> +    OBJCOPY_ARGS := -I binary -O elf64-littleaarch64 -B aarch64
> +else
> +    $(error "Unknown XEN_TARGET_ARCH: $(XEN_TARGET_ARCH)")
> +endif

As this is kind of ugly - did you try whether binutils can be talked
into generating an architecture neutral ELF object (using EM_NONE
as the architecture in the ELF header), and whether that could
then be linked? Of course that would be of limited use of the blob
was other than a plain stream of bytes (i.e. endian independent).

Jan
Daniel De Graaf May 23, 2016, 4 p.m. UTC | #5 
On 05/23/2016 11:34 AM, Jan Beulich wrote:
>>>> On 23.05.16 at 16:51, <dgdegra@tycho.nsa.gov> wrote:
>> --- a/xen/xsm/flask/Makefile
>> +++ b/xen/xsm/flask/Makefile
>> @@ -27,6 +27,27 @@ $(FLASK_H_FILES): $(FLASK_H_DEPEND)
>>   $(AV_H_FILES): $(AV_H_DEPEND)
>>   	$(CONFIG_SHELL) policy/mkaccess_vector.sh $(AWK) $(AV_H_DEPEND)
>>
>> +obj-y += policy.o
>> +
>> +ifeq ($(XEN_TARGET_ARCH),x86_64)
>> +    OBJCOPY_ARGS := -I binary -O elf64-x86-64 -B i386:x86-64
>> +else ifeq ($(XEN_TARGET_ARCH),arm32)
>> +    OBJCOPY_ARGS := -I binary -O elf32-littlearm -B arm
>> +else ifeq ($(XEN_TARGET_ARCH),arm64)
>> +    OBJCOPY_ARGS := -I binary -O elf64-littleaarch64 -B aarch64
>> +else
>> +    $(error "Unknown XEN_TARGET_ARCH: $(XEN_TARGET_ARCH)")
>> +endif
>
> As this is kind of ugly - did you try whether binutils can be talked
> into generating an architecture neutral ELF object (using EM_NONE
> as the architecture in the ELF header), and whether that could
> then be linked? Of course that would be of limited use of the blob
> was other than a plain stream of bytes (i.e. endian independent).
>
> Jan

You get EM_NONE when you omit the -B argument, but the linker refuses
to accept this as an input unless passed --accept-unknown-input-arch.
With this flag enabled, the built_in.o is binary equal.
Konrad Rzeszutek Wilk June 7, 2016, 8:19 p.m. UTC | #6 
On Mon, May 23, 2016 at 10:51:29AM -0400, Daniel De Graaf wrote:
> This includes the policy in tools/flask/policy in the hypervisor so that
> the bootloader does not need to load a policy to get sane behavior from
> an XSM-enabled hypervisor.
> 
> RFC because this adds a binding between xen's build and the tools build.
> The inclusion of policy.o could be made conditional on a Kconfig option
> (the code handles omission of the policy properly) to disable it.  ARM

And probably also a document update. To mention that the if you have
an policy built-in, you can always over-write if if you include
the policy as the last multiboot argument?

> build is also untested.
> 
> Moving the entire FLASK policy to live under the hypervisor would also
> work, but this loses the ./configure support for detecting checkpolicy.

You could do a check for checkpolicy existing like the ld-ver-build-id
does in the ./Config.mk - which then exports XEN_HAS_BUILD_ID=y.

Similary do the check and then export CHECKPOLICY=y ?

> ---
>  xen/arch/arm/xen.lds.S |  4 ++++
>  xen/arch/x86/xen.lds.S |  5 +++++
>  xen/xsm/flask/Makefile | 21 +++++++++++++++++++++
>  xen/xsm/xsm_core.c     | 12 ++++++++++++
>  4 files changed, 42 insertions(+)
> 
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 1f010bd..61dd278 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 b14bcd2..004c55f 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/xsm/flask/Makefile b/xen/xsm/flask/Makefile
> index 12fc3a9..16c9474 100644
> --- a/xen/xsm/flask/Makefile
> +++ b/xen/xsm/flask/Makefile
> @@ -27,6 +27,27 @@ $(FLASK_H_FILES): $(FLASK_H_DEPEND)
>  $(AV_H_FILES): $(AV_H_DEPEND)
>  	$(CONFIG_SHELL) policy/mkaccess_vector.sh $(AWK) $(AV_H_DEPEND)
>  
> +obj-y += policy.o
> +
> +ifeq ($(XEN_TARGET_ARCH),x86_64)
> +    OBJCOPY_ARGS := -I binary -O elf64-x86-64 -B i386:x86-64
> +else ifeq ($(XEN_TARGET_ARCH),arm32)
> +    OBJCOPY_ARGS := -I binary -O elf32-littlearm -B arm
> +else ifeq ($(XEN_TARGET_ARCH),arm64)
> +    OBJCOPY_ARGS := -I binary -O elf64-littleaarch64 -B aarch64
> +else
> +    $(error "Unknown XEN_TARGET_ARCH: $(XEN_TARGET_ARCH)")
> +endif
> +
> +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) $(OBJCOPY_ARGS) --rename-section=.data=.init.xsm_policy policy.bin $@
> +
>  .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 634ec98..af1d86f 100644
> --- a/xen/xsm/xsm_core.c
> +++ b/xen/xsm/xsm_core.c
> @@ -47,6 +47,17 @@ static void __init do_xsm_initcalls(void)
>      }
>  }
>  
> +extern char __xsm_init_policy_start[], __xsm_init_policy_end[];
> +
> +static void __init xsm_policy_init(void)
> +{
> +    if ( policy_size == 0 )
> +    {
> +        policy_buffer = __xsm_init_policy_start;
> +        policy_size = __xsm_init_policy_end - __xsm_init_policy_start;
> +    }

If there are no XSM built (and policy_size is zero), do you need to
set policy_buffer to NULL? I guess it does not hurt as
xsm_multiboot_init had already been called and didn't set policy_size.

And all code checks policy_size and ignores policy_buffer. But maybe
if somebody in the future redoes this code it may be good idea to
just set it to NULL? Or do something like:

	if ( !policy_size )
	{
		policy_size = __xsm_init_policy_end - __xsm_init_policy_start;
		if ( policy_size )
			policy_buffer = __xsm_init_policy_start;
	}
?


> +}
> +
>  static int __init xsm_core_init(void)
>  {
>      if ( verify(&dummy_xsm_ops) )
> @@ -57,6 +68,7 @@ static int __init xsm_core_init(void)
>      }
>  
>      xsm_ops = &dummy_xsm_ops;
> +    xsm_policy_init();
>      do_xsm_initcalls();
>  
>      return 0;
> -- 
> 2.5.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
diff mbox

Patch

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 1f010bd..61dd278 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 b14bcd2..004c55f 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/xsm/flask/Makefile b/xen/xsm/flask/Makefile
index 12fc3a9..16c9474 100644
--- a/xen/xsm/flask/Makefile
+++ b/xen/xsm/flask/Makefile
@@ -27,6 +27,27 @@  $(FLASK_H_FILES): $(FLASK_H_DEPEND)
 $(AV_H_FILES): $(AV_H_DEPEND)
 	$(CONFIG_SHELL) policy/mkaccess_vector.sh $(AWK) $(AV_H_DEPEND)
 
+obj-y += policy.o
+
+ifeq ($(XEN_TARGET_ARCH),x86_64)
+    OBJCOPY_ARGS := -I binary -O elf64-x86-64 -B i386:x86-64
+else ifeq ($(XEN_TARGET_ARCH),arm32)
+    OBJCOPY_ARGS := -I binary -O elf32-littlearm -B arm
+else ifeq ($(XEN_TARGET_ARCH),arm64)
+    OBJCOPY_ARGS := -I binary -O elf64-littleaarch64 -B aarch64
+else
+    $(error "Unknown XEN_TARGET_ARCH: $(XEN_TARGET_ARCH)")
+endif
+
+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) $(OBJCOPY_ARGS) --rename-section=.data=.init.xsm_policy policy.bin $@
+
 .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 634ec98..af1d86f 100644
--- a/xen/xsm/xsm_core.c
+++ b/xen/xsm/xsm_core.c
@@ -47,6 +47,17 @@  static void __init do_xsm_initcalls(void)
     }
 }
 
+extern char __xsm_init_policy_start[], __xsm_init_policy_end[];
+
+static void __init xsm_policy_init(void)
+{
+    if ( policy_size == 0 )
+    {
+        policy_buffer = __xsm_init_policy_start;
+        policy_size = __xsm_init_policy_end - __xsm_init_policy_start;
+    }
+}
+
 static int __init xsm_core_init(void)
 {
     if ( verify(&dummy_xsm_ops) )
@@ -57,6 +68,7 @@  static int __init xsm_core_init(void)
     }
 
     xsm_ops = &dummy_xsm_ops;
+    xsm_policy_init();
     do_xsm_initcalls();
 
     return 0;