Re: [PATCH] name_count array overrun

[Amy Griffis <amy.griffis@hp.com>]

Steve Grubb wrote:  [Thu Sep 07 2006, 02:00:06PM EDT]
> Hello,
> 
> The below patch closes an unbounded use of name_count. This can lead to oopses
> in some new file systems.
> 
> Signed-off-by: Steve Grubb <sgrubb@redhat.com>
> 
> 
> diff -urp linux-2.6.17.x86_64.orig/kernel/auditsc.c linux-2.6.17.x86_64/kernel/auditsc.c
> --- linux-2.6.17.x86_64.orig/kernel/auditsc.c	2006-08-29 11:21:20.000000000 -0400
> +++ linux-2.6.17.x86_64/kernel/auditsc.c	2006-08-29 15:15:28.000000000 -0400
> @@ -1281,7 +1281,15 @@ void __audit_inode(const char *name, con
>  		 * associated name? */
>  		if (context->name_count >= AUDIT_NAMES - AUDIT_NAMES_RESERVED)
>  			return;

What about this conditional, which translates to context->name_count >= 13?
Leaving the code as is means we'll never reach the new printk below,
where context->name_count == 19.

> -		idx = context->name_count++;
> +		idx = context->name_count;
> +		if (context->name_count == (AUDIT_NAMES - 1)) {
> +			printk(KERN_DEBUG
> +				"name_count maxed and losing entry [%d]=%s\n",
> +				context->name_count, 
> +				context->names[context->name_count].name ?:
> +				"(null)");

This is a little misleading, since the first time we hit it, we
haven't lost anything yet. We're only losing data on the second and
following times we hit it.

Did you consider just dropping any data encountered after we've filled
AUDIT_NAMES, instead of copying over the data for the last element?

> +		} else
> +			context->name_count++;
>  		context->names[idx].name = NULL;
>  #if AUDIT_DEBUG
>  		++context->ino_count;
> @@ -1333,7 +1341,13 @@ void __audit_inode_child(const char *dna
>  		}
>  
>  update_context:
> -	idx = context->name_count++;
> +	idx = context->name_count;
> +	if (context->name_count == (AUDIT_NAMES - 1)) {
> +		printk(KERN_DEBUG "name_count maxed and losing entry [%d]=%s\n",
> +			context->name_count, 
> +			context->names[context->name_count].name ?: "(null)");
> +	} else
> +		context->name_count++;
>  #if AUDIT_DEBUG
>  	context->ino_count++;
>  #endif
> @@ -1351,7 +1365,15 @@ update_context:
>  	/* A parent was not found in audit_names, so copy the inode data for the
>  	 * provided parent. */
>  	if (!found_name) {
> -		idx = context->name_count++;
> +		idx = context->name_count;
> +		if (context->name_count == (AUDIT_NAMES - 1)) {
> +			printk(KERN_DEBUG 
> +				"name_count maxed and losing entry [%d]=%s\n",
> +				context->name_count, 
> +				context->names[context->name_count].name ?:
> +				"(null)");
> +		} else
> +			context->name_count++;
>  #if AUDIT_DEBUG
>  		context->ino_count++;
>  #endif
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
>