Re: [PATCH v3] xen/arm64: check XSM Magic and Signature from the second unknown module.

[Julien Grall <julien.grall@arm.com>]

(Use Stefano's new e-mail address)
Hi Fu Wei,

On 29/03/16 04:26, fu.wei@linaro.org wrote:
> From: Fu Wei <fu.wei@linaro.org>
>
> This patch add a check_xsm_signature static function for detecting XSM

s/add/adds/

> from the second unknown module.
>
> If Xen can't get the kind of module from compatible, we guess the kind of
> these first two unknown respectively:

The steps below are not only for the first two modules.

>      (1) The first unknown must be kernel;
>      (2) The second unknown is ramdisk, only if we have ramdisk;

This is unclear.

>      (3) Start from the 2nd unknown, detect the XSM binary signature;
>      (4) If we got XSM in the 2nd unknown, that means we don't load initrd.

s/initrd/ramdisk/

Also, the documentation in misc/arm/device-tree/booting.txt needs to be 
updated.

ARM behavior will be compatible to x86 and will simplify multi-arch 
bootloader such as GRUB, so I'm fine to introduce this boot protocol 
change. However, I'd like to see the reason of this change spells out in 
the commit message.

>
> Signed-off-by: Fu Wei <fu.wei@linaro.org>
> ---
> Changelog:
> v3: Using memcmp instead of strncmp.
>      Using "return 0;" instead of panic();
>      Improve some comments.
>
> v2: http://lists.xen.org/archives/html/xen-devel/2016-03/msg03543.html
>      Using XEN_MAGIC macro instead of 0xf97cff8c :
>      uint32_t selinux_magic = 0xf97cff8c; --> uint32_t xen_magic = XEN_MAGIC;
>      Comment out the code(return 0 directly), if CONFIG_FLASK is not set.
>
> v1: http://lists.xen.org/archives/html/xen-devel/2016-03/msg02430.html
>      The first upstream patch to xen-devel mailing lists.
>
>   xen/arch/arm/bootfdt.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 8a14015..10d3382 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -163,6 +163,49 @@ static void __init process_memory_node(const void *fdt, int node,
>       }
>   }
>
> +/**
> + * check_xsm_signature - Check XSM Magic and Signature of the module header
> + * A XSM module has a special header
> + * ------------------------------------------------
> + * uint magic | uint target_len | uchar target[8] |
> + * 0xf97cff8c |        8        |    "XenFlask"   |
> + * ------------------------------------------------
> + * 0xf97cff8c is policy magic number (XSM_MAGIC).
> + * So we only read the first 16 bytes of the module, then check these three
> + * parts. This checking (memcmp) assumes little-endian byte order.
> + */
> +static bool __init check_xsm_signature(const void *fdt, int node,
> +                                       const char *name,
> +                                       u32 address_cells, u32 size_cells)
> +{
> +#ifdef CONFIG_FLASK
> +    u32 xen_magic = XSM_MAGIC, target_len = 8;
> +    const struct fdt_property *prop;
> +    unsigned char buff[16];
> +    paddr_t start, size;
> +    const __be32 *cell;
> +    int len;
> +
> +    prop = fdt_get_property(fdt, node, "reg", &len);
> +    if ( !prop || len < dt_cells_to_size(address_cells + size_cells))
> +        return 0;
> +
> +    cell = (const __be32 *)prop->data;
> +    device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);

I would prefer if you re-order the code in process_multiboot_node to get 
the base address and size first. This function will then only check if 
the signature is valid.

> +
> +    copy_from_paddr(buff, start, sizeof(buff));
> +
> +    if (memcmp(buff, (void *) &xen_magic, sizeof(u32)) ||
> +        memcmp(buff + sizeof(u32), (void *) &target_len, sizeof(u32)) ||
> +        memcmp(buff + sizeof(u32) * 2, "XenFlask", target_len))

Do we really need to test all those fields? The current check in 
xsm_policy.c only check the magic number.

Also I would prefer if you factor the code to copy/check in an helper 
and re-use it in xsm_dt_policy_init.

> +        return 0;
> +
> +    return 1;
> +#else
> +    return 0;
> +#endif
> +}
> +
>   static void __init process_multiboot_node(const void *fdt, int node,
>                                             const char *name,
>                                             u32 address_cells, u32 size_cells)
> @@ -186,7 +229,13 @@ static void __init process_multiboot_node(const void *fdt, int node,
>       else
>           kind = BOOTMOD_UNKNOWN;
>
> -    /* Guess that first two unknown are kernel and ramdisk respectively. */
> +    /**
> +     * Guess the kind of these first two unknown respectively:
> +     * (1) The first unknown must be kernel;
> +     * (2) The second unknown is ramdisk, only if we have ramdisk;
> +     * (3) Start from the 2nd unknown, detect the XSM binary signature;
> +     * (4) If we got XSM in the 2nd unknown, that means we have not initrd.

s/not/no/

> +     */
>       if ( kind == BOOTMOD_UNKNOWN )
>       {
>           switch ( kind_guess++ )
> @@ -195,6 +244,9 @@ static void __init process_multiboot_node(const void *fdt, int node,
>           case 1: kind = BOOTMOD_RAMDISK; break;
>           default: break;
>           }
> +        if (kind_guess > 1 && check_xsm_signature(fdt, node, name,
> +                                                  address_cells, size_cells))
> +            kind = BOOTMOD_XSM;
>       }
>
>       prop = fdt_get_property(fdt, node, "reg", &len);
>

Regards,

-- 
Julien Grall

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