| Message ID | 1459187707-25628-1-git-send-email-fu.wei@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
Hi Konrad, On 29 March 2016 at 04:54, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > On Tue, Mar 29, 2016 at 01:55:07AM +0800, fu.wei@linaro.org wrote: >> From: Fu Wei <fu.wei@linaro.org> >> >> This patch add a check_xsm_signature static function for detecting XSM >> 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: >> (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 don't load initrd. >> > > Pls make the 'xen' be 'Xen'. Thanks, :-) > >> Signed-off-by: Fu Wei <fu.wei@linaro.org> > > Cc-ing also Daniel (XSM maintainer). Thanks, I will add him into to-list next time:-) > > And Julien (linaro.org != arm.com) and Stefano. >> --- >> v2: 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 | 57 +++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 56 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c >> index 8a14015..322f17f 100644 >> --- a/xen/arch/arm/bootfdt.c >> +++ b/xen/arch/arm/bootfdt.c >> @@ -163,6 +163,52 @@ 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. >> + * So we only read the first 16 Bytes of the module, then check these three > > s/Bytes/bytes/ thanks , will do >> + * parts. > > Is it possible for the hypervisor to chnage the policy magic number? Perhaps > you should have : > > BUILD_BUG_ON(0xf97cff8c != XSM_MAGIC); > > to guard against changes? > >> + */ >> +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; >> + paddr_t start, size; >> + const __be32 *cell; >> + char buff[16]; >> + int len; >> + >> + prop = fdt_get_property(fdt, node, "reg", &len); >> + if ( !prop ) >> + panic("node %s missing `reg' property\n", name); > > Why panic? Can't you just return? > >> + >> + if ( len < dt_cells_to_size(address_cells + size_cells) ) >> + panic("fdt: node `%s': `reg` property length is too short\n", name); > > Ditto? Good point, will do Thanks for your review :-) > >> + >> + cell = (const __be32 *)prop->data; >> + device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); >> + >> + copy_from_paddr(buff, start, sizeof(buff)); >> + >> + if (strncmp(buff, (char *) &xen_magic, sizeof(u32)) || >> + strncmp(buff + sizeof(u32), (char *) &target_len, sizeof(u32)) || >> + strncmp(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 +232,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 +247,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); >> -- >> 2.5.0 >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel
Hi Daniel, On 29 March 2016 at 06:08, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote: > On 03/28/2016 04:54 PM, Konrad Rzeszutek Wilk wrote: >> >> On Tue, Mar 29, 2016 at 01:55:07AM +0800, fu.wei@linaro.org wrote: >>> >>> From: Fu Wei <fu.wei@linaro.org> >>> >>> This patch add a check_xsm_signature static function for detecting XSM >>> 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: >>> (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 don't load >>> initrd. >>> >> >> Pls make the 'xen' be 'Xen'. >> >>> Signed-off-by: Fu Wei <fu.wei@linaro.org> >> >> >> Cc-ing also Daniel (XSM maintainer). >> >> And Julien (linaro.org != arm.com) and Stefano. >>> >>> --- >>> v2: 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 | 57 >>> +++++++++++++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 56 insertions(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c >>> index 8a14015..322f17f 100644 >>> --- a/xen/arch/arm/bootfdt.c >>> +++ b/xen/arch/arm/bootfdt.c >>> @@ -163,6 +163,52 @@ 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. >>> + * So we only read the first 16 Bytes of the module, then check these >>> three >> >> >> s/Bytes/bytes/ >>> >>> + * parts. >> >> >> Is it possible for the hypervisor to chnage the policy magic number? >> Perhaps >> you should have : >> >> BUILD_BUG_ON(0xf97cff8c != XSM_MAGIC); >> >> to guard against changes? > > > The value of XSM_MAGIC will always be that constant if FLASK is the enabled > security module; the value was different when the (now-removed) ACM module > was selected. OK, it seems we can just use this :-) > > [...] >>> >>> + if (strncmp(buff, (char *) &xen_magic, sizeof(u32)) || >>> + strncmp(buff + sizeof(u32), (char *) &target_len, sizeof(u32)) >>> || >>> + strncmp(buff + sizeof(u32) * 2, "XenFlask", target_len)) >>> + return 0; >>> + > > > memcmp() is more correct than strncmp() here, especially since target_len > will > have embedded NULLs. It also assumes little endian byte order; is that > worth > commenting on? yes, thanks. I think memcmp() is correct too! :-) I have added a comment in the next version :-) Please check my v3 patch :-) http://lists.xen.org/archives/html/xen-devel/2016-03/msg03564.html > > -- > Daniel De Graaf > National Security Agency
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index 8a14015..322f17f 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -163,6 +163,52 @@ 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. + * So we only read the first 16 Bytes of the module, then check these three + * parts. + */ +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; + paddr_t start, size; + const __be32 *cell; + char buff[16]; + int len; + + prop = fdt_get_property(fdt, node, "reg", &len); + if ( !prop ) + panic("node %s missing `reg' property\n", name); + + if ( len < dt_cells_to_size(address_cells + size_cells) ) + panic("fdt: node `%s': `reg` property length is too short\n", name); + + cell = (const __be32 *)prop->data; + device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); + + copy_from_paddr(buff, start, sizeof(buff)); + + if (strncmp(buff, (char *) &xen_magic, sizeof(u32)) || + strncmp(buff + sizeof(u32), (char *) &target_len, sizeof(u32)) || + strncmp(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 +232,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 +247,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);