| Message ID | tencent_D0F3F07E25927F681055E6A35C038E168A07@qq.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: check the return value of audit_log_start() | expand |
On Tue, Dec 14, 2021 at 1:20 AM <xkernel.wang@foxmail.com> wrote: > > From: Xiaoke Wang <xkernel.wang@foxmail.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@foxmail.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;
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;