From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Viro Subject: Re: [PATCH 4/4] match audit name data Date: Mon, 19 Mar 2007 03:24:13 -0400 Message-ID: <20070319072413.GD11843@devserv.devel.redhat.com> References: <20070213191308.GA7536@fc.hp.com> <20070213191522.GE7536@fc.hp.com> <20070214180803.GA17068@fc.hp.com> <200703171902.32088.sgrubb@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <200703171902.32088.sgrubb@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: Steve Grubb Cc: linux-audit@redhat.com List-Id: linux-audit@redhat.com On Sat, Mar 17, 2007 at 07:02:31PM -0400, Steve Grubb wrote: > On Wednesday 14 February 2007 13:08:03 Amy Griffis wrote: > > Reposting with fixes for audit_inc_name_count() return value and > > better conditional context->name_count >= AUDIT_NAMES. Thanks Eric P. > > > Something is wrong with this patch as its causing slab corruption in the lspp > 65 and later kernels. I'll try to figure out what's wrong... I see one obviously wrong thing: we leave ->name and ->name_put alone in if (audit_inc_name_count(context, parent)) return; idx = context->name_count - 1; audit_copy_inode(&context->names[idx], parent); right after add_names. Note that when we do audit_syscall_exit() -> audit_free_names() we do *not* throw the context away. So ->name and ->name_put are left as-is since the before audit_free_names(), with ->name pointing to freed memory. So when we get to the quoted code, we advance ->name_count to entry that might very well have had stale ->name *and* non-zero ->name_put. Guess what happens when we do audit_free_names() again on that context? The rule we need to enforce is "anything past ->name_count is garbage". So we need to add context->names[idx].name = NULL; in there (->name_put is harmless after that). Another comment: for pity sake, simplify those boolean expressions. I mean, not only if (found_child || (!found_parent && !found_child)) { is if (found_child || !found_parent) { but in this case it's simply if (!found_parent) { since we can't get both found_parent and found_child non-NULL at the same time. The second one (several lines below) can go from if (found_parent || (!found_parent && !found_child)) { to if (found_parent || !found_child) { and to if (!found_child) { since again, non-NULL found_child means NULL found_parent. At which point code actually starts to make sense... --- kernel/auditsc.c 2007-03-19 02:36:00.000000000 -0400 +++ kernel/auditsc.c 2007-03-19 03:23:19.000000000 -0400 @@ -1404,14 +1404,15 @@ } add_names: - if (found_child || (!found_parent && !found_child)) { + if (!found_parent) { if (audit_inc_name_count(context, parent)) return; idx = context->name_count - 1; + context->names[idx].name = NULL; audit_copy_inode(&context->names[idx], parent); } - if (found_parent || (!found_parent && !found_child)) { + if (!found_child) { if (audit_inc_name_count(context, inode)) return; idx = context->name_count - 1; @@ -1424,6 +1425,8 @@ context->names[idx].name_len = AUDIT_NAME_FULL; /* don't call __putname() */ context->names[idx].name_put = 0; + } else { + context->names[idx].name = NULL; } if (inode)