[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 at 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;
>