On Thu, May 16, 2013 at 5:45 AM, Stephen Smalley <[email protected]> wrote:
> On 05/15/2013 09:52 PM, William Roberts wrote: > >> From eb7a0d5e711b555d38e3cd19c754e4**a866bb07a4 Mon Sep 17 00:00:00 2001 >>> >> From: William Roberts <[email protected]> >> Date: Wed, 15 May 2013 18:12:31 -0700 >> Subject: [PATCH] Enable splitting the logs to both auditd and kernel >> simultaneously >> >> Allow the audit subsystem to send audit events to both the kernel >> message buffer and auditd at the same time. >> >> Change-Id: I3107322c845a4cfb001352e152c08**66b8a73f02d >> Signed-off-by: William Roberts <[email protected]> >> > > The message included the patch in html (as well as plain) and used DOS > line encodings. See Documentation/**SubmittingPatches and > Documentation/email-clients.**txt for some tips or just send directly > using git send-email. > > I have two factor auth on my gmail, I need to generate a unique password for my git client. So I was lazy and copy + pasted it into an email. > Indentation is wrong; scripts/checkpatch.pl complains. > See Documentation/CodingStyle. > > Now, for more substantive comments: > > > --- >> include/linux/audit.h | 5 +++++ >> kernel/audit.c | 44 ++++++++++++++++++++++++++++++** >> +++++++++----- >> 2 files changed, 44 insertions(+), 5 deletions(-) >> >> diff --git a/include/linux/audit.h b/include/linux/audit.h >> index ed3ef19..ae8083e 100644 >> --- a/include/linux/audit.h >> +++ b/include/linux/audit.h >> @@ -314,6 +314,10 @@ enum { >> #define AUDIT_STATUS_PID 0x0004 >> #define AUDIT_STATUS_RATE_LIMIT 0x0008 >> #define AUDIT_STATUS_BACKLOG_LIMIT 0x0010 >> +#define AUDIT_STATUS_LOGSPLIT 0x0020 >> + /* Split log actions */ >> +#define AUDIT_LOGSPLIT_OFF 0 >> +#define AUDIT_LOGSPLIT_ON 1 >> /* Failure-to-log actions */ >> #define AUDIT_FAIL_SILENT 0 >> #define AUDIT_FAIL_PRINTK 1 >> @@ -359,6 +363,7 @@ struct audit_status { >> __u32 mask; /* Bit mask for valid entries */ >> __u32 enabled; /* 1 = enabled, 0 = disabled */ >> __u32 failure; /* Failure-to-log action */ >> + __u32 logsplit; /* Logsplit action */ >> __u32 pid; /* pid of auditd process */ >> __u32 rate_limit; /* messages rate limit (per second) */ >> __u32 backlog_limit; /* waiting messages limit */ >> > > Note that these definitions have moved in the mainline kernel to > include/uapi/linux/audit.h as part of splitting userspace API definitions > from kernel-internal definitions and prototypes. So you will need to > adjust the patch for mainline. > > > diff --git a/kernel/audit.c b/kernel/audit.c >> index 4096bcc..10f0457 100644 >> --- a/kernel/audit.c >> +++ b/kernel/audit.c >> @@ -74,6 +74,10 @@ static int audit_initialized; >> #define AUDIT_OFF 0 >> #define AUDIT_ON 1 >> #define AUDIT_LOCKED 2 >> + >> +#define AUDIT_PRINTK_NOHOLD 0 >> +#define AUDIT_PRINTK_HOLD 1 >> + >> > > These definitions don't seem to be used by your patch; leftovers of an > earlier version? > > Yes > > @@ -357,6 +364,16 @@ static int audit_set_failure(int state, uid_t >> loginuid, u32 sessionid, u32 sid) >> loginuid, sessionid, sid); >> } >> >> +static int audit_set_logsplit(int state, uid_t loginuid, u32 sessionid, >> u32 sid) >> +{ >> + if (state != AUDIT_LOGSPLIT_OFF >> + && state != AUDIT_LOGSPLIT_ON) >> + return -EINVAL; >> + >> + return audit_do_config_change("audit_**logsplit", &audit_logsplit, >> state, >> + loginuid, sessionid, sid); >> +} >> > > FYI, you won't need to pass down the loginuid, sessionid, or sid to this > function when you port to current mainline. No longer required by > audit_do_config_change(). > > Good to know... > >> >> @@ -725,6 +748,15 @@ static int audit_receive_msg(struct sk_buff *skb, >> struct nlmsghdr *nlh) >> if (status_get->mask & AUDIT_STATUS_BACKLOG_LIMIT) >> err = audit_set_backlog_limit(**status_get->backlog_limit, >> loginuid, sessionid, sid); >> + >> + if (status_get->mask & AUDIT_STATUS_LOGSPLIT) { >> + err = audit_set_logsplit(status_get-**>logsplit, >> + loginuid, sessionid, sid); >> + >> + if (err < 0) { >> + return err; >> + } >> + } >> > > Looks like this will lose any error from audit_set_backlog_limit(). So > you can either move your code before the AUDIT_STATUS_BACKLOG_LIMIT case, > or add an if (err < 0) return err; to that block. Also you should not use > { } around a single statement (as per Documentation/CodingStyle), so don't > do that in your if (err < 0)... statement. > > I caught this after I sent it out, already corrected. > > break; >> case AUDIT_USER: >> case AUDIT_FIRST_USER_MSG ... AUDIT_LAST_USER_MSG: >> @@ -1464,6 +1496,8 @@ void audit_log_end(struct audit_buffer *ab) >> nlh->nlmsg_len = ab->skb->len - NLMSG_SPACE(0); >> >> if (audit_pid) { >> + if(audit_logsplit == AUDIT_LOGSPLIT_ON) >> + __audit_printk_skb(ab->skb); >> skb_queue_tail(&audit_skb_**queue, ab->skb); >> wake_up_interruptible(&**kauditd_wait); >> } else { >> > > Coding style - put space between if and (. > > Will do... -- Respectfully, William C Roberts
