Re: [PATCH] Fix boot crash on xsm/flask enabled builds when no policy module is present

[Tomasz Wroblewski <tomasz.wroblewski@citrix.com>]

On 08/26/2013 03:27 PM, Daniel De Graaf wrote:
> On 08/26/2013 06:52 AM, Andrew Cooper wrote:
>> On 26/08/2013 11:03, Tomasz Wroblewski wrote:
>>> Xen crashes on boot of xsm/flask enabled builds, if policy module is 
>>> not specified.
>>> This seems to have worked on 4.1 at least. Can be fixed by testing 
>>> whether policy_buffer
>>> is NULL before attempting to load from it - it's a global which is 
>>> set to non-NULL when
>>> policy module is detected.
>>>
>>> Signed-off-by: Tomasz Wroblewski <tomasz.wroblewski@citrix.com>
>>
>> CCing Daniel De Graaf, as the maintainer of this code.
>>
>> However FWIW,
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>>> ---
>>>   xen/xsm/flask/hooks.c |    3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
>>> index fa0589a..cfa2929 100644
>>> --- a/xen/xsm/flask/hooks.c
>>> +++ b/xen/xsm/flask/hooks.c
>>> @@ -1585,7 +1585,8 @@ static __init int flask_init(void)
>>>       if ( register_xsm(&flask_ops) )
>>>           panic("Flask: Unable to register with XSM.\n");
>>>
>>> -    ret = security_load_policy(policy_buffer, policy_size);
>>> +    if ( policy_buffer )
>>> +        ret = security_load_policy(policy_buffer, policy_size);
>>>
>>>       if ( flask_enforcing )
>>>           printk("Flask:  Starting in enforcing mode.\n");
>>
>
> While this change is not wrong, I also don't see how it could fix
> anything. The security_load_policy function will not dereference
> policy_buffer if policy_size is zero (it'll return -EINVAL first),
> and the only location that sets policy_size (xsm_policy_init) also
> sets policy_buffer. If this function is setting the NULL pointer,
> it also does a dereference - and it will be visible in the printk.
>
Right. I'm trying to sift through security_load_policy to figure out why 
induces the error later, if policy_buffer is null, because yes you are 
right it does not seem to deref the pointer at first glance, but it 
definitely causes the mentioned error later on if it's called. Guess 
this needs further investigation..


> Also, on 08/26/2013 07:12 AM, Jan Beulich wrote:
>> Question is whether policy_buffer == NULL really isn't supposed
>> to result in a -E... return value (as in fact flask initialization 
>> failed).
>
> The return value of flask_init isn't ever checked. A failure to
> load the policy at boot is identical to no policy - waiting for a
> flask_disable or "xl loadpolicy" hypercall from dom0.
>