linux-kernel - Re: [PATCH ghak105 V2] audit: remove audit_context when CONFIG

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: <CAHC9VhQkLAgEUf6Rc6ovDyiJuNvm6PaKp0gGzDus6nkKDekWww@mail.gmail.com>
Date:   Tue, 29 Jan 2019 18:07:22 -0500
From:   Paul Moore <paul@...l-moore.com>
To:     Richard Guy Briggs <rgb@...hat.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Linux-Audit Mailing List <linux-audit@...hat.com>,
        Eric Paris <eparis@...hat.com>, Steve Grubb <sgrubb@...hat.com>
Subject: Re: [PATCH ghak105 V2] audit: remove audit_context when CONFIG_ AUDIT
 and not AUDITSYSCALL

On Mon, Jan 28, 2019 at 1:33 PM Richard Guy Briggs <rgb@...hat.com> wrote:
> Remove audit_context from struct task_struct and struct audit_buffer
> when CONFIG_AUDIT is enabled but CONFIG_AUDITSYSCALL is not.
>
> Also, audit_log_name() (and supporting inode and fcaps functions) should
> have been put back in auditsc.c when soft and hard link logging was
> normalized since it is only used by syscall auditing.
>
> See github issue https://github.com/linux-audit/audit-kernel/issues/105
>
> Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
> ---
> Changelog:
> v2:
> - resolve merge conflicts from rebase on upstreamed ghak103 patch
> - wrap task_struct audit_context in CONFIG_AUDITSYSCALL
>
>  include/linux/sched.h |   4 +-
>  kernel/audit.c        | 157 +++-----------------------------------------------
>  kernel/audit.h        |   9 ---
>  kernel/auditsc.c      | 150 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 161 insertions(+), 159 deletions(-)

...

> diff --git a/kernel/audit.c b/kernel/audit.c
> index 3f3f1888cac7..15e41603fd34 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -205,7 +205,9 @@ struct audit_net {
>   * use simultaneously. */
>  struct audit_buffer {
>         struct sk_buff       *skb;      /* formatted skb ready to send */
> +#ifdef CONFIG_AUDITSYSCALL
>         struct audit_context *ctx;      /* NULL or associated context */
> +#endif
>         gfp_t                gfp_mask;
>  };
>
> @@ -1696,7 +1698,9 @@ static struct audit_buffer *audit_buffer_alloc(struct audit_context *ctx,
>         if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
>                 goto err;
>
> +#ifdef CONFIG_AUDITSYSCALL
>         ab->ctx = ctx;
> +#endif

I vaguely remember reading/hearing something in the past about
kmem_cache_alloc() not returning a zero'd out buffer in all cases, can
you say for certain that "ab" in this case is always going to be
zero'd out?  This is an honest question.

If we can't guarantee that "ab" is zero'd out, we should manually set
ab->ctx to NULL when !CONFIG_AUDITSYSCALL.

>         ab->gfp_mask = gfp_mask;
>
>         return ab;
> @@ -1809,7 +1813,11 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
>                 return NULL;
>         }
>
> +#ifdef CONFIG_AUDITSYSCALL
>         audit_get_stamp(ab->ctx, &t, &serial);
> +#else
> +       audit_get_stamp(NULL, &t, &serial);
> +#endif

If ab->ctx is NULL we don't really need this, do we?

>         audit_log_format(ab, "audit(%llu.%03lu:%u): ",
>                          (unsigned long long)t.tv_sec, t.tv_nsec/1000000, serial);
>

-- 
paul moore
www.paul-moore.com