[PATCH] xsm/flask: Handle policy load failures properly

[PATCH] xsm/flask: Handle policy load failures properly

[Daniel De Graaf]

When the hypervisor is booted with an XSM policy containing an error
(such as a mismatched permission value), this error is mostly ignored
during boot.  This causes FLASK to suspend security policy enforcement
until a policy is loaded, effectively allowing all access.

This patch adds a call to panic() if the policy load fails and a
security policy was specified.  If no security policy was specified, the
existing behavior remains to allow systems to load the security policy
during the boot process with "xl loadpolicy".  A distinct initialization
message has been added to distinguish this case from a successful policy
load in logs.

To clarify that the return value of XSM initcalls is ignored, this patch
also changes the return type of these functions to void.

Reported-by: Julien Grall <julien.grall@linaro.org>
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 xen/include/xsm/xsm.h |  2 +-
 xen/xsm/flask/hooks.c | 15 +++++++++------
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 4ce089f..0437735 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -42,7 +42,7 @@ typedef enum xsm_default xsm_default_t;
 extern char *policy_buffer;
 extern u32 policy_size;
 
-typedef int (*xsm_initcall_t)(void);
+typedef void (*xsm_initcall_t)(void);
 
 extern xsm_initcall_t __xsm_initcall_start[], __xsm_initcall_end[];
 
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index d48463f..ac5d58f 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1625,14 +1625,14 @@ static struct xsm_operations flask_ops = {
 #endif
 };
 
-static __init int flask_init(void)
+static __init void flask_init(void)
 {
-    int ret = 0;
+    int ret;
 
     if ( !flask_enabled )
     {
         printk("Flask:  Disabled at boot.\n");
-        return 0;
+        return;
     }
 
     printk("Flask:  Initializing.\n");
@@ -1645,12 +1645,15 @@ static __init int flask_init(void)
 
     ret = security_load_policy(policy_buffer, policy_size);
 
-    if ( flask_enforcing )
+    if ( ret && policy_size )
+        panic("Flask: Unable to load XSM policy");
+
+    if ( ret )
+        printk("Flask:  Starting with no policy loaded.\n");
+    else if ( flask_enforcing )
         printk("Flask:  Starting in enforcing mode.\n");
     else
         printk("Flask:  Starting in permissive mode.\n");
-
-    return ret;
 }
 
 xsm_initcall(flask_init);
-- 
2.1.0

Re: [PATCH] xsm/flask: Handle policy load failures properly

[Wei Liu]

On Mon, Feb 23, 2015 at 11:11:39AM -0500, Daniel De Graaf wrote:
[...]
> -    if ( flask_enforcing )
> +    if ( ret && policy_size )
> +        panic("Flask: Unable to load XSM policy");
> +
> +    if ( ret )
> +        printk("Flask:  Starting with no policy loaded.\n");
> +    else if ( flask_enforcing )
>          printk("Flask:  Starting in enforcing mode.\n");

I have a question with regard to XSM in general.

This branching gives me the impression that if no policy is provided
flask is not enforced even if you have flask_enforned=1. What mode is it
in? Enforcing or permissive? Is it in permissive mode until a policy is
loaded? Is it enforcing dummy policy (though it appears to pass every
check)?

Wei.

>      else
>          printk("Flask:  Starting in permissive mode.\n");
> -
> -    return ret;
>  }
>  
>  xsm_initcall(flask_init);
> -- 
> 2.1.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] xsm/flask: Handle policy load failures properly

[Daniel De Graaf]

On 02/23/2015 11:48 AM, Wei Liu wrote:
> On Mon, Feb 23, 2015 at 11:11:39AM -0500, Daniel De Graaf wrote:
> [...]
>> -    if ( flask_enforcing )
>> +    if ( ret && policy_size )
>> +        panic("Flask: Unable to load XSM policy");
>> +
>> +    if ( ret )
>> +        printk("Flask:  Starting with no policy loaded.\n");
>> +    else if ( flask_enforcing )
>>           printk("Flask:  Starting in enforcing mode.\n");
>
> I have a question with regard to XSM in general.
>
> This branching gives me the impression that if no policy is provided
> flask is not enforced even if you have flask_enforned=1. What mode is it
> in? Enforcing or permissive? Is it in permissive mode until a policy is
> loaded? Is it enforcing dummy policy (though it appears to pass every
> check)?
>
> Wei.

