From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gary Tierney Subject: Re: [PATCH 1/2] selinux: log errors when loading new policy Date: Mon, 19 Dec 2016 16:00:41 +0000 Message-ID: <20161219160041.GB5359@workstation> References: <1482007719-14313-1-git-send-email-gary.tierney@gmx.com> <1482007719-14313-2-git-send-email-gary.tierney@gmx.com> <1482158586.28570.17.camel@tycho.nsa.gov> <20161219151946.GA5359@workstation> <1482161529.28570.25.camel@tycho.nsa.gov> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <1482161529.28570.25.camel@tycho.nsa.gov> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: sds@tycho.nsa.gov Cc: selinux@tycho.nsa.gov, linux-audit@redhat.com List-Id: linux-audit@redhat.com 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.=A0=A0If a policy fails to load, an error > > > > message > > > > will > > > > be printed to dmesg with a description of what > > > > failed.=A0=A0Previously if > > > > there was an error during policy loading there would be no > > > > indication > > > > that it failed. > > > > = > > > > Signed-off-by: Gary Tierney > > > > --- > > > > =A0security/selinux/selinuxfs.c | 26 +++++++++++++++++++++----- > > > > =A01 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, > > > > =A0 goto out; > > > > =A0 > > > > =A0 length =3D security_load_policy(data, count); > > > > - if (length) > > > > + if (length) { > > > > + pr_err("SELinux: %s: failed to load policy\n", > > > > + =A0=A0=A0=A0=A0=A0__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().=A0=A0If 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.=A0=A0Though 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.=A0=A0It 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 h= is feedback. > > = > > > = > > > There was an earlier discussion about augmenting the audit logging > > > from > > > this function, so this might overlap with that. =A0I don't know where > > > that stands. > > > = > > > > = > > > > =A0 goto out; > > > > + } > > > > =A0 > > > > =A0 length =3D sel_make_bools(); > > > > - if (length) > > > > + if (length) { > > > > + pr_warn("SELinux: %s: failed to load policy > > > > booleans\n", > > > > + =A0=A0=A0=A0=A0=A0=A0__func__); > > > > =A0 goto out1; > > > > + } > > > > =A0 > > > > =A0 length =3D sel_make_classes(); > > > > - if (length) > > > > + if (length) { > > > > + pr_warn("SELinux: %s: failed to load policy > > > > classes\n", > > > > + =A0=A0=A0=A0=A0=A0=A0__func__); > > > > =A0 goto out1; > > > > + } > > > > =A0 > > > > =A0 length =3D sel_make_policycap(); > > > > - if (length) > > > > + if (length) { > > > > + pr_warn("SELinux: %s: failed to load policy > > > > capabilities\n", > > > > + =A0=A0=A0=A0=A0=A0=A0__func__); > > > > =A0 goto out1; > > > > + } > > > > =A0 > > > > =A0 length =3D count; > > > > =A0 > > > > @@ -1299,9 +1311,13 @@ static int sel_make_bools(void) > > > > =A0 > > > > =A0 isec =3D (struct inode_security_struct *)inode- > > > > > = > > > > > i_security; > > > > =A0 ret =3D security_genfs_sid("selinuxfs", page, > > > > SECCLASS_FILE, &sid); > > > > - if (ret) > > > > + if (ret) { > > > > + pr_warn_ratelimited("SELinux: %s: failed > > > > to > > > > lookup sid for %s\n", > > > > + =A0=A0=A0__func__, page); > > > > =A0 goto out; > > > > =A0 > > > > + } > > > > + > > > > =A0 isec->sid =3D sid; > > > > =A0 isec->initialized =3D LABEL_INITIALIZED; > > > > =A0 inode->i_fop =3D &sel_bool_ops; > > = -- = Gary Tierney GPG fingerprint: 412C 0EF9 C305 68E6 B660 BDAF 706E D765 85AA 79D8 https://sks-keyservers.net/pks/lookup?op=3Dget&search=3D0x706ED76585AA79D8