Re: [PATCH] name_count array overrun

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

Steve Grubb wrote:  [Sun Sep 24 2006, 08:56:57AM EDT]
> On Thursday 07 September 2006 16:43, Amy Griffis wrote:
> > Did you consider just dropping any data encountered after we've filled
> > AUDIT_NAMES, instead of copying over the data for the last element?
> 
> OK, corrected patch follows.

Sorry for the delayed response, I've been out of town.

Do we want a similar printk in __audit_inode()?

> 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.18.x86_64.orig/kernel/auditsc.c 
> linux-2.6.18.x86_64/kernel/auditsc.c
> --- linux-2.6.18.x86_64.orig/kernel/auditsc.c	2006-09-24 
> 08:24:27.000000000 -0400
> +++ linux-2.6.18.x86_64/kernel/auditsc.c	2006-09-24 08:42:01.000000000 -0400
> @@ -1347,7 +1347,13 @@ void __audit_inode_child(const char *dna
>  		}
>  
>  update_context:
> -	idx = context->name_count++;
> +	idx = context->name_count;
> +	if (context->name_count == AUDIT_NAMES) {
> +		printk(KERN_DEBUG "name_count maxed and losing %s\n",
> +			found_name ?: "(null)");
> +		return;
> +	} 
> +	context->name_count++;
>  #if AUDIT_DEBUG
>  	context->ino_count++;
>  #endif
> @@ -1365,7 +1371,18 @@ 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) {
> +			printk(KERN_DEBUG 
> +				"name_count maxed and losing parent inode data: dev=%02x:%02x rdev=%02x:
> %02x, inode=%lu",
> +				MAJOR(parent->i_sb->s_dev),
> +				MINOR(parent->i_sb->s_dev),
> +				MAJOR(parent->i_rdev),
> +				MINOR(parent->i_rdev),
> +				parent->i_ino);
> +			return;
> +		}
> +		context->name_count++;
>  #if AUDIT_DEBUG
>  		context->ino_count++;
>  #endif
>