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

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

[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