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?
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
