[Crash-utility] feature to dump audit logs in vmcore

[Hatayama, Daisuke]

> -----Original Message-----
> From: crash-utility-bounces at redhat.com
> [mailto:crash-utility-bounces at redhat.com] On Behalf Of Dave Anderson
> Sent: Tuesday, March 14, 2017 4:18 AM
> To: Discussion list for crash utility usage, maintenance and development
> <crash-utility at redhat.com>
> Subject: Re: [Crash-utility] feature to dump audit logs in vmcore
> 
> 
> 
> ----- Original Message -----
> 
> > I've written the first version of the patch adding a feature to dump kernel
> > audit logs as log -a.
> > Could you review this patch?
> >
> > I made this patch on top of today's latest commit on github crash utility
> > repository:
> >
> >
> https://github.com/crash-utility/crash/commit/ed60e97e319a1cfc9e2779aa1baa
> c305677393d8
> >
> > Thanks.
> > HATAYAMA, Daisuke
> >
> 
> ----- Original Message -----
> 
> > What the extension module does is so simple that it retrives audit
> > logs from a queue. Looking back into kernel git logs, this design that
> > audit logs are held in the queue, was introduced at the following
> > patch to introduce kauditd kernel thread and have never changed until
> > now:
> >
> >     # git describe b7d1125817c9a46cc46f57db89d9c195e7af22f8
> >     v2.6.12-rc4-48-gb7d1125
> >
> >     # git log -1 -p b7d1125817c9a46cc46f57db89d9c195e7af22f8
> >     commit b7d1125817c9a46cc46f57db89d9c195e7af22f8
> >     Author: David Woodhouse <dwmw2 shinybook infradead org>
> >     Date:   Thu May 19 10:56:58 2005 +0100
> >
> >         AUDIT: Send netlink messages from a separate kernel thread
> >
> >         netlink_unicast() will attempt to reallocate and will free messages
> if
> >         the socket's rcvbuf limit is reached unless we give it an infinite
> >         timeout. So do that, from a kernel thread which is dedicated to
> spewing
> >         stuff up the netlink socket.
> >
> >         Signed-off-by: David Woodhouse <dwmw2 infradead org>
> >
> > I confirmed the comamnd works well on fedora 4.8 kernel, RHEL7.3,
> > RHEL6.8 and RHEL5.11; RHEL6.8 was tested both on x86_64 and x86_32.
> 
> I tested this on ~250 sample vmcores I keep around, but absolutely none of them
> had any audit data stored.  So "log -a" either quietly did nothing, or in a
> handful
> of cases, failed with "log: -a option not supported or applicable on this
> architecture
> or kernel".  Perhaps it would be helpful if you could also print a message if
> there
> is no audit data stored in the kernel?

Thanks for your reviewing.

Sure, I add such message such as ''kernel audit buffer is empty''.

> 
> Also, with respect to "have never changed until now", your patch does this:

Sorry, 'never' was not sutable.

I mean the relevant data structures such as sk_buff and the way handling them
are not changed. A kind of queues and their handling have varied a little.
So, I wanted to say it's not so much complicated to implement dumping feature
of kernel audit logs.

> 
>   +static void
>   +__dump_audit(char *symname)
>   +{
>   +       if (symbol_exists(symname)) {
>   +               if (CRASHDEBUG(1))
>   +                       fprintf(fp, "# %s:\n", symname);
>   +               dump_audit_skb_queue(symbol_value(symname));
>   +       }
>   +}
>   +
>   +static void
>   +dump_audit(void)
>   +{
>   +       if (!symbol_exists("audit_skb_queue"))
>   +               option_not_supported('a');
>   +
>   +       __dump_audit("audit_skb_hold_queue");
>   +       __dump_audit("audit_skb_queue");
>   +}
> 
> But in 4.10, the command fails due to this commit, which changed the names
> of both of those two symbols above:
> 
>   commit af8b824f283de5acc9b9ae8dbb60e4adacff721b
>   Author: Paul Moore <paul at paul-moore.com>
>   Date:   Tue Nov 29 16:53:24 2016 -0500
> 
>       audit: rename the queues and kauditd related functions
> 
>       The audit queue names can be shortened and the record sending
>       helpers associated with the kauditd task could be named better, do
>       these small cleanups now to make life easier once we start reworking
>       the queues and kauditd code.
> 
>       Signed-off-by: Paul Moore <paul at paul-moore.com>
> 
>   diff --git a/kernel/audit.c b/kernel/audit.c
>   index 801247a..6ac1df1 100644
>   --- a/kernel/audit.c
>   +++ b/kernel/audit.c
>   @@ -138,9 +138,9 @@
>    static int	   audit_freelist_count;
>    static LIST_HEAD(audit_freelist);
> 
>   -static struct sk_buff_head audit_skb_queue;
>   +static struct sk_buff_head audit_queue;
>    /* queue of skbs to send to auditd when/if it comes back */
>   -static struct sk_buff_head audit_skb_hold_queue;
>   +static struct sk_buff_head audit_hold_queue;
>    static struct task_struct *kauditd_task;
>    static DECLARE_WAIT_QUEUE_HEAD(kauditd_wait);
>    static DECLARE_WAIT_QUEUE_HEAD(audit_backlog_wait);
>   ...
> 
> I don't know what else may have changed with the patch above, but the
> "reworking the queuees and kauditd code" comment above sounds like the
> whole infrastructure is due for some signficant changes soon.  In any
> case, it would be unfortunate to check in a new option that doesn't at
> least work on the current stable upstream kernel.

I've noticed this for the first time. I'll look into this.

As the first look, it looks to me that this patch also doesn't change data
structures and the way of handling them. It may be enough to adapt to
changes of symbol names and addition of new kind of audit queues.

> 
> With respect to the size_table changes, I understand why you might want
> to store the structure member sizes of nlmsghdr.nlmsg_len, nlmsghdr.nlmsg_type
> and and sk_buff_head.qlen, but it's overkill to bother storing the sizes of
> sk_buff_head.next, sk_buff.data and sk_buff.next:
> 
>   @@ -2126,6 +2132,13 @@ struct size_table {         /* stash of commonly-used
> sizes */
>           long task_struct_flags;
>           long timer_base;
>           long taint_flag;
>   +       long nlmsghdr;
>   +       long nlmsghdr_nlmsg_len;
>   +       long nlmsghdr_nlmsg_type;
>   +       long sk_buff_head_next;
>   +       long sk_buff_head_qlen;
>   +       long sk_buff_data;
>   +       long sk_buff_next;
>    };
> 
> Given that those 3 members are all pointer types, you could just use
> "sizeof(void *)" for the readmem() calls, as is done throughout the
> crash code.  Note that there are not any MEMBER_SIZE_INIT() callers
> for structure members that are known, guaranteed, pointer values.

I'm doing this considering x86 build as x86_64 binary where pointer size
differs, or is this unnecessary for such x86 build? Sorry, I didn't test
x86 build as x86_64 binary.

> 
> Also, can you please display the new offset_table[] and size_table[]
> members in dump_offset_table() for use by "help -o"?

I see. I totally overlooked this.

Thanks.
HATAYAMA, Daisuke