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