Re: [PATCH 4/4] match audit name data

[Alexander Viro <aviro@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)