linux-kernel - Re: [PATCH] selinux: check the return value of audit_log

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAHC9VhRa3H7_CxP_pTsXPgmkTt908k6epMUaK84-FWyvikofpA@mail.gmail.com>
Date:   Tue, 14 Dec 2021 17:59:21 -0500
From:   Paul Moore <paul@...l-moore.com>
To:     xkernel.wang@...mail.com
Cc:     stephen.smalley.work@...il.com, eparis@...isplace.org,
        selinux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] selinux: check the return value of audit_log_start()

On Tue, Dec 14, 2021 at 1:20 AM <xkernel.wang@...mail.com> wrote:
>
> From: Xiaoke Wang <xkernel.wang@...mail.com>
>
> audit_log_start() returns audit_buffer pointer on success or NULL on
> error. It is better to check the return value of it so to prevent
> potential memory access error.

The audit_log_start() function can return NULL in normal use, it does
not always indicate an error; returning NULL simply indicates that no
auditing should be done for this particular instance, e.g. an audit
filter is excluding the record.  Further, there is no potential memory
access error as the audit_log_*() functions are designed to accept a
NULL audit_buffer and return without error.

There have been some cases where we check for a NULL audit_buffer and
skip the following audit_log_*() calls, but that is typically in
performance critical code where the additional function calls would
have an impact (small as they might be).  In the case below, this is
not a critical code path, it is an error path, and here I would rather
have the code as it currently stands; I believe it is cleaner and
easier to read this way.

Regardless, thank you for taking the time to submit a patch.

> Signed-off-by: Xiaoke Wang <xkernel.wang@...mail.com>
> ---
>  security/selinux/ss/services.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index e5f1b27..759d878 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -3277,11 +3277,13 @@ int security_sid_mls_copy(struct selinux_state *state,
>                                 ab = audit_log_start(audit_context(),
>                                                      GFP_ATOMIC,
>                                                      AUDIT_SELINUX_ERR);
> -                               audit_log_format(ab,
> -                                                "op=security_sid_mls_copy invalid_context=");
> -                               /* don't record NUL with untrusted strings */
> -                               audit_log_n_untrustedstring(ab, s, len - 1);
> -                               audit_log_end(ab);
> +                               if (ab) {
> +                                       audit_log_format(ab,
> +                                                       "op=security_sid_mls_copy invalid_context=");
> +                                       /* don't record NUL with untrusted strings */
> +                                       audit_log_n_untrustedstring(ab, s, len - 1);
> +                                       audit_log_end(ab);
> +                               }
>                                 kfree(s);
>                         }
>                         goto out_unlock;

-- 
paul moore
www.paul-moore.com