| Message ID | 1459222015-12036-1-git-send-email-fu.wei@linaro.org |
|---|---|
| State | New |
| Headers | show |
(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,
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@arm.com> wrote: > (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
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); + + 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)) + 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. + */ 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);