On Wed, Mar 3, 2021 at 3:54 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> On Wed, Mar 3, 2021 at 3:56 AM Tyler Hicks <tyhicks@xxxxxxxxxxxxxxxxxxx> wrote:
> > On 2021-02-28 13:52:52, Paul Moore wrote:
> > > On Fri, Feb 26, 2021 at 9:46 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> > > > On Thu, Feb 25, 2021 at 7:15 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > > > On Fri, Feb 12, 2021 at 1:59 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > > > ---
> > > > > > 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.
> >
> > I was a little surprised to see pr_warn_ratelimited() (from both the
> > KERN_WARNING and ratelimited perspectives) used in the policy loading error
> > path so I poked around a bit. The description of commit 4262fb51c9f5 ("selinux:
> > log errors when loading new policy") explains the reasoning:
> >
> > If the policy fails to be loaded from userspace then a warning message is
> > printed, whereas if a failure occurs after loading policy from userspace an
> > error message will be printed with details on where policy loading failed
> > (recreating one of /classes/, /policy_capabilities/, /booleans/ in the
> > SELinux fs).
> >
> > This seems like sound logic and would result in Ondrej using pr_err() in the
> > sel_make_policy_nodes() error path.
>
> 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.
--
paul moore
www.paul-moore.com
