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 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.

>         }
>
>         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