[PATCH 0/2] kernel: add error handling / logging to sel_write_load()/sel_make_bools()

[PATCH 0/2] kernel: add error handling / logging to sel_write_load()/sel_make_bools()

[Gary Tierney]

Adds error logging to sel_write_load() so there is warning/error messages about
what specifically failed.  Also prints a warning when security_genfs_sid()
fails in sel_make_bools() and defaults the labeling of the relevant /booleans/*
entries to SECINITSID_SECURITY.

Currently if security_genfs_sid() fails in sel_make_bools() the policy will
fail to load, and the system will consequently fail to complete booting.  This
is quite easy to reproduce on Fedora:

# semodule --cil -E base
# sed -i '/genfscon selinuxfs/d' base.cil
# semodule -i base.cil

This will cause load_policy to exit with an error, though it will seem as if
the policy was loaded succesfully (until reboot at least).  When rebooting I
see an error message and the system hangs for a while waiting on D-Bus and
eventually fails to start the login service:

SELinux:  Could not load policy file /etc/selinux/targeted/policy/policy.30:  No such file or directory
... snip ...
[FAILED] Failed to start Login Service.

With the first patch a message will be printed indicating where
sel_write_load() failed and print an error message in sel_make_bools(), the
second will print a warning then also use SECINITSID_SECURITY as a default SID:

[ 1682.776151] SELinux: sel_make_bools: no sid found, defaulting to security isid for /booleans/antivirus_can_scan_system
[ 1682.781782] SELinux: sel_make_bools: no sid found, defaulting to security isid for /booleans/antivirus_use_jit
[ 1682.787027] SELinux: sel_make_bools: no sid found, defaulting to security isid for /booleans/httpd_anon_write

With /sys/fs/selinux/booleans/* showing the correct labels (the security initial SID):

bash-4.3# ls -Z /sys/fs/selinux/booleans/ | head -n 5
system_u:object_r:security_t:s0 abrt_anon_write
system_u:object_r:security_t:s0 abrt_handle_event
system_u:object_r:security_t:s0 abrt_upload_watch_anon_write
system_u:object_r:security_t:s0 antivirus_can_scan_system
system_u:object_r:security_t:s0 antivirus_use_jit

Gary Tierney (2):
  selinux: log errors when loading new policy
  selinux: default to security isid in sel_make_bools() if no sid is
    found

 security/selinux/selinuxfs.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

--
2.7.4

[PATCH 1/2] selinux: log errors when loading new policy

[Gary Tierney]

Adds error and warning messages to the codepaths which can fail when
loading a new policy.  If a policy fails to load, an error message will
be printed to dmesg with a description of what failed.  Previously if
there was an error during policy loading there would be no indication
that it failed.

Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
---
 security/selinux/selinuxfs.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 0aac402..2139cc7 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -522,20 +522,32 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
 		goto out;
 
 	length = security_load_policy(data, count);
-	if (length)
+	if (length) {
+		pr_err("SELinux: %s: failed to load policy\n",
+		      __func__);
 		goto out;
+	}
 
 	length = sel_make_bools();
-	if (length)
+	if (length) {
+		pr_warn("SELinux: %s: failed to load policy booleans\n",
+		       __func__);
 		goto out1;
+	}
 
 	length = sel_make_classes();
-	if (length)
+	if (length) {
+		pr_warn("SELinux: %s: failed to load policy classes\n",
+		       __func__);
 		goto out1;
+	}
 
 	length = sel_make_policycap();
-	if (length)
+	if (length) {
+		pr_warn("SELinux: %s: failed to load policy capabilities\n",
+		       __func__);
 		goto out1;
+	}
 
 	length = count;
 
@@ -1299,9 +1311,13 @@ static int sel_make_bools(void)
 
 		isec = (struct inode_security_struct *)inode->i_security;
 		ret = security_genfs_sid("selinuxfs", page, SECCLASS_FILE, &sid);
-		if (ret)
+		if (ret) {
+			pr_warn_ratelimited("SELinux: %s: failed to lookup sid for %s\n",
+					   __func__, page);
 			goto out;
 
+		}
+
 		isec->sid = sid;
 		isec->initialized = LABEL_INITIALIZED;
 		inode->i_fop = &sel_bool_ops;
-- 
2.7.4

[PATCH 2/2] selinux: default to security isid in sel_make_bools() if no sid is found

[Gary Tierney]

Use SECINITSID_SECURITY as the default SID for booleans which don't have
a matching SID returned from security_genfs_sid().

This prevents the policy failing to load (and consequently the system
failing to boot) when there is no default genfscon statement matched for
the selinuxfs in the new policy.

Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
---
 security/selinux/selinuxfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 2139cc7..c282150 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1312,10 +1312,10 @@ static int sel_make_bools(void)
 		isec = (struct inode_security_struct *)inode->i_security;
 		ret = security_genfs_sid("selinuxfs", page, SECCLASS_FILE, &sid);
 		if (ret) {
-			pr_warn_ratelimited("SELinux: %s: failed to lookup sid for %s\n",
+			pr_warn_ratelimited("SELinux: %s: no sid found, defaulting to security isid for %s\n",
 					   __func__, page);
-			goto out;
 
+			sid = SECINITSID_SECURITY;
 		}
 
 		isec->sid = sid;
-- 
2.7.4

Re: [PATCH 1/2] selinux: log errors when loading new policy

[Stephen Smalley]

On Sat, 2016-12-17 at 20:48 +0000, Gary Tierney wrote:
> Adds error and warning messages to the codepaths which can fail when
> loading a new policy.  If a policy fails to load, an error message
> will
> be printed to dmesg with a description of what failed.  Previously if
> there was an error during policy loading there would be no indication
> that it failed.
> 
> Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
> ---
>  security/selinux/selinuxfs.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/security/selinux/selinuxfs.c
> b/security/selinux/selinuxfs.c
> index 0aac402..2139cc7 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -522,20 +522,32 @@ static ssize_t sel_write_load(struct file
> *file, const char __user *buf,
>  		goto out;
>  
>  	length = security_load_policy(data, count);
> -	if (length)
> +	if (length) {
> +		pr_err("SELinux: %s: failed to load policy\n",
> +		      __func__);

Not sure about your usage of pr_err() vs pr_warn();
security_load_policy() may simply fail due to invalid policy from
userspace, not a kernel-internal error per se.

I would tend to omit the function name; I don't think it is especially
helpful.

There was an earlier discussion about augmenting the audit logging from
this function, so this might overlap with that.  I don't know where
that stands.

>  		goto out;
> +	}
>  
>  	length = sel_make_bools();
> -	if (length)
> +	if (length) {
> +		pr_warn("SELinux: %s: failed to load policy
> booleans\n",
> +		       __func__);
>  		goto out1;
> +	}
>  
>  	length = sel_make_classes();
> -	if (length)
> +	if (length) {
> +		pr_warn("SELinux: %s: failed to load policy
> classes\n",
> +		       __func__);
>  		goto out1;
> +	}
>  
>  	length = sel_make_policycap();
> -	if (length)
> +	if (length) {
> +		pr_warn("SELinux: %s: failed to load policy
> capabilities\n",
> +		       __func__);
>  		goto out1;
> +	}
>  
>  	length = count;
>  
> @@ -1299,9 +1311,13 @@ static int sel_make_bools(void)
>  
>  		isec = (struct inode_security_struct *)inode-
> >i_security;
>  		ret = security_genfs_sid("selinuxfs", page,
> SECCLASS_FILE, &sid);
> -		if (ret)
> +		if (ret) {
> +			pr_warn_ratelimited("SELinux: %s: failed to
> lookup sid for %s\n",
> +					   __func__, page);
>  			goto out;
>  
> +		}
> +
>  		isec->sid = sid;
>  		isec->initialized = LABEL_INITIALIZED;
>  		inode->i_fop = &sel_bool_ops;

Re: [PATCH 1/2] selinux: log errors when loading new policy

[Stephen Smalley]

On Sat, 2016-12-17 at 20:48 +0000, Gary Tierney wrote:
> Adds error and warning messages to the codepaths which can fail when
> loading a new policy.  If a policy fails to load, an error message
> will
> be printed to dmesg with a description of what failed.  Previously if
> there was an error during policy loading there would be no indication
> that it failed.
> 
> Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
> ---
>  security/selinux/selinuxfs.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/security/selinux/selinuxfs.c
> b/security/selinux/selinuxfs.c
> index 0aac402..2139cc7 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -522,20 +522,32 @@ static ssize_t sel_write_load(struct file
> *file, const char __user *buf,
>  		goto out;
>  
>  	length = security_load_policy(data, count);
> -	if (length)
> +	if (length) {
> +		pr_err("SELinux: %s: failed to load policy\n",
> +		      __func__);

Not sure about your usage of pr_err() vs pr_warn();
security_load_policy() may simply fail due to invalid policy from
userspace, not a kernel-internal error per se.

I would tend to omit the function name; I don't think it is especially
helpful.

There was an earlier discussion about augmenting the audit logging from
this function, so this might overlap with that.  I don't know where
that stands.

>  		goto out;
> +	}
>  
>  	length = sel_make_bools();
> -	if (length)
> +	if (length) {
> +		pr_warn("SELinux: %s: failed to load policy
> booleans\n",
> +		       __func__);
>  		goto out1;
> +	}
>  
>  	length = sel_make_classes();
> -	if (length)
> +	if (length) {
> +		pr_warn("SELinux: %s: failed to load policy
> classes\n",
> +		       __func__);
>  		goto out1;
> +	}
>  
>  	length = sel_make_policycap();
> -	if (length)
> +	if (length) {
> +		pr_warn("SELinux: %s: failed to load policy
> capabilities\n",
> +		       __func__);
>  		goto out1;
> +	}
>  
>  	length = count;
>  
> @@ -1299,9 +1311,13 @@ static int sel_make_bools(void)
>  
>  		isec = (struct inode_security_struct *)inode-
> >i_security;
>  		ret = security_genfs_sid("selinuxfs", page,
> SECCLASS_FILE, &sid);
> -		if (ret)
> +		if (ret) {
> +			pr_warn_ratelimited("SELinux: %s: failed to
> lookup sid for %s\n",
> +					   __func__, page);
>  			goto out;
>  
> +		}
> +
>  		isec->sid = sid;
>  		isec->initialized = LABEL_INITIALIZED;
>  		inode->i_fop = &sel_bool_ops;

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 2/2] selinux: default to security isid in sel_make_bools() if no sid is found

[Stephen Smalley]

On Sat, 2016-12-17 at 20:48 +0000, Gary Tierney wrote:
> Use SECINITSID_SECURITY as the default SID for booleans which don't
> have
> a matching SID returned from security_genfs_sid().
> 
> This prevents the policy failing to load (and consequently the system
> failing to boot) when there is no default genfscon statement matched
> for
> the selinuxfs in the new policy.
> 
> Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
> ---
>  security/selinux/selinuxfs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/security/selinux/selinuxfs.c
> b/security/selinux/selinuxfs.c
> index 2139cc7..c282150 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1312,10 +1312,10 @@ static int sel_make_bools(void)
>  		isec = (struct inode_security_struct *)inode-
> >i_security;
>  		ret = security_genfs_sid("selinuxfs", page,
> SECCLASS_FILE, &sid);
>  		if (ret) {
> -			pr_warn_ratelimited("SELinux: %s: failed to
> lookup sid for %s\n",
> +			pr_warn_ratelimited("SELinux: %s: no sid
> found, defaulting to security isid for %s\n",
>  					   __func__, page);
> -			goto out;
>  
> +			sid = SECINITSID_SECURITY;

I wouldn't include the function name; otherwise, LGTM.

>  		}
>  
>  		isec->sid = sid;

Re: [PATCH 1/2] selinux: log errors when loading new policy

[Steve Grubb]

On Monday, December 19, 2016 9:43:06 AM EST Stephen Smalley wrote:
> On Sat, 2016-12-17 at 20:48 +0000, Gary Tierney wrote:
> > Adds error and warning messages to the codepaths which can fail when
> > loading a new policy.  If a policy fails to load, an error message
> > will
> > be printed to dmesg with a description of what failed.  Previously if
> > there was an error during policy loading there would be no indication
> > that it failed.
> > 
> > Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
> > ---
> >  security/selinux/selinuxfs.c | 26 +++++++++++++++++++++-----
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/security/selinux/selinuxfs.c
> > b/security/selinux/selinuxfs.c
> > index 0aac402..2139cc7 100644
> > --- a/security/selinux/selinuxfs.c
> > +++ b/security/selinux/selinuxfs.c
> > @@ -522,20 +522,32 @@ static ssize_t sel_write_load(struct file
> > *file, const char __user *buf,
> >  		goto out;
> >  
> >  	length = security_load_policy(data, count);
> > -	if (length)
> > +	if (length) {
> > +		pr_err("SELinux: %s: failed to load policy\n",
> > +		      __func__);
> 
> Not sure about your usage of pr_err() vs pr_warn();
> security_load_policy() may simply fail due to invalid policy from
> userspace, not a kernel-internal error per se.
> 
> I would tend to omit the function name; I don't think it is especially
> helpful.
> 
> There was an earlier discussion about augmenting the audit logging from
> this function, so this might overlap with that.  I don't know where
> that stands.

I have a new patch that I'm going to send soon that addresses this. But I also 
have a second patch that fixes the setboolean auditing as well, but it 
deadlocks the system. I talked about it with Paul and I have an idea on how to 
fix the deadlock but I haven't sent the updated patches yet. I plan to get to 
them later this week.

-Steve

> >  		goto out;
> > +	}
> >  
> >  	length = sel_make_bools();
> > -	if (length)
> > +	if (length) {
> > +		pr_warn("SELinux: %s: failed to load policy
> > booleans\n",
> > +		       __func__);
> >  		goto out1;
> > +	}
> >  
> >  	length = sel_make_classes();
> > -	if (length)
> > +	if (length) {
> > +		pr_warn("SELinux: %s: failed to load policy
> > classes\n",
> > +		       __func__);
> >  		goto out1;
> > +	}
> >  
> >  	length = sel_make_policycap();
> > -	if (length)
> > +	if (length) {
> > +		pr_warn("SELinux: %s: failed to load policy
> > capabilities\n",
> > +		       __func__);
> >  		goto out1;
> > +	}
> >  
> >  	length = count;
> >  
> > @@ -1299,9 +1311,13 @@ static int sel_make_bools(void)
> >  
> >  		isec = (struct inode_security_struct *)inode-
> > 
> > >i_security;
> > 
> >  		ret = security_genfs_sid("selinuxfs", page,
> > SECCLASS_FILE, &sid);
> > -		if (ret)
> > +		if (ret) {
> > +			pr_warn_ratelimited("SELinux: %s: failed to
> > lookup sid for %s\n",
> > +					   __func__, page);
> >  			goto out;
> >  
> > +		}
> > +
> >  		isec->sid = sid;
> >  		isec->initialized = LABEL_INITIALIZED;
> >  		inode->i_fop = &sel_bool_ops;

Re: [PATCH 1/2] selinux: log errors when loading new policy

[Steve Grubb]

On Monday, December 19, 2016 9:43:06 AM EST Stephen Smalley wrote:
> On Sat, 2016-12-17 at 20:48 +0000, Gary Tierney wrote:
> > Adds error and warning messages to the codepaths which can fail when
> > loading a new policy.  If a policy fails to load, an error message
> > will
> > be printed to dmesg with a description of what failed.  Previously if
> > there was an error during policy loading there would be no indication
> > that it failed.
> > 
> > Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
> > ---
> >  security/selinux/selinuxfs.c | 26 +++++++++++++++++++++-----
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/security/selinux/selinuxfs.c
> > b/security/selinux/selinuxfs.c
> > index 0aac402..2139cc7 100644
> > --- a/security/selinux/selinuxfs.c
> > +++ b/security/selinux/selinuxfs.c
> > @@ -522,20 +522,32 @@ static ssize_t sel_write_load(struct file
> > *file, const char __user *buf,
> >  		goto out;
> >  
> >  	length = security_load_policy(data, count);
> > -	if (length)
> > +	if (length) {
> > +		pr_err("SELinux: %s: failed to load policy\n",
> > +		      __func__);
> 
> Not sure about your usage of pr_err() vs pr_warn();
> security_load_policy() may simply fail due to invalid policy from
> userspace, not a kernel-internal error per se.
> 
> I would tend to omit the function name; I don't think it is especially
> helpful.
> 
> There was an earlier discussion about augmenting the audit logging from
> this function, so this might overlap with that.  I don't know where
> that stands.

I have a new patch that I'm going to send soon that addresses this. But I also 
have a second patch that fixes the setboolean auditing as well, but it 
deadlocks the system. I talked about it with Paul and I have an idea on how to 
fix the deadlock but I haven't sent the updated patches yet. I plan to get to 
them later this week.

-Steve

> >  		goto out;
> > +	}
> >  
> >  	length = sel_make_bools();
> > -	if (length)
> > +	if (length) {
> > +		pr_warn("SELinux: %s: failed to load policy
> > booleans\n",
> > +		       __func__);
> >  		goto out1;
> > +	}
> >  
> >  	length = sel_make_classes();
> > -	if (length)
> > +	if (length) {
> > +		pr_warn("SELinux: %s: failed to load policy
> > classes\n",
> > +		       __func__);
> >  		goto out1;
> > +	}
> >  
> >  	length = sel_make_policycap();
> > -	if (length)
> > +	if (length) {
> > +		pr_warn("SELinux: %s: failed to load policy
> > capabilities\n",
> > +		       __func__);
> >  		goto out1;
> > +	}
> >  
> >  	length = count;
> >  
> > @@ -1299,9 +1311,13 @@ static int sel_make_bools(void)
> >  
> >  		isec = (struct inode_security_struct *)inode-
> > 
> > >i_security;
> > 
> >  		ret = security_genfs_sid("selinuxfs", page,
> > SECCLASS_FILE, &sid);
> > -		if (ret)
> > +		if (ret) {
> > +			pr_warn_ratelimited("SELinux: %s: failed to
> > lookup sid for %s\n",
> > +					   __func__, page);
> >  			goto out;
> >  
> > +		}
> > +
> >  		isec->sid = sid;
> >  		isec->initialized = LABEL_INITIALIZED;
> >  		inode->i_fop = &sel_bool_ops;

Re: [PATCH 1/2] selinux: log errors when loading new policy

[Gary Tierney]

On Mon, Dec 19, 2016 at 09:43:06AM -0500, Stephen Smalley wrote:
> On Sat, 2016-12-17 at 20:48 +0000, Gary Tierney wrote:
> > Adds error and warning messages to the codepaths which can fail when
> > loading a new policy.  If a policy fails to load, an error message
> > will
> > be printed to dmesg with a description of what failed.  Previously if
> > there was an error during policy loading there would be no indication
> > that it failed.
> > 
> > Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
> > ---
> >  security/selinux/selinuxfs.c | 26 +++++++++++++++++++++-----
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/security/selinux/selinuxfs.c
> > b/security/selinux/selinuxfs.c
> > index 0aac402..2139cc7 100644
> > --- a/security/selinux/selinuxfs.c
> > +++ b/security/selinux/selinuxfs.c
> > @@ -522,20 +522,32 @@ static ssize_t sel_write_load(struct file
> > *file, const char __user *buf,
> >  		goto out;
> >  
> >  	length = security_load_policy(data, count);
> > -	if (length)
> > +	if (length) {
> > +		pr_err("SELinux: %s: failed to load policy\n",
> > +		      __func__);
> 
> Not sure about your usage of pr_err() vs pr_warn();
> security_load_policy() may simply fail due to invalid policy from
> userspace, not a kernel-internal error per se.
> 

The intention was to make a distinction between failures on or after
security_load_policy().  If security_load_policy() fails then no audit message
will be logged about loading a new policy, so it seemed more appropriate to
treat that case as KERN_ERROR.  Though with what you said in mind, it is
probably better to change this to pr_warn() as security_load_policy() is
unlikely to cause an actual kernel-internal error.

> I would tend to omit the function name; I don't think it is especially
> helpful.
> 

Agreed.  It seems to be used as a convention throughout security/selinux,
though am happy to drop it from the patch.

I was planning to send a v2 with pr_err() swapped for pr_warn() and __func__
dropped from the log message, though keeping in mind that Steve has prepared a
patch for this (also, logging to the audit subsystem might be more
appropriate) would it be better to drop #1 and keep #2?

> There was an earlier discussion about augmenting the audit logging from
> this function, so this might overlap with that.  I don't know where
> that stands.
> 
> >  		goto out;
> > +	}
> >  
> >  	length = sel_make_bools();
> > -	if (length)
> > +	if (length) {
> > +		pr_warn("SELinux: %s: failed to load policy
> > booleans\n",
> > +		       __func__);
> >  		goto out1;
> > +	}
> >  
> >  	length = sel_make_classes();
> > -	if (length)
> > +	if (length) {
> > +		pr_warn("SELinux: %s: failed to load policy
> > classes\n",
> > +		       __func__);
> >  		goto out1;
> > +	}
> >  
> >  	length = sel_make_policycap();
> > -	if (length)
> > +	if (length) {
> > +		pr_warn("SELinux: %s: failed to load policy
> > capabilities\n",
> > +		       __func__);
> >  		goto out1;
> > +	}
> >  
> >  	length = count;
> >  
> > @@ -1299,9 +1311,13 @@ static int sel_make_bools(void)
> >  
> >  		isec = (struct inode_security_struct *)inode-
> > >i_security;
> >  		ret = security_genfs_sid("selinuxfs", page,
> > SECCLASS_FILE, &sid);
> > -		if (ret)
> > +		if (ret) {
> > +			pr_warn_ratelimited("SELinux: %s: failed to
> > lookup sid for %s\n",
> > +					   __func__, page);
> >  			goto out;
> >  
> > +		}
> > +
> >  		isec->sid = sid;
> >  		isec->initialized = LABEL_INITIALIZED;
> >  		inode->i_fop = &sel_bool_ops;

-- 
Gary Tierney

GPG fingerprint: 412C 0EF9 C305 68E6 B660  BDAF 706E D765 85AA 79D8
https://sks-keyservers.net/pks/lookup?op=get&search=0x706ED76585AA79D8

Re: [PATCH 1/2] selinux: log errors when loading new policy

[Gary Tierney]

On Mon, Dec 19, 2016 at 09:43:06AM -0500, Stephen Smalley wrote:
> On Sat, 2016-12-17 at 20:48 +0000, Gary Tierney wrote:
> > Adds error and warning messages to the codepaths which can fail when
> > loading a new policy.  If a policy fails to load, an error message
> > will
> > be printed to dmesg with a description of what failed.  Previously if
> > there was an error during policy loading there would be no indication
> > that it failed.
> > 
> > Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
> > ---
> >  security/selinux/selinuxfs.c | 26 +++++++++++++++++++++-----
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/security/selinux/selinuxfs.c
> > b/security/selinux/selinuxfs.c
> > index 0aac402..2139cc7 100644
> > --- a/security/selinux/selinuxfs.c
> > +++ b/security/selinux/selinuxfs.c
> > @@ -522,20 +522,32 @@ static ssize_t sel_write_load(struct file
> > *file, const char __user *buf,
> >  		goto out;
> >  
> >  	length = security_load_policy(data, count);
> > -	if (length)
> > +	if (length) {
> > +		pr_err("SELinux: %s: failed to load policy\n",
> > +		      __func__);
> 
> Not sure about your usage of pr_err() vs pr_warn();
> security_load_policy() may simply fail due to invalid policy from
> userspace, not a kernel-internal error per se.
> 

The intention was to make a distinction between failures on or after
security_load_policy().  If security_load_policy() fails then no audit message
will be logged about loading a new policy, so it seemed more appropriate to
treat that case as KERN_ERROR.  Though with what you said in mind, it is
probably better to change this to pr_warn() as security_load_policy() is
unlikely to cause an actual kernel-internal error.

> I would tend to omit the function name; I don't think it is especially
> helpful.
> 

Agreed.  It seems to be used as a convention throughout security/selinux,
though am happy to drop it from the patch.

I was planning to send a v2 with pr_err() swapped for pr_warn() and __func__
dropped from the log message, though keeping in mind that Steve has prepared a
patch for this (also, logging to the audit subsystem might be more
appropriate) would it be better to drop #1 and keep #2?

> There was an earlier discussion about augmenting the audit logging from
> this function, so this might overlap with that.  I don't know where
> that stands.
> 
> >  		goto out;
> > +	}
> >  
> >  	length = sel_make_bools();
> > -	if (length)
> > +	if (length) {
> > +		pr_warn("SELinux: %s: failed to load policy
> > booleans\n",
> > +		       __func__);
> >  		goto out1;
> > +	}
> >  
> >  	length = sel_make_classes();
> > -	if (length)
> > +	if (length) {
> > +		pr_warn("SELinux: %s: failed to load policy
> > classes\n",
> > +		       __func__);
> >  		goto out1;
> > +	}
> >  
> >  	length = sel_make_policycap();
> > -	if (length)
> > +	if (length) {
> > +		pr_warn("SELinux: %s: failed to load policy
> > capabilities\n",
> > +		       __func__);
> >  		goto out1;
> > +	}
> >  
> >  	length = count;
> >  
> > @@ -1299,9 +1311,13 @@ static int sel_make_bools(void)
> >  
> >  		isec = (struct inode_security_struct *)inode-
> > >i_security;
> >  		ret = security_genfs_sid("selinuxfs", page,
> > SECCLASS_FILE, &sid);
> > -		if (ret)
> > +		if (ret) {
> > +			pr_warn_ratelimited("SELinux: %s: failed to
> > lookup sid for %s\n",
> > +					   __func__, page);
> >  			goto out;
> >  
> > +		}
> > +
> >  		isec->sid = sid;
> >  		isec->initialized = LABEL_INITIALIZED;
> >  		inode->i_fop = &sel_bool_ops;

-- 
Gary Tierney

GPG fingerprint: 412C 0EF9 C305 68E6 B660  BDAF 706E D765 85AA 79D8
https://sks-keyservers.net/pks/lookup?op=get&search=0x706ED76585AA79D8

Re: [PATCH 1/2] selinux: log errors when loading new policy

[Stephen Smalley]

On Mon, 2016-12-19 at 15:19 +0000, Gary Tierney wrote:
> On Mon, Dec 19, 2016 at 09:43:06AM -0500, Stephen Smalley wrote:
> > 
> > On Sat, 2016-12-17 at 20:48 +0000, Gary Tierney wrote:
> > > 
> > > Adds error and warning messages to the codepaths which can fail
> > > when
> > > loading a new policy.  If a policy fails to load, an error
> > > message
> > > will
> > > be printed to dmesg with a description of what
> > > failed.  Previously if
> > > there was an error during policy loading there would be no
> > > indication
> > > that it failed.
> > > 
> > > Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
> > > ---
> > >  security/selinux/selinuxfs.c | 26 +++++++++++++++++++++-----
> > >  1 file changed, 21 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/security/selinux/selinuxfs.c
> > > b/security/selinux/selinuxfs.c
> > > index 0aac402..2139cc7 100644
> > > --- a/security/selinux/selinuxfs.c
> > > +++ b/security/selinux/selinuxfs.c
> > > @@ -522,20 +522,32 @@ static ssize_t sel_write_load(struct file
> > > *file, const char __user *buf,
> > >  		goto out;
> > >  
> > >  	length = security_load_policy(data, count);
> > > -	if (length)
> > > +	if (length) {
> > > +		pr_err("SELinux: %s: failed to load policy\n",
> > > +		      __func__);
> > 
> > Not sure about your usage of pr_err() vs pr_warn();
> > security_load_policy() may simply fail due to invalid policy from
> > userspace, not a kernel-internal error per se.
> > 
> 
> The intention was to make a distinction between failures on or after
> security_load_policy().  If security_load_policy() fails then no
> audit message
> will be logged about loading a new policy, so it seemed more
> appropriate to
> treat that case as KERN_ERROR.  Though with what you said in mind, it
> is
> probably better to change this to pr_warn() as security_load_policy()
> is
> unlikely to cause an actual kernel-internal error.

Yes, I tend to view them in the reverse; a failure on
security_load_policy() is just a typical userspace-induced (or OOM)
failure, whereas failure on any of the later calls will leave the
kernel in an inconsistent internal state, so if anything, those should
be the pr_err() cases instead, while security_load_policy() failure
might even need/want a pr_warn_ratelimited() since it can be induced by
userspace (albeit only root with :security load_policy permission).

> 
> > 
> > I would tend to omit the function name; I don't think it is
> > especially
> > helpful.
> > 
> 
> Agreed.  It seems to be used as a convention throughout
> security/selinux,
> though am happy to drop it from the patch.
> 
> I was planning to send a v2 with pr_err() swapped for pr_warn() and
> __func__
> dropped from the log message, though keeping in mind that Steve has
> prepared a
> patch for this (also, logging to the audit subsystem might be more
> appropriate) would it be better to drop #1 and keep #2?

Not sure - I'd have to see Steve's patch or at least hear more details
from him to know whether his patch would obsolete yours or just
complement it.

> 
> > 
> > There was an earlier discussion about augmenting the audit logging
> > from
> > this function, so this might overlap with that.  I don't know where
> > that stands.
> > 
> > > 
> > >  		goto out;
> > > +	}
> > >  
> > >  	length = sel_make_bools();
> > > -	if (length)
> > > +	if (length) {
> > > +		pr_warn("SELinux: %s: failed to load policy
> > > booleans\n",
> > > +		       __func__);
> > >  		goto out1;
> > > +	}
> > >  
> > >  	length = sel_make_classes();
> > > -	if (length)
> > > +	if (length) {
> > > +		pr_warn("SELinux: %s: failed to load policy
> > > classes\n",
> > > +		       __func__);
> > >  		goto out1;
> > > +	}
> > >  
> > >  	length = sel_make_policycap();
> > > -	if (length)
> > > +	if (length) {
> > > +		pr_warn("SELinux: %s: failed to load policy
> > > capabilities\n",
> > > +		       __func__);
> > >  		goto out1;
> > > +	}
> > >  
> > >  	length = count;
> > >  
> > > @@ -1299,9 +1311,13 @@ static int sel_make_bools(void)
> > >  
> > >  		isec = (struct inode_security_struct *)inode-
> > > > 
> > > > i_security;
> > >  		ret = security_genfs_sid("selinuxfs", page,
> > > SECCLASS_FILE, &sid);
> > > -		if (ret)
> > > +		if (ret) {
> > > +			pr_warn_ratelimited("SELinux: %s: failed
> > > to
> > > lookup sid for %s\n",
> > > +					   __func__, page);
> > >  			goto out;
> > >  
> > > +		}
> > > +
> > >  		isec->sid = sid;
> > >  		isec->initialized = LABEL_INITIALIZED;
> > >  		inode->i_fop = &sel_bool_ops;
> 

Re: [PATCH 1/2] selinux: log errors when loading new policy

[Stephen Smalley]

On Mon, 2016-12-19 at 15:19 +0000, Gary Tierney wrote:
> On Mon, Dec 19, 2016 at 09:43:06AM -0500, Stephen Smalley wrote:
> > 
> > On Sat, 2016-12-17 at 20:48 +0000, Gary Tierney wrote:
> > > 
> > > Adds error and warning messages to the codepaths which can fail
> > > when
> > > loading a new policy.  If a policy fails to load, an error
> > > message
> > > will
> > > be printed to dmesg with a description of what
> > > failed.  Previously if
> > > there was an error during policy loading there would be no
> > > indication
> > > that it failed.
> > > 
> > > Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
> > > ---
> > >  security/selinux/selinuxfs.c | 26 +++++++++++++++++++++-----
> > >  1 file changed, 21 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/security/selinux/selinuxfs.c
> > > b/security/selinux/selinuxfs.c
> > > index 0aac402..2139cc7 100644
> > > --- a/security/selinux/selinuxfs.c
> > > +++ b/security/selinux/selinuxfs.c
> > > @@ -522,20 +522,32 @@ static ssize_t sel_write_load(struct file
> > > *file, const char __user *buf,
> > >  		goto out;
> > >  
> > >  	length = security_load_policy(data, count);
> > > -	if (length)
> > > +	if (length) {
> > > +		pr_err("SELinux: %s: failed to load policy\n",
> > > +		      __func__);
> > 
> > Not sure about your usage of pr_err() vs pr_warn();
> > security_load_policy() may simply fail due to invalid policy from
> > userspace, not a kernel-internal error per se.
> > 
> 
> The intention was to make a distinction between failures on or after
> security_load_policy().  If security_load_policy() fails then no
> audit message
> will be logged about loading a new policy, so it seemed more
> appropriate to
> treat that case as KERN_ERROR.  Though with what you said in mind, it
> is
> probably better to change this to pr_warn() as security_load_policy()
> is
> unlikely to cause an actual kernel-internal error.

Yes, I tend to view them in the reverse; a failure on
security_load_policy() is just a typical userspace-induced (or OOM)
failure, whereas failure on any of the later calls will leave the
kernel in an inconsistent internal state, so if anything, those should
be the pr_err() cases instead, while security_load_policy() failure
might even need/want a pr_warn_ratelimited() since it can be induced by
userspace (albeit only root with :security load_policy permission).

> 
> > 
> > I would tend to omit the function name; I don't think it is
> > especially
> > helpful.
> > 
> 
> Agreed.  It seems to be used as a convention throughout
> security/selinux,
> though am happy to drop it from the patch.
> 
> I was planning to send a v2 with pr_err() swapped for pr_warn() and
> __func__
> dropped from the log message, though keeping in mind that Steve has
> prepared a
> patch for this (also, logging to the audit subsystem might be more
> appropriate) would it be better to drop #1 and keep #2?

Not sure - I'd have to see Steve's patch or at least hear more details
from him to know whether his patch would obsolete yours or just
complement it.

> 
> > 
> > There was an earlier discussion about augmenting the audit logging
> > from
> > this function, so this might overlap with that.  I don't know where
> > that stands.
> > 
> > > 
> > >  		goto out;
> > > +	}
> > >  
> > >  	length = sel_make_bools();
> > > -	if (length)
> > > +	if (length) {
> > > +		pr_warn("SELinux: %s: failed to load policy
> > > booleans\n",
> > > +		       __func__);
> > >  		goto out1;
> > > +	}
> > >  
> > >  	length = sel_make_classes();
> > > -	if (length)
> > > +	if (length) {
> > > +		pr_warn("SELinux: %s: failed to load policy
> > > classes\n",
> > > +		       __func__);
> > >  		goto out1;
> > > +	}
> > >  
> > >  	length = sel_make_policycap();
> > > -	if (length)
> > > +	if (length) {
> > > +		pr_warn("SELinux: %s: failed to load policy
> > > capabilities\n",
> > > +		       __func__);
> > >  		goto out1;
> > > +	}
> > >  
> > >  	length = count;
> > >  
> > > @@ -1299,9 +1311,13 @@ static int sel_make_bools(void)
> > >  
> > >  		isec = (struct inode_security_struct *)inode-
> > > > 
> > > > i_security;
> > >  		ret = security_genfs_sid("selinuxfs", page,
> > > SECCLASS_FILE, &sid);
> > > -		if (ret)
> > > +		if (ret) {
> > > +			pr_warn_ratelimited("SELinux: %s: failed
> > > to
> > > lookup sid for %s\n",
> > > +					   __func__, page);
> > >  			goto out;
> > >  
> > > +		}
> > > +
> > >  		isec->sid = sid;
> > >  		isec->initialized = LABEL_INITIALIZED;
> > >  		inode->i_fop = &sel_bool_ops;
> 

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/2] selinux: log errors when loading new policy

[Gary Tierney]

On Mon, Dec 19, 2016 at 10:32:09AM -0500, Stephen Smalley wrote:
> On Mon, 2016-12-19 at 15:19 +0000, Gary Tierney wrote:
> > On Mon, Dec 19, 2016 at 09:43:06AM -0500, Stephen Smalley wrote:
> > > 
> > > On Sat, 2016-12-17 at 20:48 +0000, Gary Tierney wrote:
> > > > 
> > > > Adds error and warning messages to the codepaths which can fail
> > > > when
> > > > loading a new policy.  If a policy fails to load, an error
> > > > message
> > > > will
> > > > be printed to dmesg with a description of what
> > > > failed.  Previously if
> > > > there was an error during policy loading there would be no
> > > > indication
> > > > that it failed.
> > > > 
> > > > Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
> > > > ---
> > > >  security/selinux/selinuxfs.c | 26 +++++++++++++++++++++-----
> > > >  1 file changed, 21 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/security/selinux/selinuxfs.c
> > > > b/security/selinux/selinuxfs.c
> > > > index 0aac402..2139cc7 100644
> > > > --- a/security/selinux/selinuxfs.c
> > > > +++ b/security/selinux/selinuxfs.c
> > > > @@ -522,20 +522,32 @@ static ssize_t sel_write_load(struct file
> > > > *file, const char __user *buf,
> > > >  		goto out;
> > > >  
> > > >  	length = security_load_policy(data, count);
> > > > -	if (length)
> > > > +	if (length) {
> > > > +		pr_err("SELinux: %s: failed to load policy\n",
> > > > +		      __func__);
> > > 
> > > Not sure about your usage of pr_err() vs pr_warn();
> > > security_load_policy() may simply fail due to invalid policy from
> > > userspace, not a kernel-internal error per se.
> > > 
> > 
> > The intention was to make a distinction between failures on or after
> > security_load_policy().  If security_load_policy() fails then no
> > audit message
> > will be logged about loading a new policy, so it seemed more
> > appropriate to
> > treat that case as KERN_ERROR.  Though with what you said in mind, it
> > is
> > probably better to change this to pr_warn() as security_load_policy()
> > is
> > unlikely to cause an actual kernel-internal error.
> 
> Yes, I tend to view them in the reverse; a failure on
> security_load_policy() is just a typical userspace-induced (or OOM)
> failure, whereas failure on any of the later calls will leave the
> kernel in an inconsistent internal state, so if anything, those should
> be the pr_err() cases instead, while security_load_policy() failure
> might even need/want a pr_warn_ratelimited() since it can be induced by
> userspace (albeit only root with :security load_policy permission).
> 

Noted.

> > 
> > > 
> > > I would tend to omit the function name; I don't think it is
> > > especially
> > > helpful.
> > > 
> > 
> > Agreed.  It seems to be used as a convention throughout
> > security/selinux,
> > though am happy to drop it from the patch.
> > 
> > I was planning to send a v2 with pr_err() swapped for pr_warn() and
> > __func__
> > dropped from the log message, though keeping in mind that Steve has
> > prepared a
> > patch for this (also, logging to the audit subsystem might be more
> > appropriate) would it be better to drop #1 and keep #2?
> 
> Not sure - I'd have to see Steve's patch or at least hear more details
> from him to know whether his patch would obsolete yours or just
> complement it.
> 

Right, I'll spin up a v2 with the recommended changes and CC in Steve for his
feedback.

> > 
> > > 
> > > There was an earlier discussion about augmenting the audit logging
> > > from
> > > this function, so this might overlap with that.  I don't know where
> > > that stands.
> > > 
> > > > 
> > > >  		goto out;
> > > > +	}
> > > >  
> > > >  	length = sel_make_bools();
> > > > -	if (length)
> > > > +	if (length) {
> > > > +		pr_warn("SELinux: %s: failed to load policy
> > > > booleans\n",
> > > > +		       __func__);
> > > >  		goto out1;
> > > > +	}
> > > >  
> > > >  	length = sel_make_classes();
> > > > -	if (length)
> > > > +	if (length) {
> > > > +		pr_warn("SELinux: %s: failed to load policy
> > > > classes\n",
> > > > +		       __func__);
> > > >  		goto out1;
> > > > +	}
> > > >  
> > > >  	length = sel_make_policycap();
> > > > -	if (length)
> > > > +	if (length) {
> > > > +		pr_warn("SELinux: %s: failed to load policy
> > > > capabilities\n",
> > > > +		       __func__);
> > > >  		goto out1;
> > > > +	}
> > > >  
> > > >  	length = count;
> > > >  
> > > > @@ -1299,9 +1311,13 @@ static int sel_make_bools(void)
> > > >  
> > > >  		isec = (struct inode_security_struct *)inode-
> > > > > 
> > > > > i_security;
> > > >  		ret = security_genfs_sid("selinuxfs", page,
> > > > SECCLASS_FILE, &sid);
> > > > -		if (ret)
> > > > +		if (ret) {
> > > > +			pr_warn_ratelimited("SELinux: %s: failed
> > > > to
> > > > lookup sid for %s\n",
> > > > +					   __func__, page);
> > > >  			goto out;
> > > >  
> > > > +		}
> > > > +
> > > >  		isec->sid = sid;
> > > >  		isec->initialized = LABEL_INITIALIZED;
> > > >  		inode->i_fop = &sel_bool_ops;
> > 

-- 
Gary Tierney

GPG fingerprint: 412C 0EF9 C305 68E6 B660  BDAF 706E D765 85AA 79D8
https://sks-keyservers.net/pks/lookup?op=get&search=0x706ED76585AA79D8

Re: [PATCH 1/2] selinux: log errors when loading new policy

[Gary Tierney]

On Mon, Dec 19, 2016 at 10:32:09AM -0500, Stephen Smalley wrote:
> On Mon, 2016-12-19 at 15:19 +0000, Gary Tierney wrote:
> > On Mon, Dec 19, 2016 at 09:43:06AM -0500, Stephen Smalley wrote:
> > > 
> > > On Sat, 2016-12-17 at 20:48 +0000, Gary Tierney wrote:
> > > > 
> > > > Adds error and warning messages to the codepaths which can fail
> > > > when
> > > > loading a new policy.  If a policy fails to load, an error
> > > > message
> > > > will
> > > > be printed to dmesg with a description of what
> > > > failed.  Previously if
> > > > there was an error during policy loading there would be no
> > > > indication
> > > > that it failed.
> > > > 
> > > > Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
> > > > ---
> > > >  security/selinux/selinuxfs.c | 26 +++++++++++++++++++++-----
> > > >  1 file changed, 21 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/security/selinux/selinuxfs.c
> > > > b/security/selinux/selinuxfs.c
> > > > index 0aac402..2139cc7 100644
> > > > --- a/security/selinux/selinuxfs.c
> > > > +++ b/security/selinux/selinuxfs.c
> > > > @@ -522,20 +522,32 @@ static ssize_t sel_write_load(struct file
> > > > *file, const char __user *buf,
> > > >  		goto out;
> > > >  
> > > >  	length = security_load_policy(data, count);
> > > > -	if (length)
> > > > +	if (length) {
> > > > +		pr_err("SELinux: %s: failed to load policy\n",
> > > > +		      __func__);
> > > 
> > > Not sure about your usage of pr_err() vs pr_warn();
> > > security_load_policy() may simply fail due to invalid policy from
> > > userspace, not a kernel-internal error per se.
> > > 
> > 
> > The intention was to make a distinction between failures on or after
> > security_load_policy().  If security_load_policy() fails then no
> > audit message
> > will be logged about loading a new policy, so it seemed more
> > appropriate to
> > treat that case as KERN_ERROR.  Though with what you said in mind, it
> > is
> > probably better to change this to pr_warn() as security_load_policy()
> > is
> > unlikely to cause an actual kernel-internal error.
> 
> Yes, I tend to view them in the reverse; a failure on
> security_load_policy() is just a typical userspace-induced (or OOM)
> failure, whereas failure on any of the later calls will leave the
> kernel in an inconsistent internal state, so if anything, those should
> be the pr_err() cases instead, while security_load_policy() failure
> might even need/want a pr_warn_ratelimited() since it can be induced by
> userspace (albeit only root with :security load_policy permission).
> 

Noted.

> > 
> > > 
> > > I would tend to omit the function name; I don't think it is
> > > especially
> > > helpful.
> > > 
> > 
> > Agreed.  It seems to be used as a convention throughout
> > security/selinux,
> > though am happy to drop it from the patch.
> > 
> > I was planning to send a v2 with pr_err() swapped for pr_warn() and
> > __func__
> > dropped from the log message, though keeping in mind that Steve has
> > prepared a
> > patch for this (also, logging to the audit subsystem might be more
> > appropriate) would it be better to drop #1 and keep #2?
> 
> Not sure - I'd have to see Steve's patch or at least hear more details
> from him to know whether his patch would obsolete yours or just
> complement it.
> 

Right, I'll spin up a v2 with the recommended changes and CC in Steve for his
feedback.

> > 
> > > 
> > > There was an earlier discussion about augmenting the audit logging
> > > from
> > > this function, so this might overlap with that.  I don't know where
> > > that stands.
> > > 
> > > > 
> > > >  		goto out;
> > > > +	}
> > > >  
> > > >  	length = sel_make_bools();
> > > > -	if (length)
> > > > +	if (length) {
> > > > +		pr_warn("SELinux: %s: failed to load policy
> > > > booleans\n",
> > > > +		       __func__);
> > > >  		goto out1;
> > > > +	}
> > > >  
> > > >  	length = sel_make_classes();
> > > > -	if (length)
> > > > +	if (length) {
> > > > +		pr_warn("SELinux: %s: failed to load policy
> > > > classes\n",
> > > > +		       __func__);
> > > >  		goto out1;
> > > > +	}
> > > >  
> > > >  	length = sel_make_policycap();
> > > > -	if (length)
> > > > +	if (length) {
> > > > +		pr_warn("SELinux: %s: failed to load policy
> > > > capabilities\n",
> > > > +		       __func__);
> > > >  		goto out1;
> > > > +	}
> > > >  
> > > >  	length = count;
> > > >  
> > > > @@ -1299,9 +1311,13 @@ static int sel_make_bools(void)
> > > >  
> > > >  		isec = (struct inode_security_struct *)inode-
> > > > > 
> > > > > i_security;
> > > >  		ret = security_genfs_sid("selinuxfs", page,
> > > > SECCLASS_FILE, &sid);
> > > > -		if (ret)
> > > > +		if (ret) {
> > > > +			pr_warn_ratelimited("SELinux: %s: failed
> > > > to
> > > > lookup sid for %s\n",
> > > > +					   __func__, page);
> > > >  			goto out;
> > > >  
> > > > +		}
> > > > +
> > > >  		isec->sid = sid;
> > > >  		isec->initialized = LABEL_INITIALIZED;
> > > >  		inode->i_fop = &sel_bool_ops;
> > 

-- 
Gary Tierney

GPG fingerprint: 412C 0EF9 C305 68E6 B660  BDAF 706E D765 85AA 79D8
https://sks-keyservers.net/pks/lookup?op=get&search=0x706ED76585AA79D8

[PATCH v2 0/2]

[Gary Tierney]

Have updated the patches to print error messages for failures which result in
indeterminate state and warnings for failures to load policy from userspace.
Also updated the patches to remove the function name from log messages.

Steve,

Does your work on AUDIT_MAC_STATUS_FAIL/AUDIT_MAC_LOAD_FAIL messages (I'm
assuming that's what Stephen's referencing in his previous mail) obsolete the
printk logs in the first patch?  An AUDIT_MAC_POLICY_LOAD message would still
be logged presently even if one of sel_make_{bools,classes,policycap} fails, so
I'm not sure if you would also want an
AUDIT_MAC_STATUS_FAIL/AUDIT_MAC_LOAD_FAIL message when that happens, though I
think you might want one in the first case when security_load_policy() fails
(or anything up until that point).

Gary Tierney (2):
  selinux: log errors when loading new policy
  selinux: default to security isid in sel_make_bools() if no sid is
    found

 security/selinux/selinuxfs.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] selinux: log errors when loading new policy

[Gary Tierney]

Adds error logging to the code paths which can fail when loading a new
policy in sel_write_load().  If the policy fails to be loaded from
userspace then a warning message is printed, whereas if a failure occurs
after loading policy from userspace an error message will be printed
with details on where policy loading failed (recreating one of /classes/,
/policy_capabilities/, /booleans/ in the SELinux fs).

Also, if sel_make_bools() fails to obtain an SID for an entry in
/booleans/* an error will be printed indicating the path of the
boolean.

Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
---
 security/selinux/selinuxfs.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 0aac402..e667c34 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -522,20 +522,28 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
 		goto out;
 
 	length = security_load_policy(data, count);
-	if (length)
+	if (length) {
+		pr_warn_ratelimited("SELinux: failed to load policy\n");
 		goto out;
+	}
 
 	length = sel_make_bools();
-	if (length)
+	if (length) {
+		pr_err("SELinux: failed to load policy booleans\n");
 		goto out1;
+	}
 
 	length = sel_make_classes();
-	if (length)
+	if (length) {
+		pr_err("SELinux: failed to load policy classes\n");
 		goto out1;
+	}
 
 	length = sel_make_policycap();
-	if (length)
+	if (length) {
+		pr_err("SELinux: failed to load policy capabilities\n");
 		goto out1;
+	}
 
 	length = count;
 
@@ -1299,9 +1307,12 @@ static int sel_make_bools(void)
 
 		isec = (struct inode_security_struct *)inode->i_security;
 		ret = security_genfs_sid("selinuxfs", page, SECCLASS_FILE, &sid);
-		if (ret)
+		if (ret) {
+			pr_err("SELinux: failed to lookup sid for %s\n", page);
 			goto out;
 
+		}
+
 		isec->sid = sid;
 		isec->initialized = LABEL_INITIALIZED;
 		inode->i_fop = &sel_bool_ops;
-- 
2.7.4

[PATCH v2 2/2] selinux: default to security isid in sel_make_bools() if no sid is found

[Gary Tierney]

Use SECINITSID_SECURITY as the default SID for booleans which don't have
a matching SID returned from security_genfs_sid(), also update the
error message to a warning which matches this.

This prevents the policy failing to load (and consequently the system
failing to boot) when there is no default genfscon statement matched for
the selinuxfs in the new policy.

Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
---
 security/selinux/selinuxfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index e667c34..616a8d2 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1308,9 +1308,9 @@ static int sel_make_bools(void)
 		isec = (struct inode_security_struct *)inode->i_security;
 		ret = security_genfs_sid("selinuxfs", page, SECCLASS_FILE, &sid);
 		if (ret) {
-			pr_err("SELinux: failed to lookup sid for %s\n", page);
-			goto out;
-
+			pr_warn_ratelimited("SELinux: no sid found, defaulting to security isid for %s\n",
+					   page);
+			sid = SECINITSID_SECURITY;
 		}
 
 		isec->sid = sid;
-- 
2.7.4

Re: [PATCH v2 0/2]

[Steve Grubb]

On Tuesday, December 20, 2016 1:28:45 AM EST Gary Tierney wrote:
> Have updated the patches to print error messages for failures which result
> in indeterminate state and warnings for failures to load policy from
> userspace. Also updated the patches to remove the function name from log
> messages.
> 
> Steve,
> 
> Does your work on AUDIT_MAC_STATUS_FAIL/AUDIT_MAC_LOAD_FAIL messages (I'm
> assuming that's what Stephen's referencing in his previous mail) obsolete
> the printk logs in the first patch?

No, audit cares only about audit events. We don't care at all about syslog 
messages. However, they ought to be singing the same song so to speak.

-Steve

Re: [PATCH v2 1/2] selinux: log errors when loading new policy

[Stephen Smalley]

On Tue, 2016-12-20 at 01:28 +0000, Gary Tierney wrote:
> Adds error logging to the code paths which can fail when loading a
> new
> policy in sel_write_load().  If the policy fails to be loaded from
> userspace then a warning message is printed, whereas if a failure
> occurs
> after loading policy from userspace an error message will be printed
> with details on where policy loading failed (recreating one of
> /classes/,
> /policy_capabilities/, /booleans/ in the SELinux fs).
> 
> Also, if sel_make_bools() fails to obtain an SID for an entry in
> /booleans/* an error will be printed indicating the path of the
> boolean.
> 
> Signed-off-by: Gary Tierney <gary.tierney@gmx.com>

Acked-by: Stephen Smalley <sds@tycho.nsa.gov>

> ---
>  security/selinux/selinuxfs.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/security/selinux/selinuxfs.c
> b/security/selinux/selinuxfs.c
> index 0aac402..e667c34 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -522,20 +522,28 @@ static ssize_t sel_write_load(struct file
> *file, const char __user *buf,
>  		goto out;
>  
>  	length = security_load_policy(data, count);
> -	if (length)
> +	if (length) {
> +		pr_warn_ratelimited("SELinux: failed to load
> policy\n");
>  		goto out;
> +	}
>  
>  	length = sel_make_bools();
> -	if (length)
> +	if (length) {
> +		pr_err("SELinux: failed to load policy booleans\n");
>  		goto out1;
> +	}
>  
>  	length = sel_make_classes();
> -	if (length)
> +	if (length) {
> +		pr_err("SELinux: failed to load policy classes\n");
>  		goto out1;
> +	}
>  
>  	length = sel_make_policycap();
> -	if (length)
> +	if (length) {
> +		pr_err("SELinux: failed to load policy
> capabilities\n");
>  		goto out1;
> +	}
>  
>  	length = count;
>  
> @@ -1299,9 +1307,12 @@ static int sel_make_bools(void)
>  
>  		isec = (struct inode_security_struct *)inode-
> >i_security;
>  		ret = security_genfs_sid("selinuxfs", page,
> SECCLASS_FILE, &sid);
> -		if (ret)
> +		if (ret) {
> +			pr_err("SELinux: failed to lookup sid for
> %s\n", page);
>  			goto out;
>  
> +		}
> +
>  		isec->sid = sid;
>  		isec->initialized = LABEL_INITIALIZED;
>  		inode->i_fop = &sel_bool_ops;

Re: [PATCH v2 2/2] selinux: default to security isid in sel_make_bools() if no sid is found

[Stephen Smalley]

On Tue, 2016-12-20 at 01:28 +0000, Gary Tierney wrote:
> Use SECINITSID_SECURITY as the default SID for booleans which don't
> have
> a matching SID returned from security_genfs_sid(), also update the
> error message to a warning which matches this.
> 
> This prevents the policy failing to load (and consequently the system
> failing to boot) when there is no default genfscon statement matched
> for
> the selinuxfs in the new policy.
> 
> Signed-off-by: Gary Tierney <gary.tierney@gmx.com>

Acked-by: Stephen Smalley <sds@tycho.nsa.gov>

> ---
>  security/selinux/selinuxfs.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/security/selinux/selinuxfs.c
> b/security/selinux/selinuxfs.c
> index e667c34..616a8d2 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1308,9 +1308,9 @@ static int sel_make_bools(void)
>  		isec = (struct inode_security_struct *)inode-
> >i_security;
>  		ret = security_genfs_sid("selinuxfs", page,
> SECCLASS_FILE, &sid);
>  		if (ret) {
> -			pr_err("SELinux: failed to lookup sid for
> %s\n", page);
> -			goto out;
> -
> +			pr_warn_ratelimited("SELinux: no sid found,
> defaulting to security isid for %s\n",
> +					   page);
> +			sid = SECINITSID_SECURITY;
>  		}
>  
>  		isec->sid = sid;

Re: [PATCH v2 1/2] selinux: log errors when loading new policy

[Paul Moore]

On Mon, Dec 19, 2016 at 8:28 PM, Gary Tierney <gary.tierney@gmx.com> wrote:
> Adds error logging to the code paths which can fail when loading a new
> policy in sel_write_load().  If the policy fails to be loaded from
> userspace then a warning message is printed, whereas if a failure occurs
> after loading policy from userspace an error message will be printed
> with details on where policy loading failed (recreating one of /classes/,
> /policy_capabilities/, /booleans/ in the SELinux fs).
>
> Also, if sel_make_bools() fails to obtain an SID for an entry in
> /booleans/* an error will be printed indicating the path of the
> boolean.
>
> Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
> ---
>  security/selinux/selinuxfs.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)

My apologies for the delay, this looks good to me - merged.  Thanks.

> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 0aac402..e667c34 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -522,20 +522,28 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
>                 goto out;
>
>         length = security_load_policy(data, count);
> -       if (length)
> +       if (length) {
> +               pr_warn_ratelimited("SELinux: failed to load policy\n");
>                 goto out;
> +       }
>
>         length = sel_make_bools();
> -       if (length)
> +       if (length) {
> +               pr_err("SELinux: failed to load policy booleans\n");
>                 goto out1;
> +       }
>
>         length = sel_make_classes();
> -       if (length)
> +       if (length) {
> +               pr_err("SELinux: failed to load policy classes\n");
>                 goto out1;
> +       }
>
>         length = sel_make_policycap();
> -       if (length)
> +       if (length) {
> +               pr_err("SELinux: failed to load policy capabilities\n");
>                 goto out1;
> +       }
>
>         length = count;
>
> @@ -1299,9 +1307,12 @@ static int sel_make_bools(void)
>
>                 isec = (struct inode_security_struct *)inode->i_security;
>                 ret = security_genfs_sid("selinuxfs", page, SECCLASS_FILE, &sid);
> -               if (ret)
> +               if (ret) {
> +                       pr_err("SELinux: failed to lookup sid for %s\n", page);
>                         goto out;
>
> +               }
> +
>                 isec->sid = sid;
>                 isec->initialized = LABEL_INITIALIZED;
>                 inode->i_fop = &sel_bool_ops;
> --
> 2.7.4
>



-- 
paul moore
www.paul-moore.com

Re: [PATCH v2 2/2] selinux: default to security isid in sel_make_bools() if no sid is found

[Paul Moore]

On Mon, Dec 19, 2016 at 8:28 PM, Gary Tierney <gary.tierney@gmx.com> wrote:
> Use SECINITSID_SECURITY as the default SID for booleans which don't have
> a matching SID returned from security_genfs_sid(), also update the
> error message to a warning which matches this.
>
> This prevents the policy failing to load (and consequently the system
> failing to boot) when there is no default genfscon statement matched for
> the selinuxfs in the new policy.
>
> Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
> ---
>  security/selinux/selinuxfs.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Also merged, thank you.

> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index e667c34..616a8d2 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1308,9 +1308,9 @@ static int sel_make_bools(void)
>                 isec = (struct inode_security_struct *)inode->i_security;
>                 ret = security_genfs_sid("selinuxfs", page, SECCLASS_FILE, &sid);
>                 if (ret) {
> -                       pr_err("SELinux: failed to lookup sid for %s\n", page);
> -                       goto out;
> -
> +                       pr_warn_ratelimited("SELinux: no sid found, defaulting to security isid for %s\n",
> +                                          page);
> +                       sid = SECINITSID_SECURITY;
>                 }
>
>                 isec->sid = sid;
> --
> 2.7.4
>

-- 
paul moore
www.paul-moore.com