[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[PATCH] cleanups + fixes against audit.56



Hello,

This patch introduces fixes for:

1.  sys_rename() return code debacle
-> 
as a side effect of removing the error handling from fs/namei.c this bug was 
also removed

2.  leaky memory in auditfs_attach_wdata in failure path

3.  NULL dereference on audit_inode_free()
-> 
race could occur between the child inode being deleted and the watch being 
removed from parent

This patch adds:

1.  Implicit watc removal message with -1 loginuid

2.  New type, AUDIT_FS_INODE (1308)
-> 
now that we have watches per inode per record, we collect common inode 
information for the watch on AUDIT_FS_INODE and use AUDIT_FS_WATCH to list 
the watch information

3.  Minor code cleanups (eliminating pointless goto's)

What's left:

1.  Hooking chmod/chown/chgrp and the appropriate ACL calls (Me)

2.  Watch scalability problem (Me)

3.  AUID filtering on USER messages and watches (David)

4.  PATH record woes... add a new token stating "I'm the parent of the file or 
I'm the file"

-tim

diff -Nurp linux-2.6.9/fs/namei.c linux-2.6.9~56~tc2/fs/namei.c
--- linux-2.6.9/fs/namei.c	2005-06-15 17:57:52.124961352 -0500
+++ linux-2.6.9~56~tc2/fs/namei.c	2005-06-14 19:24:47.000000000 -0500
@@ -213,8 +213,7 @@ int permission(struct inode * inode,int 
 	int retval;
 	int submask;
 
-	if (audit_notify_watch(inode, mask))
-		return -ENOMEM;
+	audit_notify_watch(inode, mask);
 
 	/* Ordinary permission routines do not understand MAY_APPEND. */
 	submask = mask & ~MAY_APPEND;
@@ -332,8 +331,7 @@ static inline int exec_permission_lite(s
 	if (inode->i_op && inode->i_op->permission)
 		return -EAGAIN;
 
-	if (audit_notify_watch(inode, MAY_EXEC))
-		return -ENOMEM;
+	audit_notify_watch(inode, MAY_EXEC);
 
 	if (current->fsuid == inode->i_uid)
 		mode >>= 6;
@@ -1115,9 +1113,7 @@ static inline int may_delete(struct inod
 
 	BUG_ON(victim->d_parent->d_inode != dir);
 
-	error = audit_notify_watch(victim->d_inode, MAY_WRITE);
-	if (error)
-		return error;
+	audit_notify_watch(victim->d_inode, MAY_WRITE);
 
 	error = permission(dir,MAY_WRITE | MAY_EXEC, NULL);
 	if (error)
@@ -1244,9 +1240,8 @@ int vfs_create(struct inode *dir, struct
 		return error;
 	DQUOT_INIT(dir);
 	error = dir->i_op->create(dir, dentry, mode, nd);
-	if (!error)
-		error =	audit_notify_watch(dentry->d_inode, MAY_WRITE);
 	if (!error) {
+		audit_notify_watch(dentry->d_inode, MAY_WRITE);
 		inode_dir_notify(dir, DN_CREATE);
 		security_inode_post_create(dir, dentry, mode);
 	}
@@ -1560,9 +1555,8 @@ int vfs_mknod(struct inode *dir, struct 
 
 	DQUOT_INIT(dir);
 	error = dir->i_op->mknod(dir, dentry, mode, dev);
-	if (!error)
-		error = audit_notify_watch(dentry->d_inode, MAY_WRITE);
 	if (!error) {
+		audit_notify_watch(dentry->d_inode, MAY_WRITE);
 		inode_dir_notify(dir, DN_CREATE);
 		security_inode_post_mknod(dir, dentry, mode, dev);
 	}
@@ -1635,9 +1629,8 @@ int vfs_mkdir(struct inode *dir, struct 
 
 	DQUOT_INIT(dir);
 	error = dir->i_op->mkdir(dir, dentry, mode);
-	if (!error)
-		error = audit_notify_watch(dentry->d_inode, MAY_WRITE);
 	if (!error) {
+		audit_notify_watch(dentry->d_inode, MAY_WRITE);
 		inode_dir_notify(dir, DN_CREATE);
 		security_inode_post_mkdir(dir,dentry, mode);
 	}
@@ -1881,9 +1874,8 @@ int vfs_symlink(struct inode *dir, struc
 
 	DQUOT_INIT(dir);
 	error = dir->i_op->symlink(dir, dentry, oldname);
-	if (!error)
-		error = audit_notify_watch(dentry->d_inode, MAY_WRITE);
 	if (!error) {
+		audit_notify_watch(dentry->d_inode, MAY_WRITE);
 		inode_dir_notify(dir, DN_CREATE);
 		security_inode_post_symlink(dir, dentry, oldname);
 	}
@@ -1956,9 +1948,8 @@ int vfs_link(struct dentry *old_dentry, 
 	DQUOT_INIT(dir);
 	error = dir->i_op->link(old_dentry, dir, new_dentry);
 	up(&old_dentry->d_inode->i_sem);
-	if (!error)
-		error = audit_notify_watch(new_dentry->d_inode, MAY_WRITE);
 	if (!error) {
+		audit_notify_watch(new_dentry->d_inode, MAY_WRITE);
 		inode_dir_notify(dir, DN_CREATE);
 		security_inode_post_link(old_dentry, dir, new_dentry);
 	}
@@ -2080,13 +2071,12 @@ int vfs_rename_dir(struct inode *old_dir
 			d_rehash(new_dentry);
 		dput(new_dentry);
 	}
-	if (!error)
+	if (!error) {
 		d_move(old_dentry,new_dentry);
-	
-	error =	audit_notify_watch(old_dentry->d_inode, MAY_WRITE);
-	if (!error)
+		audit_notify_watch(old_dentry->d_inode, MAY_WRITE);
 		security_inode_post_rename(old_dir, old_dentry,
 					   new_dir, new_dentry);
+	}
 	return error;
 }
 
@@ -2112,12 +2102,9 @@ int vfs_rename_other(struct inode *old_d
 		/* The following d_move() should become unconditional */
 		if (!(old_dir->i_sb->s_type->fs_flags & FS_ODD_RENAME))
 			d_move(old_dentry, new_dentry);
-	}
-
-	error = audit_notify_watch(old_dentry->d_inode, MAY_WRITE);
-	if (!error)
+		audit_notify_watch(old_dentry->d_inode, MAY_WRITE);
 		security_inode_post_rename(old_dir, old_dentry, new_dir, new_dentry);
-
+	}
 	if (target)
 		up(&target->i_sem);
 	dput(new_dentry);
diff -Nurp linux-2.6.9/include/linux/audit.h linux-2.6.9~56~tc2/include/linux/audit.h
--- linux-2.6.9/include/linux/audit.h	2005-06-15 17:57:52.125961200 -0500
+++ linux-2.6.9~56~tc2/include/linux/audit.h	2005-06-14 19:24:47.000000000 -0500
@@ -75,6 +75,7 @@ struct atomic_t;
 #define AUDIT_CONFIG_CHANGE	1305	/* Audit system configuration change */
 #define AUDIT_SOCKADDR		1306	/* sockaddr copied as syscall arg */
 #define AUDIT_CWD		1307	/* Current working directory */
+#define AUDIT_FS_INODE		1308	/* File system inode */
 
 #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
 #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
diff -Nurp linux-2.6.9/kernel/auditfs.c linux-2.6.9~56~tc2/kernel/auditfs.c
--- linux-2.6.9/kernel/auditfs.c	2005-06-15 17:57:52.128960744 -0500
+++ linux-2.6.9~56~tc2/kernel/auditfs.c	2005-06-15 18:17:15.465106768 -0500
@@ -215,6 +215,7 @@ static inline void audit_watch_free(stru
 		kfree(watch->w_filterkey);
 		BUG_ON(!hlist_unhashed(&watch->w_node));
 		BUG_ON(!hlist_unhashed(&watch->w_master));
+		BUG_ON(!hlist_unhashed(&watch->w_watched));
 		kmem_cache_free(audit_watch_cache, watch);
 	}
 }
@@ -249,13 +250,12 @@ static inline struct audit_watch *audit_
 	watch->w_path[t->pathlen] = 0;
 	memcpy(watch->w_path, memblk + offset, t->pathlen);
 
-	goto audit_to_watch_exit;
+	return watch;
 
 audit_to_watch_fail:
 	audit_watch_free(watch);
-	watch = NULL;
 audit_to_watch_exit:
-	return watch;
+	return NULL;
 }
 
 /*
@@ -294,13 +294,20 @@ audit_to_transport_exit:
 static inline void audit_destroy_watch(struct audit_watch *watch)
 {
 	if (watch) {
-		hlist_del_init(&watch->w_watched);
-		hlist_del_init(&watch->w_master);
-		hlist_del_init(&watch->w_node);
-		audit_watch_put(watch);
-		audit_watch_put(watch);
-		audit_watch_put(watch);
+		if (!hlist_unhashed(&watch->w_watched)) {
+			hlist_del_init(&watch->w_watched);
+			audit_watch_put(watch);
+		}
+	
+		if (!hlist_unhashed(&watch->w_master)) {
+			hlist_del_init(&watch->w_master);
+			audit_watch_put(watch);
+		}
 
+		if (!hlist_unhashed(&watch->w_node)) {
+			hlist_del_init(&watch->w_node);
+			audit_watch_put(watch);
+		}
 	}
 }
 
@@ -310,8 +317,11 @@ static inline void audit_drain_watchlist
 	struct hlist_node *pos, *tmp;
 
 	hlist_for_each_entry_safe(watch, pos, tmp, &data->watchlist, w_node) {
+		spin_lock(&auditfs_lock);
 		audit_destroy_watch(watch);
+		spin_unlock(&auditfs_lock);
 		audit_data_pool_shrink();
+		audit_log(NULL, AUDIT_CONFIG_CHANGE, "auid=%u removed watch implicitly", -1);
 	}
 }
 
@@ -413,9 +423,9 @@ static inline int audit_insert_watch(str
 	watch->w_dev = nd.dentry->d_inode->i_sb->s_dev;
 
 	ret = 0;
-	audit_watch_get(watch);
 	spin_lock(&auditfs_lock);
 	hlist_add_head(&watch->w_node, &pdata->watchlist);
+	audit_watch_get(watch);
 	hlist_add_head(&watch->w_master, &master_watchlist);
 	spin_unlock(&auditfs_lock);
 
@@ -471,12 +481,11 @@ static inline int audit_remove_watch(str
 		spin_unlock(&auditfs_lock);
 		goto audit_remove_watch_release;
 	}
+	ret = 0;
 	audit_destroy_watch(real);
 	spin_unlock(&auditfs_lock);
 	audit_watch_put(real);
 
-	ret = 0;
-
 audit_remove_watch_release:
 	path_release(&nd);
 audit_remove_watch_exit:
@@ -583,7 +592,8 @@ void audit_update_watch(struct dentry *d
 	if (remove) {
 		me = audit_is_watched(watch, data);
 		if (me) {
-			hlist_del_init(&me->w_watched);
+			hlist_del(&watch->w_watched);
+			audit_watch_put(watch);
 			audit_watch_put(me);
 		}
 	} else {
@@ -649,7 +659,7 @@ audit_queue_watch_exit:
  */
 int audit_list_watches(int pid, int seq)
 {
-	int ret = 0;
+	int ret;
 	struct hlist_head skb_list;
 	struct hlist_node *tmp, *pos;
 	struct audit_skb_list *entry;
@@ -698,7 +708,7 @@ int audit_list_watches(int pid, int seq)
 	}
 
 	audit_send_reply(pid, seq, AUDIT_WATCH_LIST, 1, 1, NULL, 0);
-	goto audit_list_watches_exit;
+	return 0;
 
 audit_list_watches_fail:
 	hlist_for_each_entry_safe(entry, pos, tmp, &skb_list, list) {
@@ -706,7 +716,6 @@ audit_list_watches_fail:
 		kfree(entry->memblk);
 		kfree(entry);
 	}
-audit_list_watches_exit:
 	return ret;
 }
 
@@ -717,11 +726,15 @@ int audit_receive_watch(int type, int pi
 	struct audit_watch *watch = NULL;
 	char *payload = (char *)&req[1];
 
-	ret = -EINVAL;
-	if (req->pathlen > PATH_MAX || req->pathlen == 0)
+	ret = -ENAMETOOLONG;
+	if (req->pathlen >= PATH_MAX)
 		goto audit_receive_watch_exit;
 
-	if (req->fklen > AUDIT_FILTERKEY_MAX)
+	if (req->fklen >= AUDIT_FILTERKEY_MAX)
+		goto audit_receive_watch_exit;
+	
+	ret = -EINVAL;
+	if (req->pathlen == 0)
 		goto audit_receive_watch_exit;
 
 	if (payload[req->fklen] != '/')
@@ -769,19 +782,19 @@ void audit_inode_free(struct inode *inod
 
 		audit_drain_watchlist(data);
 		/* Release all our references to any watches we may have on us */
-        	hlist_for_each_entry_safe(watch, pos, tmp, &data->watches, w_watched) {
+		spin_lock(&auditfs_lock);
+		hlist_for_each_entry_safe(watch, pos, tmp, &data->watches, w_watched) {
 			hlist_del(&watch->w_watched);
                 	audit_watch_put(watch);
         	}
+		spin_unlock(&auditfs_lock);
 		audit_data_put(data);
 	}
 }
 
 int audit_filesystem_init(void)
 {
-	int ret;
 
-	ret = -ENOMEM;
 	audit_watch_cache =
 	    kmem_cache_create("audit_watch_cache",
 			      sizeof(struct audit_watch), 0, 0, NULL, NULL);
@@ -807,13 +820,11 @@ int audit_filesystem_init(void)
 
 	memset(auditfs_hash_table, 0, auditfs_cache_buckets * sizeof(void *));
 
-	ret = 0;
-	goto audit_filesystem_init_exit;
+	return 0;
 
 audit_filesystem_init_fail:
 	kmem_cache_destroy(audit_watch_cache);
-audit_filesystem_init_exit:
-	return ret;
+	return -ENOMEM;
 }
 
 
diff -Nurp linux-2.6.9/kernel/auditsc.c linux-2.6.9~56~tc2/kernel/auditsc.c
--- linux-2.6.9/kernel/auditsc.c	2005-06-15 17:57:52.129960592 -0500
+++ linux-2.6.9~56~tc2/kernel/auditsc.c	2005-06-15 18:48:57.016027088 -0500
@@ -586,7 +586,7 @@ static inline void audit_free_aux(struct
 			dput(axi->dentry);
 			mntput(axi->mnt);
 			break; }
-		case AUDIT_FS_WATCH: {
+		case AUDIT_FS_INODE: {
 			struct audit_aux_data_watched *axi = (void *)aux;
 			hlist_for_each_entry_safe(winfo, pos, tmp, &axi->watches, node) {
 				audit_watch_put(winfo->watch);
@@ -774,7 +774,7 @@ static void audit_log_exit(struct audit_
 			audit_log_d_path(ab, "path=", axi->dentry, axi->mnt);
 			break; }
 
-		case AUDIT_FS_WATCH: {
+		case AUDIT_FS_INODE: {
 			struct audit_aux_data_watched *axi = (void *)aux;
 			struct audit_buffer *sub_ab;
 			audit_log_format(ab,
@@ -1250,7 +1250,7 @@ int auditfs_attach_wdata(struct inode *i
 	ax->dev = inode->i_sb->s_dev;
 	ax->rdev = inode->i_rdev;
 
-	ax->link.type = AUDIT_FS_WATCH;
+	ax->link.type = AUDIT_FS_INODE;
 	ax->link.next = context->aux;
 	context->aux = (void *)ax;
 
@@ -1262,6 +1262,7 @@ auditfs_attach_wdata_fail:
 		audit_watch_put(this->watch);
 		kfree(this);
 	}
+	kfree(ax);
 
 	return -ENOMEM;
 }

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]