Re: [Xen-devel] [PATCH] [RFC] xsm: add a default policy to .init.data

[Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx>]

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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel