linux-kernel - Re: [PATCH] lsm_audit: use get_task

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAHC9VhTjpUShdbNpw0SaG0LS44UyLxcbCz+6aPaamA26wA-ywQ@mail.gmail.com>
Date:   Mon, 28 Aug 2017 17:54:27 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Geliang Tang <geliangtang@...il.com>
Cc:     James Morris <james.l.morris@...cle.com>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-audit@...hat.com
Subject: Re: [PATCH] lsm_audit: use get_task_comm

On Mon, Aug 28, 2017 at 9:58 AM, Geliang Tang <geliangtang@...il.com> wrote:
> get_task_comm() copys the task's comm under the task_lock, it's safer
> than directly using memcpy().
>
> Signed-off-by: Geliang Tang <geliangtang@...il.com>
> ---
>  security/lsm_audit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> index 28d4c3a..555b1c4 100644
> --- a/security/lsm_audit.c
> +++ b/security/lsm_audit.c
> @@ -221,7 +221,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>         BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
>
>         audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current));
> -       audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
> +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
>
>         switch (a->type) {
>         case LSM_AUDIT_DATA_NONE:
> @@ -312,7 +312,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>                                 char comm[sizeof(tsk->comm)];
>                                 audit_log_format(ab, " opid=%d ocomm=", pid);
>                                 audit_log_untrustedstring(ab,
> -                                   memcpy(comm, tsk->comm, sizeof(comm)));
> +                                   get_task_comm(comm, tsk));

[NOTE: adding the linux-audit mailing list to this thread]

This isn't strictly a problem with this patch, but I think we should
be able to get rid of the 'comm' variable in this if-block as simply
reuse the 'comm' from the top of the function.  It would be nice to
include that in this patch.

Other than that minor nit, this patch looks good to me; if you make
that small change I'll merge it into the audit/next branch for the
upcoming merge window.

-- 
paul moore
www.paul-moore.com