From: Xiaoke Wang <[email protected]>
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.
Signed-off-by: Xiaoke Wang <[email protected]>
---
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;
--
On Tue, Dec 14, 2021 at 1:20 AM <[email protected]> wrote:
>
> From: Xiaoke Wang <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
http://www.paul-moore.com