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

[Fu Wei <fu.wei@xxxxxxxxxx>]

Hi Daniel,

On 29 March 2016 at 06:08, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
> On 03/28/2016 04:54 PM, Konrad Rzeszutek Wilk wrote:
>>
>> On Tue, Mar 29, 2016 at 01:55:07AM +0800, fu.wei@xxxxxxxxxx wrote:
>>>
>>> From: Fu Wei <fu.wei@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>
>>
>> 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



-- 
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