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

[Fu Wei <fu.wei@xxxxxxxxxx>]

Hi Julien,

I have taken all you suggestions and posted the v4 patch, please review it.
Great thanks for your help !!

On 2 April 2016 at 02:10, Julien Grall <julien.grall@xxxxxxx> wrote:
> (Use Stefano's new e-mail address)
> Hi Fu Wei,
>
> On 29/03/16 04:26, fu.wei@xxxxxxxxxx wrote:
>>
>> From: Fu Wei <fu.wei@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>> 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



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
Ph: +86 21 61221326(direct)
Ph: +86 186 2020 4684 (mobile)
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021

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