Re: [PATCH v2 1/2] selinux: don't log MAC_POLICY_LOAD record on failed policy load

[Ondrej Mosnacek <omosnace@redhat.com>]

On Thu, Feb 25, 2021 at 7:15 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Feb 12, 2021 at 1:59 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > If sel_make_policy_nodes() fails, we should jump to 'out', not 'out1',
> > as the latter would incorrectly log an MAC_POLICY_LOAD audit record,
> > even though the policy hasn't actually been reloaded. The 'out1' jump
> > label now becomes unused and can be removed.
> >
> > Fixes: 02a52c5c8c3b ("selinux: move policy commit after updating selinuxfs")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  security/selinux/selinuxfs.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> > index 01a7d50ed39b..340711e3dc9a 100644
> > --- a/security/selinux/selinuxfs.c
> > +++ b/security/selinux/selinuxfs.c
> > @@ -651,14 +651,13 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
> >         length = sel_make_policy_nodes(fsi, newpolicy);
> >         if (length) {
> >                 selinux_policy_cancel(fsi->state, newpolicy);
> > -               goto out1;
> > +               goto out;
>
> This looks good, especially with AUDIT_MAC_POLICY_LOAD recording
> "res=1".  However, now that I'm looking at the error path here, we
> don't display anything if sel_make_policy_nodes() fails, do we?  If
> security_load_policy fails we at least do a printk(), but if this
> fails it silently kills the policy load; at the very least I think we
> want a `pr_warn_ratelimited("SELinux: failed to load policy due to
> selinuxfs failures")` or something similar.

There are error messages in some error paths in
sel_make_policy_nodes(), but not all. Those are pr_err()s, while in
sel_write_load() there is a pr_warn_ratelimited(). Could we just unify
the sel_make_policy_nodes() failure to a single message? (I don't
think the information on which part has failed is very useful as the
most likely cause here is a memory allocation failure, not bad
policy.) If so, should it be a pr_warn() or pr_err()? Ratelimited or
not?

>
> >         }
> >
> >         selinux_policy_commit(fsi->state, newpolicy);
> >
> >         length = count;
> >
> > -out1:
> >         audit_log(audit_context(), GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
> >                 "auid=%u ses=%u lsm=selinux res=1",
> >                 from_kuid(&init_user_ns, audit_get_loginuid(current)),
> > --
> > 2.29.2
>
> --
> paul moore
> www.paul-moore.com
>

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.