[PATCH 0/4] audit obj cleanups

[PATCH 0/4] audit obj cleanups

[Amy Griffis]

A few patches to clean up auditing for syscall objects.

    - stop logging bogus object labels
    - handle edge cases for xattr and mqueue calls
    - try harder to log only 1 PATH record per object

These patches are based on audit.b36. Thanks for reviewing.

Amy

[PATCH 1/4] initialize name osid

[Amy Griffis]

Audit contexts can be reused, so initialize a name's osid to the
default in audit_getname(). This ensures we don't log a bogus object
label when no inode data is collected for a name.

Signed-off-by: Amy Griffis <amy.griffis@hp.com>
---
 kernel/auditsc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 3599558..b3f5cd6 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1228,6 +1228,7 @@ void __audit_getname(const char *name)
 	context->names[context->name_count].name_len = AUDIT_NAME_FULL;
 	context->names[context->name_count].name_put = 1;
 	context->names[context->name_count].ino  = (unsigned long)-1;
+	context->names[context->name_count].osid = 0;
 	++context->name_count;
 	if (!context->pwd) {
 		read_lock(&current->fs->lock);
-- 
1.4.4.4

[PATCH 2/4] audit inode for all xattr syscalls

[Amy Griffis]

Collect inode info for the remaining xattr syscalls that operate on a file
descriptor. These don't call a path_lookup variant, so they aren't covered by
the general audit hook.

Signed-off-by: Amy Griffis <amy.griffis@hp.com>
---
 fs/xattr.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index 3864613..4f21fc9 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -346,11 +346,14 @@ asmlinkage ssize_t
 sys_fgetxattr(int fd, char __user *name, void __user *value, size_t size)
 {
 	struct file *f;
+	struct dentry *dentry;
 	ssize_t error = -EBADF;
 
 	f = fget(fd);
 	if (!f)
 		return error;
+	dentry = f->f_path.dentry;
+	audit_inode(NULL, dentry->d_inode);
 	error = getxattr(f->f_path.dentry, name, value, size);
 	fput(f);
 	return error;
@@ -418,11 +421,14 @@ asmlinkage ssize_t
 sys_flistxattr(int fd, char __user *list, size_t size)
 {
 	struct file *f;
+	struct dentry *dentry;
 	ssize_t error = -EBADF;
 
 	f = fget(fd);
 	if (!f)
 		return error;
+	dentry = f->f_path.dentry;
+	audit_inode(NULL, dentry->d_inode);
 	error = listxattr(f->f_path.dentry, list, size);
 	fput(f);
 	return error;
-- 
1.4.4.4

[PATCH 3/4] complete message queue auditing

[Amy Griffis]

Handle the edge cases for POSIX message queue auditing. Collect inode
info when opening an existing mq, and for send/receive operations. Remove
audit_inode_update() as it has really evolved into the equivalent of
audit_inode().

Signed-off-by: Amy Griffis <amy.griffis@hp.com>
---
 fs/namei.c            |    2 +-
 include/linux/audit.h |    7 -------
 ipc/mqueue.c          |    4 ++++
 kernel/auditsc.c      |   27 ---------------------------
 4 files changed, 5 insertions(+), 35 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 161e222..3cddefb 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1701,7 +1701,7 @@ do_last:
 	 * It already exists.
 	 */
 	mutex_unlock(&dir->d_inode->i_mutex);
-	audit_inode_update(path.dentry->d_inode);
+	audit_inode(pathname, path.dentry->d_inode);
 
 	error = -EEXIST;
 	if (flag & O_EXCL)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 229fa01..aa205cd 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -350,7 +350,6 @@ extern void audit_putname(const char *name);
 extern void __audit_inode(const char *name, const struct inode *inode);
 extern void __audit_inode_child(const char *dname, const struct inode *inode,
 				const struct inode *parent);
-extern void __audit_inode_update(const struct inode *inode);
 static inline int audit_dummy_context(void)
 {
 	void *p = current->audit_context;
@@ -371,10 +370,6 @@ static inline void audit_inode_child(const char *dname,
 	if (unlikely(!audit_dummy_context()))
 		__audit_inode_child(dname, inode, parent);
 }
-static inline void audit_inode_update(const struct inode *inode) {
-	if (unlikely(!audit_dummy_context()))
-		__audit_inode_update(inode);
-}
 
 				/* Private API (for audit.c only) */
 extern unsigned int audit_serial(void);
@@ -456,10 +451,8 @@ extern int audit_n_rules;
 #define audit_putname(n) do { ; } while (0)
 #define __audit_inode(n,i) do { ; } while (0)
 #define __audit_inode_child(d,i,p) do { ; } while (0)
-#define __audit_inode_update(i) do { ; } while (0)
 #define audit_inode(n,i) do { ; } while (0)
 #define audit_inode_child(d,i,p) do { ; } while (0)
-#define audit_inode_update(i) do { ; } while (0)
 #define auditsc_get_stamp(c,t,s) do { BUG(); } while (0)
 #define audit_get_loginuid(c) ({ -1; })
 #define audit_log_task_context(b) do { ; } while (0)
diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 7a8ce61..84cf05d 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -682,6 +682,7 @@ asmlinkage long sys_mq_open(const char __user *u_name, int oflag, mode_t mode,
 
 	if (oflag & O_CREAT) {
 		if (dentry->d_inode) {	/* entry already exists */
+        		audit_inode(name, dentry->d_inode);
 			error = -EEXIST;
 			if (oflag & O_EXCL)
 				goto out;
@@ -694,6 +695,7 @@ asmlinkage long sys_mq_open(const char __user *u_name, int oflag, mode_t mode,
 		error = -ENOENT;
 		if (!dentry->d_inode)
 			goto out;
+        	audit_inode(name, dentry->d_inode);
 		filp = do_open(dentry, oflag);
 	}
 
@@ -840,6 +842,7 @@ asmlinkage long sys_mq_timedsend(mqd_t mqdes, const char __user *u_msg_ptr,
 	if (unlikely(filp->f_op != &mqueue_file_operations))
 		goto out_fput;
 	info = MQUEUE_I(inode);
+	audit_inode(NULL, inode);
 
 	if (unlikely(!(filp->f_mode & FMODE_WRITE)))
 		goto out_fput;
@@ -923,6 +926,7 @@ asmlinkage ssize_t sys_mq_timedreceive(mqd_t mqdes, char __user *u_msg_ptr,
 	if (unlikely(filp->f_op != &mqueue_file_operations))
 		goto out_fput;
 	info = MQUEUE_I(inode);
+	audit_inode(NULL, inode);
 
 	if (unlikely(!(filp->f_mode & FMODE_READ)))
 		goto out_fput;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index b3f5cd6..6f9c14e 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1415,33 +1415,6 @@ update_context:
 }
 
 /**
- * audit_inode_update - update inode info for last collected name
- * @inode: inode being audited
- *
- * When open() is called on an existing object with the O_CREAT flag, the inode
- * data audit initially collects is incorrect.  This additional hook ensures
- * audit has the inode data for the actual object to be opened.
- */
-void __audit_inode_update(const struct inode *inode)
-{
-	struct audit_context *context = current->audit_context;
-	int idx;
-
-	if (!context->in_syscall || !inode)
-		return;
-
-	if (context->name_count == 0) {
-		context->name_count++;
-#if AUDIT_DEBUG
-		context->ino_count++;
-#endif
-	}
-	idx = context->name_count - 1;
-
-	audit_copy_inode(&context->names[idx], inode);
-}
-
-/**
  * auditsc_get_stamp - get local copies of audit_context values
  * @ctx: audit_context for the task
  * @t: timespec to store time recorded in the audit_context
-- 
1.4.4.4

[PATCH 4/4] match audit name data

[Amy Griffis]

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..f9b140b 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 0;
+	}
+	context->name_count++;
+#if AUDIT_DEBUG
+	context->ino_count++;
+#endif
+	return 1;
+}
+
 /* 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;
+	}
 }
 
 /**
-- 
1.4.4.4

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

[Amy Griffis]

Reposting with fixes for audit_inc_name_count() return value and
better conditional context->name_count >= AUDIT_NAMES. Thanks Eric P.

--

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;
+	}
 }
 
 /**
-- 
1.4.4.4

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

[Steve Grubb]

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;
> +	}
>  }
>
>  /**

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

[Alexander Viro]

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)

[PATCH 4/4] match audit name data

[Amy Griffis]

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 |  143 +++++++++++++++++++++++++++++++----------------------
 1 files changed, 84 insertions(+), 59 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index e241541..3cfa38f 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
 
@@ -1280,6 +1275,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)
 {
@@ -1317,13 +1334,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);
 }
@@ -1347,69 +1361,80 @@ 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_parent) {
+		if (audit_inc_name_count(context, parent))
 			return;
-		}
-		context->name_count++;
-#if AUDIT_DEBUG
-		context->ino_count++;
-#endif
+		idx = context->name_count - 1;
+		context->names[idx].name = NULL;
 		audit_copy_inode(&context->names[idx], parent);
 	}
+
+	if (!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;
+		} else {
+			context->names[idx].name = NULL;
+		}
+
+		if (inode)
+			audit_copy_inode(&context->names[idx], inode);
+		else
+			context->names[idx].ino = (unsigned long)-1;
+	}
 }
 
 /**
-- 
1.4.4.4