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

[Steve Grubb <sgrubb@redhat.com>]

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...

-Steve


> Make more effort to detect previously collected names, so we don't log
> multiple PATH records for a single filesystem object. Add
> audit_inc_name_count() to reduce duplicate code.
>
> Signed-off-by: Amy Griffis <amy.griffis@hp.com>
> ---
>  kernel/auditsc.c |  140
> +++++++++++++++++++++++++++++++----------------------- 1 files changed, 81
> insertions(+), 59 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 6f9c14e..1b427d9 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -78,11 +78,6 @@ extern int audit_enabled;
>   * for saving names from getname(). */
>  #define AUDIT_NAMES    20
>
> -/* AUDIT_NAMES_RESERVED is the number of slots we reserve in the
> - * audit_context from being used for nameless inodes from
> - * path_lookup. */
> -#define AUDIT_NAMES_RESERVED 7
> -
>  /* Indicates that audit should log the full pathname. */
>  #define AUDIT_NAME_FULL -1
>
> @@ -1282,6 +1277,28 @@ void audit_putname(const char *name)
>  #endif
>  }
>
> +static int audit_inc_name_count(struct audit_context *context,
> +				const struct inode *inode)
> +{
> +	if (context->name_count >= AUDIT_NAMES) {
> +		if (inode)
> +			printk(KERN_DEBUG "name_count maxed, losing inode data: "
> +			       "dev=%02x:%02x, inode=%lu",
> +			       MAJOR(inode->i_sb->s_dev),
> +			       MINOR(inode->i_sb->s_dev),
> +			       inode->i_ino);
> +
> +		else
> +			printk(KERN_DEBUG "name_count maxed, losing inode data");
> +		return 1;
> +	}
> +	context->name_count++;
> +#if AUDIT_DEBUG
> +	context->ino_count++;
> +#endif
> +	return 0;
> +}
> +
>  /* Copy inode data into an audit_names. */
>  static void audit_copy_inode(struct audit_names *name, const struct inode
> *inode) {
> @@ -1319,13 +1336,10 @@ void __audit_inode(const char *name, const struct
> inode *inode) else {
>  		/* FIXME: how much do we care about inodes that have no
>  		 * associated name? */
> -		if (context->name_count >= AUDIT_NAMES - AUDIT_NAMES_RESERVED)
> +		if (audit_inc_name_count(context, inode))
>  			return;
> -		idx = context->name_count++;
> +		idx = context->name_count - 1;
>  		context->names[idx].name = NULL;
> -#if AUDIT_DEBUG
> -		++context->ino_count;
> -#endif
>  	}
>  	audit_copy_inode(&context->names[idx], inode);
>  }
> @@ -1349,69 +1363,77 @@ void __audit_inode_child(const char *dname, const
> struct inode *inode, {
>  	int idx;
>  	struct audit_context *context = current->audit_context;
> -	const char *found_name = NULL;
> +	const char *found_parent = NULL, *found_child = NULL;
>  	int dirlen = 0;
>
>  	if (!context->in_syscall)
>  		return;
>
> -	/* determine matching parent */
>  	if (!dname)
> -		goto update_context;
> -	for (idx = 0; idx < context->name_count; idx++)
> -		if (context->names[idx].ino == parent->i_ino) {
> -			const char *name = context->names[idx].name;
> +		goto add_names;
>
> -			if (!name)
> -				continue;
> +	/* parent is more likely, look for it first */
> +	for (idx = 0; idx < context->name_count; idx++) {
> +		struct audit_names *n = &context->names[idx];
>
> -			if (audit_compare_dname_path(dname, name, &dirlen) == 0) {
> -				context->names[idx].name_len = dirlen;
> -				found_name = name;
> -				break;
> -			}
> +		if (!n->name)
> +			continue;
> +
> +		if ((n->ino == parent->i_ino) &&
> +		    !audit_compare_dname_path(dname, n->name, &dirlen)) {
> +			n->name_len = dirlen; /* update parent data in place */
> +			found_parent = n->name;
> +			goto add_names;
>  		}
> +	}
>
> -update_context:
> -	idx = context->name_count;
> -	if (context->name_count == AUDIT_NAMES) {
> -		printk(KERN_DEBUG "name_count maxed and losing %s\n",
> -			found_name ?: "(null)");
> -		return;
> +	/* no matching parent, look for matching child */
> +	for (idx = 0; idx < context->name_count; idx++) {
> +		struct audit_names *n = &context->names[idx];
> +
> +		if (!n->name)
> +			continue;
> +
> +		/* strcmp() is the more likely scenario */
> +		if (!strcmp(dname, n->name) ||
> +		     !audit_compare_dname_path(dname, n->name, &dirlen)) {
> +			if (inode)
> +				audit_copy_inode(n, inode);
> +			else
> +				n->ino = (unsigned long)-1;
> +			found_child = n->name;
> +			goto add_names;
> +		}
>  	}
> -	context->name_count++;
> -#if AUDIT_DEBUG
> -	context->ino_count++;
> -#endif
> -	/* Re-use the name belonging to the slot for a matching parent directory.
> -	 * All names for this context are relinquished in audit_free_names() */
> -	context->names[idx].name = found_name;
> -	context->names[idx].name_len = AUDIT_NAME_FULL;
> -	context->names[idx].name_put = 0;	/* don't call __putname() */
> -
> -	if (!inode)
> -		context->names[idx].ino = (unsigned long)-1;
> -	else
> -		audit_copy_inode(&context->names[idx], inode);
> -
> -	/* A parent was not found in audit_names, so copy the inode data for the
> -	 * provided parent. */
> -	if (!found_name) {
> -		idx = context->name_count;
> -		if (context->name_count == AUDIT_NAMES) {
> -			printk(KERN_DEBUG
> -				"name_count maxed and losing parent inode data: dev=%02x:%02x,
> inode=%lu", -				MAJOR(parent->i_sb->s_dev),
> -				MINOR(parent->i_sb->s_dev),
> -				parent->i_ino);
> +
> +add_names:
> +	if (found_child || (!found_parent && !found_child)) {
> +		if (audit_inc_name_count(context, parent))
>  			return;
> -		}
> -		context->name_count++;
> -#if AUDIT_DEBUG
> -		context->ino_count++;
> -#endif
> +		idx = context->name_count - 1;
>  		audit_copy_inode(&context->names[idx], parent);
>  	}
> +
> +	if (found_parent || (!found_parent && !found_child)) {
> +		if (audit_inc_name_count(context, inode))
> +			return;
> +		idx = context->name_count - 1;
> +
> +		/* Re-use the name belonging to the slot for a matching parent
> +		 * directory. All names for this context are relinquished in
> +		 * audit_free_names() */
> +		if (found_parent) {
> +			context->names[idx].name = found_parent;
> +			context->names[idx].name_len = AUDIT_NAME_FULL;
> +			/* don't call __putname() */
> +			context->names[idx].name_put = 0;
> +		}
> +
> +		if (inode)
> +			audit_copy_inode(&context->names[idx], inode);
> +		else
> +			context->names[idx].ino = (unsigned long)-1;
> +	}
>  }
>
>  /**