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

[Paul Moore <paul@paul-moore.com>]

On Fri, Feb 26, 2021 at 9:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> 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?

My personal opinion is that the kernel only needs to provide the error
details to userspace which can be useful in determining what wrong,
and how the user can fix it.  For example, if there is a memory
allocation failure in the kernel there is often little the user can do
(and it is often transient anyway due to loading and other factors),
so simply reporting that there was an allocation failure while
attempting X is sufficient.

Beyond that, I think things can get a little fuzzy, e.g. pr_warn() or
pr_err?  Ratelimit or always emit the message?  I also think the
answers can change as userspace behaviors change over time.  If one of
the policy load error paths uses a pr_err() then we should probably
stick with that; it also seems appropriate as failing to (re)load a
SELinux policy *is* a serious matter.  As far as the rate limiting is
concerned, I'm not sure if that is an important difference here; if
the system is getting enough requests to reload the policy, and
repeatedly failing, such that the ratelimiting matters there are
likely other, much larger, issues at play on the system.

-- 
paul moore
www.paul-moore.com