When no policy is loaded, the FLASK policy is equivalent to an allow-all
policy; see xen/xsm/flask/ss/services.c:security_compute_av where it
bails out if !ss_initialized.  It could be considered as either enforcing
or being permissive with an allow-all policy, but the actual access is
the same.

When a policy is loaded later, the value of flask_enforcing will be used
to decide if the policy is applied in enforcing or permissive mode; by
that time, the value could also have been changed using xl setenforce.

I decided to make the messages exclusive so that you could more easily
tell by looking at a single line if the policy was loaded and enforced
correctly.  Combining both pieces of information in a single line like
the following would also work, if you think this would be better:

printk("Flask: Starting with%s policy loaded in %s mode.\n",
        ret ? " no" : "", flask_enforcing ? "enforcing" : "permissive");

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH] xsm/flask: Handle policy load failures properly

[Wei Liu]

On Mon, Feb 23, 2015 at 12:53:52PM -0500, Daniel De Graaf wrote:
> On 02/23/2015 11:48 AM, Wei Liu wrote:
> >On Mon, Feb 23, 2015 at 11:11:39AM -0500, Daniel De Graaf wrote:
> >[...]
> >>-    if ( flask_enforcing )
> >>+    if ( ret && policy_size )
> >>+        panic("Flask: Unable to load XSM policy");
> >>+
> >>+    if ( ret )
> >>+        printk("Flask:  Starting with no policy loaded.\n");
> >>+    else if ( flask_enforcing )
> >>          printk("Flask:  Starting in enforcing mode.\n");
> >
> >I have a question with regard to XSM in general.
> >
> >This branching gives me the impression that if no policy is provided
> >flask is not enforced even if you have flask_enforned=1. What mode is it
> >in? Enforcing or permissive? Is it in permissive mode until a policy is
> >loaded? Is it enforcing dummy policy (though it appears to pass every
> >check)?
> >
> >Wei.
> 
> When no policy is loaded, the FLASK policy is equivalent to an allow-all
> policy; see xen/xsm/flask/ss/services.c:security_compute_av where it
> bails out if !ss_initialized.  It could be considered as either enforcing
> or being permissive with an allow-all policy, but the actual access is
> the same.
> 
> When a policy is loaded later, the value of flask_enforcing will be used
> to decide if the policy is applied in enforcing or permissive mode; by
> that time, the value could also have been changed using xl setenforce.
> 

Thanks for the explanation.

> I decided to make the messages exclusive so that you could more easily
> tell by looking at a single line if the policy was loaded and enforced
> correctly.  Combining both pieces of information in a single line like
> the following would also work, if you think this would be better:
> 
> printk("Flask: Starting with%s policy loaded in %s mode.\n",
>        ret ? " no" : "", flask_enforcing ? "enforcing" : "permissive");
> 

Yes, I think this is clearer. Thanks.

Wei.

> -- 
> Daniel De Graaf
> National Security Agency

Re: [PATCH] xsm/flask: Handle policy load failures properly

[Andrew Cooper]

On 23/02/15 18:00, Wei Liu wrote:
> On Mon, Feb 23, 2015 at 12:53:52PM -0500, Daniel De Graaf wrote:
>> On 02/23/2015 11:48 AM, Wei Liu wrote:
>>> On Mon, Feb 23, 2015 at 11:11:39AM -0500, Daniel De Graaf wrote:
>>> [...]
>>>> -    if ( flask_enforcing )
>>>> +    if ( ret && policy_size )
>>>> +        panic("Flask: Unable to load XSM policy");
>>>> +
>>>> +    if ( ret )
>>>> +        printk("Flask:  Starting with no policy loaded.\n");
>>>> +    else if ( flask_enforcing )
>>>>          printk("Flask:  Starting in enforcing mode.\n");
>>> I have a question with regard to XSM in general.
>>>
>>> This branching gives me the impression that if no policy is provided
>>> flask is not enforced even if you have flask_enforned=1. What mode is it
>>> in? Enforcing or permissive? Is it in permissive mode until a policy is
>>> loaded? Is it enforcing dummy policy (though it appears to pass every
>>> check)?
>>>
>>> Wei.
>> When no policy is loaded, the FLASK policy is equivalent to an allow-all
>> policy; see xen/xsm/flask/ss/services.c:security_compute_av where it
>> bails out if !ss_initialized.  It could be considered as either enforcing
>> or being permissive with an allow-all policy, but the actual access is
>> the same.
>>
>> When a policy is loaded later, the value of flask_enforcing will be used
>> to decide if the policy is applied in enforcing or permissive mode; by
>> that time, the value could also have been changed using xl setenforce.
>>
> Thanks for the explanation.
>
>> I decided to make the messages exclusive so that you could more easily
>> tell by looking at a single line if the policy was loaded and enforced
>> correctly.  Combining both pieces of information in a single line like
>> the following would also work, if you think this would be better:
>>
>> printk("Flask: Starting with%s policy loaded in %s mode.\n",
>>        ret ? " no" : "", flask_enforcing ? "enforcing" : "permissive");
>>
> Yes, I think this is clearer. Thanks.

