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

From: Stephen Smalley <stephen.smalley.work@gmail.com>
To: Paul Moore <paul@paul-moore.com>
Cc: Ondrej Mosnacek <omosnace@redhat.com>,
	Tyler Hicks <tyhicks@linux.microsoft.com>,
	SElinux list <selinux@vger.kernel.org>
Subject: Re: [PATCH v2 1/2] selinux: don't log MAC_POLICY_LOAD record on failed policy load
Date: Thu, 18 Mar 2021 11:12:45 -0400	[thread overview]
Message-ID: <CAEjxPJ4+-PUJW+PbNDwBjoAFJTNQ-7WDNgC3zybYNo3a9eFV=w@mail.gmail.com> (raw)
In-Reply-To: <CAHC9VhSjtP9JN4XTWi0WyWnbF2j5D5HweQn9-Okut_D==UGQ-A@mail.gmail.com>

On Thu, Mar 18, 2021 at 10:48 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Mar 3, 2021 at 3:54 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > The situation has changed a bit since that was written, though... Back
> > then after the policy had been loaded there was no way to turn back
> > and if sel_make_policy_nodes() failed, the new policy would stay and
> > selinuxfs would have been left behind in an inconsistent/broken state.
> > Now this issue is fixed and the new policy isn't actually applied
> > until the selinuxfs preparation succeeds. So from a certain POV, the
> > selinuxfs failure is no longer that fatal and could just print a
> > warning like the other error path, because the result is the same
> > after both failures (active policy and selinuxfs state remains
> > unchanged).
> >
> > Paul (or Stephen if you are reading this and have time to comment),
> > what do you think?
>
> Sorry for the late reply, I lost this in my inbox and since I already
> marked the patchset as "changes requested" in patchwork it fell off my
> radar ...
>
> Anyway, back to your question ... it does seem like pr_warn() is the
> right answer here for the reasons that Ondrej mentioned above, and I
> personally feel it is in keeping with the original patch's intention
> as well; "If the policy fails to be loaded from userspace then a
> warning message is printed ..."  However, I'm not going to lose a lot
> of sleep over differences between pr_warn() and pr_err() here, if
> someone feels strongly that it should be pr_err() and can back that up
> with some solid reasoning and/or precedence then so be it.

That's fine with me.  FWIW, I think the rationale for using
pr_warn_ratelimited() for error returns from security_load_policy()
was that the failure could be entirely userspace-induced, i.e. just
pass the kernel an invalid policy.
The pr_err() messages on sel_make_bools/classes were in contrast
entirely kernel-internal errors and could leave the system in an
inconsistent state as you noted.