From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Paris Subject: Re: [PATCH] Audit: fix audit nlmsg_len from the kernel Date: Tue, 02 Jun 2009 16:38:02 -0400 Message-ID: <1243975082.2923.6.camel@dhcp231-142.rdu.redhat.com> References: <20090528231753.14980.15527.stgit@paris.rdu.redhat.com> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20090528231753.14980.15527.stgit@paris.rdu.redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: linux-audit@redhat.com Cc: viro@ZenIV.linux.org.uk List-Id: linux-audit@redhat.com On Thu, 2009-05-28 at 19:17 -0400, Eric Paris wrote: > Currently audit emits it's netlink record setting the nlmsg_len equal to the > size of the data in the netlink message rather than the size of the netlink > header plus the size of the data in the msg. nlmsg_end() happens to be a > nice function which calculates and sets nlmsg_len for you, so we use that > function to make sure we get it right from now on. > > The busted nlmsg_len doesn't hurt anything as long as sizeof(data) > > sizeof(nlmsg_hdr) and we don't pack mulitple netlink messages into one skb. > Since sizeof(nlmsg_hdr)=16 and we send (as it turns out by sheer dumb luck) at > least 16 bytes of audit timestamp into every message we aren't running into > problems. > > But lets fix it anyway rather than rely on dumb luck and implementation > details. Self NAK. Userspace was also written by someone who didn't know netlink. Since userspace is half using the netlink macros and half depending on this broken nlmsg_len implementation I don't think we can make any changes in the kernel and they wouldn't be backwards compatible... With this patch userspace prints crap on the end of the intended message. Not sure how I missed it at first.... I'm going to instead rewrite the userspace stuff to completely expect the broken behaviour rather than half netlink macros and half broken implementation.... uggghhhh Do not apply this patch. > > Signed-off-by: Eric Paris > --- > > kernel/audit.c | 13 ++++++++----- > 1 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/kernel/audit.c b/kernel/audit.c > index 9442c35..5c2ccef 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -1468,22 +1468,25 @@ void audit_log_end(struct audit_buffer *ab) > if (!audit_rate_check()) { > audit_log_lost("rate limit exceeded"); > } else { > - struct nlmsghdr *nlh = nlmsg_hdr(ab->skb); > - nlh->nlmsg_len = ab->skb->len - NLMSG_SPACE(0); > + struct sk_buff *skb = ab->skb; > + struct nlmsghdr *nlh = nlmsg_hdr(skb); > + > + /* correctly account for nlh->nlmsg_len */ > + nlmsg_end(skb, nlh); > > if (audit_pid) { > - skb_queue_tail(&audit_skb_queue, ab->skb); > + skb_queue_tail(&audit_skb_queue, skb); > wake_up_interruptible(&kauditd_wait); > } else { > if (nlh->nlmsg_type != AUDIT_EOE) { > if (printk_ratelimit()) { > printk(KERN_NOTICE "type=%d %s\n", > nlh->nlmsg_type, > - ab->skb->data + NLMSG_SPACE(0)); > + skb->data + NLMSG_SPACE(0)); > } else > audit_log_lost("printk limit exceeded\n"); > } > - audit_hold_skb(ab->skb); > + audit_hold_skb(skb); > } > ab->skb = NULL; > } >