One possible string out of that printk is "Starting with no policy
loaded in permissive mode" which is ambiguous.

Perhaps: "Flask: Starting in %s mode.  Policy is %s loaded"

with s/no/not/ ?

~Andrew

Re: [PATCH] xsm/flask: Handle policy load failures properly

[Ian Campbell]

On Mon, 2015-02-23 at 12:53 -0500, Daniel De Graaf wrote:
> When no policy is loaded, the FLASK policy is equivalent to an allow-all
> policy; see xen/xsm/flask/ss/services.c:security_compute_av where it
> bails out if !ss_initialized.  It could be considered as either enforcing
> or being permissive with an allow-all policy, but the actual access is
> the same.

Do you think anyone would want an option to be provided which causes Xen
to fail to boot if a proper policy isn't provided (and loaded)? Similar
to how iommu=force works.

I can see how osstest testcases for xsm might want this to avoid
accidentally testing with no policy, but not sure if it would be
considered generally useful enough to be added.

Ian.

Re: [PATCH] xsm/flask: Handle policy load failures properly

[Julien Grall]

On 24/02/2015 08:47, Ian Campbell wrote:
> On Mon, 2015-02-23 at 12:53 -0500, Daniel De Graaf wrote:
>> When no policy is loaded, the FLASK policy is equivalent to an allow-all
>> policy; see xen/xsm/flask/ss/services.c:security_compute_av where it
>> bails out if !ss_initialized.  It could be considered as either enforcing
>> or being permissive with an allow-all policy, but the actual access is
>> the same.
>
> Do you think anyone would want an option to be provided which causes Xen
> to fail to boot if a proper policy isn't provided (and loaded)? Similar
> to how iommu=force works.
>
> I can see how osstest testcases for xsm might want this to avoid
> accidentally testing with no policy, but not sure if it would be
> considered generally useful enough to be added.

I think it would make sense to panic when flask_enforcing is enabled and 
the policy is not loaded or valid.

Regards,

-- 
Julien Grall

Re: [PATCH] xsm/flask: Handle policy load failures properly

[Ian Campbell]

On Tue, 2015-02-24 at 09:31 +0000, Julien Grall wrote:
> 
> On 24/02/2015 08:47, Ian Campbell wrote:
> > On Mon, 2015-02-23 at 12:53 -0500, Daniel De Graaf wrote:
> >> When no policy is loaded, the FLASK policy is equivalent to an allow-all
> >> policy; see xen/xsm/flask/ss/services.c:security_compute_av where it
> >> bails out if !ss_initialized.  It could be considered as either enforcing
> >> or being permissive with an allow-all policy, but the actual access is
> >> the same.
> >
> > Do you think anyone would want an option to be provided which causes Xen
> > to fail to boot if a proper policy isn't provided (and loaded)? Similar
> > to how iommu=force works.
> >
> > I can see how osstest testcases for xsm might want this to avoid
> > accidentally testing with no policy, but not sure if it would be
> > considered generally useful enough to be added.
> 
> I think it would make sense to panic when flask_enforcing is enabled and 
> the policy is not loaded or valid.

That would stop you running in enforcing mode with a late loaded policy.
A separate flag to enforce boot time loading was what I was thinking of.

Ian.

Re: [PATCH] xsm/flask: Handle policy load failures properly

[Julien Grall]

On 24/02/2015 09:39, Ian Campbell wrote:
> On Tue, 2015-02-24 at 09:31 +0000, Julien Grall wrote:
>>
>> On 24/02/2015 08:47, Ian Campbell wrote:
>>> On Mon, 2015-02-23 at 12:53 -0500, Daniel De Graaf wrote:
>>>> When no policy is loaded, the FLASK policy is equivalent to an allow-all
>>>> policy; see xen/xsm/flask/ss/services.c:security_compute_av where it
>>>> bails out if !ss_initialized.  It could be considered as either enforcing
>>>> or being permissive with an allow-all policy, but the actual access is
>>>> the same.
>>>
>>> Do you think anyone would want an option to be provided which causes Xen
>>> to fail to boot if a proper policy isn't provided (and loaded)? Similar
>>> to how iommu=force works.
>>>
>>> I can see how osstest testcases for xsm might want this to avoid
>>> accidentally testing with no policy, but not sure if it would be
>>> considered generally useful enough to be added.
>>
>> I think it would make sense to panic when flask_enforcing is enabled and
>> the policy is not loaded or valid.
>
> That would stop you running in enforcing mode with a late loaded policy.
> A separate flag to enforce boot time loading was what I was thinking of.

You can enforce the policy later via xl setenforce.

So if someone wants to load a policy later and enforced it, he would 
have to call :
	- xl loadpolicy
	- xl setenforce

IHMO, when you set flask_enforcing on the command line, you expect to 
pass a policy via the bootloader.

Regards,

-- 
Julien Grall

Re: [PATCH] xsm/flask: Handle policy load failures properly

[Ian Campbell]

On Tue, 2015-02-24 at 09:51 +0000, Julien Grall wrote:
> 
> On 24/02/2015 09:39, Ian Campbell wrote:
> > On Tue, 2015-02-24 at 09:31 +0000, Julien Grall wrote:
> >>
> >> On 24/02/2015 08:47, Ian Campbell wrote:
> >>> On Mon, 2015-02-23 at 12:53 -0500, Daniel De Graaf wrote:
> >>>> When no policy is loaded, the FLASK policy is equivalent to an allow-all
> >>>> policy; see xen/xsm/flask/ss/services.c:security_compute_av where it
> >>>> bails out if !ss_initialized.  It could be considered as either enforcing
> >>>> or being permissive with an allow-all policy, but the actual access is
> >>>> the same.
> >>>
> >>> Do you think anyone would want an option to be provided which causes Xen
> >>> to fail to boot if a proper policy isn't provided (and loaded)? Similar
> >>> to how iommu=force works.
> >>>
> >>> I can see how osstest testcases for xsm might want this to avoid
> >>> accidentally testing with no policy, but not sure if it would be
> >>> considered generally useful enough to be added.
> >>
> >> I think it would make sense to panic when flask_enforcing is enabled and
> >> the policy is not loaded or valid.
> >
> > That would stop you running in enforcing mode with a late loaded policy.
> > A separate flag to enforce boot time loading was what I was thinking of.
> 
> You can enforce the policy later via xl setenforce.

Ah, good.

> So if someone wants to load a policy later and enforced it, he would 
> have to call :
> 	- xl loadpolicy
> 	- xl setenforce
> 
> IHMO, when you set flask_enforcing on the command line, you expect to 
> pass a policy via the bootloader.

That doesn't seem unreasonable -- Daniel what do you think?

Re: [PATCH] xsm/flask: Handle policy load failures properly

[Daniel De Graaf]

On 02/24/2015 05:21 AM, Ian Campbell wrote:
> On Tue, 2015-02-24 at 09:51 +0000, Julien Grall wrote:
>>
>> On 24/02/2015 09:39, Ian Campbell wrote:
>>> On Tue, 2015-02-24 at 09:31 +0000, Julien Grall wrote:
>>>>
>>>> On 24/02/2015 08:47, Ian Campbell wrote:
>>>>> On Mon, 2015-02-23 at 12:53 -0500, Daniel De Graaf wrote:
>>>>>> When no policy is loaded, the FLASK policy is equivalent to an allow-all
>>>>>> policy; see xen/xsm/flask/ss/services.c:security_compute_av where it
>>>>>> bails out if !ss_initialized.  It could be considered as either enforcing
>>>>>> or being permissive with an allow-all policy, but the actual access is
>>>>>> the same.
>>>>>
>>>>> Do you think anyone would want an option to be provided which causes Xen
>>>>> to fail to boot if a proper policy isn't provided (and loaded)? Similar
>>>>> to how iommu=force works.
>>>>>
>>>>> I can see how osstest testcases for xsm might want this to avoid
>>>>> accidentally testing with no policy, but not sure if it would be
>>>>> considered generally useful enough to be added.
>>>>
>>>> I think it would make sense to panic when flask_enforcing is enabled and
>>>> the policy is not loaded or valid.
>>>
>>> That would stop you running in enforcing mode with a late loaded policy.
>>> A separate flag to enforce boot time loading was what I was thinking of.
>>
>> You can enforce the policy later via xl setenforce.
>
> Ah, good.
>
>> So if someone wants to load a policy later and enforced it, he would
>> have to call :
>> 	- xl loadpolicy
>> 	- xl setenforce
>>
>> IHMO, when you set flask_enforcing on the command line, you expect to
>> pass a policy via the bootloader.
>
> That doesn't seem unreasonable -- Daniel what do you think?

This seems a reasonable solution if we don't want to change how the boot
parameters are set up.

Another alternative would be to change flask_enforcing/flask_enabled to
a single "flask=" parameter with options:
  disabled - revert to dummy (no XSM) policy, same as flask_enabled=0
  develop/permissive - a missing or broken policy does not panic
  enforce/enforcing/force - require policy to be loaded at boot time
  late/load - bootloader policy is not used; later loadpolicy is enforcing

The default would be "permissive" as in the existing hypervisor.  This
would be more flexible, but I'm not sure it is worth breaking existing
command lines and changing documentation to implement.

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH] xsm/flask: Handle policy load failures properly

[Julien Grall]

Hi Daniel,

On 24/02/15 15:53, Daniel De Graaf wrote:
> This seems a reasonable solution if we don't want to change how the boot
> parameters are set up.
> 
> Another alternative would be to change flask_enforcing/flask_enabled to
> a single "flask=" parameter with options:
>  disabled - revert to dummy (no XSM) policy, same as flask_enabled=0
>  develop/permissive - a missing or broken policy does not panic
>  enforce/enforcing/force - require policy to be loaded at boot time
>  late/load - bootloader policy is not used; later loadpolicy is enforcing
> 
> The default would be "permissive" as in the existing hypervisor.  This
> would be more flexible, but I'm not sure it is worth breaking existing
> command lines and changing documentation to implement.

This look a good solution, having flask_enforcing without flask_enable
doesn't make much sense.

Although I don't know what is the policy about xen parameters. Maybe Ian
or Jan have an idea about it.

Regards,

-- 
Julien Grall

Re: [PATCH] xsm/flask: Handle policy load failures properly

[Ian Campbell]

On Fri, 2015-02-27 at 14:03 +0000, Julien Grall wrote:
> Hi Daniel,
> 
> On 24/02/15 15:53, Daniel De Graaf wrote:
> > This seems a reasonable solution if we don't want to change how the boot
> > parameters are set up.
> > 
> > Another alternative would be to change flask_enforcing/flask_enabled to
> > a single "flask=" parameter with options:
> >  disabled - revert to dummy (no XSM) policy, same as flask_enabled=0
> >  develop/permissive - a missing or broken policy does not panic
> >  enforce/enforcing/force - require policy to be loaded at boot time
> >  late/load - bootloader policy is not used; later loadpolicy is enforcing
> > 
> > The default would be "permissive" as in the existing hypervisor.  This
> > would be more flexible, but I'm not sure it is worth breaking existing
> > command lines and changing documentation to implement.
> 
> This look a good solution, having flask_enforcing without flask_enable
> doesn't make much sense.
> 
> Although I don't know what is the policy about xen parameters. Maybe Ian
> or Jan have an idea about it.

I don't think we generally shy away from making such changes where we
have a good reason.

It might be nice to keep the old options as aliases for the equivalent
new behaviour, I don't know if that should be mandatory thoguh.

Ian.