[2/6] audit: Fix possible spurious -ENOSPC error
diff mbox

Message ID 20180628164014.4925-3-jack@suse.cz
State New
Headers show

Commit Message

Jan Kara June 28, 2018, 4:40 p.m. UTC
When an inode is tagged with a tree, tag_chunk() checks whether there is
audit_tree_group mark attached to the inode and adds one if not. However
nothing protects another tag_chunk() to add the mark between we've
checked and try to add the fsnotify mark thus resulting in an error from
fsnotify_add_mark() and consequently an ENOSPC error from tag_chunk().

Fix the problem by holding mark_mutex over the whole check-insert code
sequence.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 kernel/audit_tree.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

Comments

Amir Goldstein June 29, 2018, 11:42 a.m. UTC | #1
On Thu, Jun 28, 2018 at 7:40 PM, Jan Kara <jack@suse.cz> wrote:
> When an inode is tagged with a tree, tag_chunk() checks whether there is
> audit_tree_group mark attached to the inode and adds one if not. However
> nothing protects another tag_chunk() to add the mark between we've
> checked and try to add the fsnotify mark thus resulting in an error from
> fsnotify_add_mark() and consequently an ENOSPC error from tag_chunk().
>
> Fix the problem by holding mark_mutex over the whole check-insert code
> sequence.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---

Never dived into the audit_tree code, but this looks sane.

Thanks,
Amir.
Dan Carpenter July 2, 2018, 6:05 a.m. UTC | #2
Hi Jan,

url:    https://github.com/0day-ci/linux/commits/Jan-Kara/audit-Fix-various-races-when-tagging-and-untagging-mounts/20180629-043337

smatch warnings:
kernel/audit_tree.c:484 tag_chunk() warn: inconsistent returns 'mutex:&audit_tree_group->mark_mutex'.
  Locked on:   line 400
  Unlocked on: line 411

# https://github.com/0day-ci/linux/commit/86c9c9a738e409c85891519c17d94043b7f434d5
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 86c9c9a738e409c85891519c17d94043b7f434d5
vim +484 kernel/audit_tree.c

74c3cbe33 Al Viro         2007-07-22  386  
74c3cbe33 Al Viro         2007-07-22  387  /* the first tagged inode becomes root of tree */
74c3cbe33 Al Viro         2007-07-22  388  static int tag_chunk(struct inode *inode, struct audit_tree *tree)
74c3cbe33 Al Viro         2007-07-22  389  {
e61ce8673 Eric Paris      2009-12-17  390  	struct fsnotify_mark *old_entry, *chunk_entry;
74c3cbe33 Al Viro         2007-07-22  391  	struct audit_tree *owner;
74c3cbe33 Al Viro         2007-07-22  392  	struct audit_chunk *chunk, *old;
74c3cbe33 Al Viro         2007-07-22  393  	struct node *p;
74c3cbe33 Al Viro         2007-07-22  394  	int n;
74c3cbe33 Al Viro         2007-07-22  395  
86c9c9a73 Jan Kara        2018-06-28  396  	mutex_lock(&audit_tree_group->mark_mutex);
b1362edfe Jan Kara        2016-12-21  397  	old_entry = fsnotify_find_mark(&inode->i_fsnotify_marks,
b1362edfe Jan Kara        2016-12-21  398  				       audit_tree_group);
28a3a7eb3 Eric Paris      2009-12-17  399  	if (!old_entry)
74c3cbe33 Al Viro         2007-07-22  400  		return create_chunk(inode, tree);
                                                               ^^^^^^^^^^^^^^^^^^^^^^^^^
Should we drop the lock before this return?

74c3cbe33 Al Viro         2007-07-22  401  
28a3a7eb3 Eric Paris      2009-12-17  402  	old = container_of(old_entry, struct audit_chunk, mark);
74c3cbe33 Al Viro         2007-07-22  403  
74c3cbe33 Al Viro         2007-07-22  404  	/* are we already there? */
74c3cbe33 Al Viro         2007-07-22  405  	spin_lock(&hash_lock);
74c3cbe33 Al Viro         2007-07-22  406  	for (n = 0; n < old->count; n++) {
74c3cbe33 Al Viro         2007-07-22  407  		if (old->owners[n].owner == tree) {
74c3cbe33 Al Viro         2007-07-22  408  			spin_unlock(&hash_lock);
86c9c9a73 Jan Kara        2018-06-28  409  			mutex_unlock(&audit_tree_group->mark_mutex);
28a3a7eb3 Eric Paris      2009-12-17  410  			fsnotify_put_mark(old_entry);
74c3cbe33 Al Viro         2007-07-22  411  			return 0;
74c3cbe33 Al Viro         2007-07-22  412  		}
74c3cbe33 Al Viro         2007-07-22  413  	}
74c3cbe33 Al Viro         2007-07-22  414  	spin_unlock(&hash_lock);
74c3cbe33 Al Viro         2007-07-22  415  
74c3cbe33 Al Viro         2007-07-22  416  	chunk = alloc_chunk(old->count + 1);
b4c30aad3 Al Viro         2009-12-19  417  	if (!chunk) {
86c9c9a73 Jan Kara        2018-06-28  418  		mutex_unlock(&audit_tree_group->mark_mutex);
28a3a7eb3 Eric Paris      2009-12-17  419  		fsnotify_put_mark(old_entry);
74c3cbe33 Al Viro         2007-07-22  420  		return -ENOMEM;
b4c30aad3 Al Viro         2009-12-19  421  	}
74c3cbe33 Al Viro         2007-07-22  422  
28a3a7eb3 Eric Paris      2009-12-17  423  	chunk_entry = &chunk->mark;
28a3a7eb3 Eric Paris      2009-12-17  424  
6b3f05d24 Jan Kara        2016-12-21  425  	/*
6b3f05d24 Jan Kara        2016-12-21  426  	 * mark_mutex protects mark from getting detached and thus also from
36f10f55f Amir Goldstein  2018-06-23  427  	 * mark->connector->obj getting NULL.
6b3f05d24 Jan Kara        2016-12-21  428  	 */
43471d15d Jan Kara        2017-04-03  429  	if (!(old_entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) {
28a3a7eb3 Eric Paris      2009-12-17  430  		/* old_entry is being shot, lets just lie */
86c9c9a73 Jan Kara        2018-06-28  431  		mutex_unlock(&audit_tree_group->mark_mutex);
28a3a7eb3 Eric Paris      2009-12-17  432  		fsnotify_put_mark(old_entry);
7b1293234 Jan Kara        2016-12-21  433  		fsnotify_put_mark(&chunk->mark);
28a3a7eb3 Eric Paris      2009-12-17  434  		return -ENOENT;
28a3a7eb3 Eric Paris      2009-12-17  435  	}
28a3a7eb3 Eric Paris      2009-12-17  436  
36f10f55f Amir Goldstein  2018-06-23  437  	if (fsnotify_add_mark_locked(chunk_entry, old_entry->connector->obj,
36f10f55f Amir Goldstein  2018-06-23  438  				     FSNOTIFY_OBJ_TYPE_INODE, 1)) {
86c9c9a73 Jan Kara        2018-06-28  439  		mutex_unlock(&audit_tree_group->mark_mutex);
0fe33aae0 Miklos Szeredi  2012-08-15  440  		fsnotify_put_mark(chunk_entry);
28a3a7eb3 Eric Paris      2009-12-17  441  		fsnotify_put_mark(old_entry);
74c3cbe33 Al Viro         2007-07-22  442  		return -ENOSPC;
74c3cbe33 Al Viro         2007-07-22  443  	}
28a3a7eb3 Eric Paris      2009-12-17  444  
74c3cbe33 Al Viro         2007-07-22  445  	spin_lock(&hash_lock);
74c3cbe33 Al Viro         2007-07-22  446  	if (tree->goner) {
74c3cbe33 Al Viro         2007-07-22  447  		spin_unlock(&hash_lock);
74c3cbe33 Al Viro         2007-07-22  448  		chunk->dead = 1;
86c9c9a73 Jan Kara        2018-06-28  449  		mutex_unlock(&audit_tree_group->mark_mutex);
28a3a7eb3 Eric Paris      2009-12-17  450  
e2a29943e Lino Sanfilippo 2011-06-14  451  		fsnotify_destroy_mark(chunk_entry, audit_tree_group);
28a3a7eb3 Eric Paris      2009-12-17  452  
28a3a7eb3 Eric Paris      2009-12-17  453  		fsnotify_put_mark(chunk_entry);
28a3a7eb3 Eric Paris      2009-12-17  454  		fsnotify_put_mark(old_entry);
74c3cbe33 Al Viro         2007-07-22  455  		return 0;
74c3cbe33 Al Viro         2007-07-22  456  	}
74c3cbe33 Al Viro         2007-07-22  457  	list_replace_init(&old->trees, &chunk->trees);
74c3cbe33 Al Viro         2007-07-22  458  	for (n = 0, p = chunk->owners; n < old->count; n++, p++) {
74c3cbe33 Al Viro         2007-07-22  459  		struct audit_tree *s = old->owners[n].owner;
74c3cbe33 Al Viro         2007-07-22  460  		p->owner = s;
74c3cbe33 Al Viro         2007-07-22  461  		p->index = old->owners[n].index;
74c3cbe33 Al Viro         2007-07-22  462  		if (!s) /* result of fallback in untag */
74c3cbe33 Al Viro         2007-07-22  463  			continue;
74c3cbe33 Al Viro         2007-07-22  464  		get_tree(s);
74c3cbe33 Al Viro         2007-07-22  465  		list_replace_init(&old->owners[n].list, &p->list);
74c3cbe33 Al Viro         2007-07-22  466  	}
74c3cbe33 Al Viro         2007-07-22  467  	p->index = (chunk->count - 1) | (1U<<31);
74c3cbe33 Al Viro         2007-07-22  468  	p->owner = tree;
74c3cbe33 Al Viro         2007-07-22  469  	get_tree(tree);
74c3cbe33 Al Viro         2007-07-22  470  	list_add(&p->list, &tree->chunks);
74c3cbe33 Al Viro         2007-07-22  471  	list_replace_rcu(&old->hash, &chunk->hash);
74c3cbe33 Al Viro         2007-07-22  472  	list_for_each_entry(owner, &chunk->trees, same_root)
74c3cbe33 Al Viro         2007-07-22  473  		owner->root = chunk;
74c3cbe33 Al Viro         2007-07-22  474  	old->dead = 1;
74c3cbe33 Al Viro         2007-07-22  475  	if (!tree->root) {
74c3cbe33 Al Viro         2007-07-22  476  		tree->root = chunk;
74c3cbe33 Al Viro         2007-07-22  477  		list_add(&tree->same_root, &chunk->trees);
74c3cbe33 Al Viro         2007-07-22  478  	}
74c3cbe33 Al Viro         2007-07-22  479  	spin_unlock(&hash_lock);
86c9c9a73 Jan Kara        2018-06-28  480  	mutex_unlock(&audit_tree_group->mark_mutex);
e2a29943e Lino Sanfilippo 2011-06-14  481  	fsnotify_destroy_mark(old_entry, audit_tree_group);
b3e8692b4 Miklos Szeredi  2012-08-15  482  	fsnotify_put_mark(chunk_entry);	/* drop initial reference */
28a3a7eb3 Eric Paris      2009-12-17  483  	fsnotify_put_mark(old_entry); /* pair to fsnotify_find mark_entry */
74c3cbe33 Al Viro         2007-07-22 @484  	return 0;
74c3cbe33 Al Viro         2007-07-22  485  }
74c3cbe33 Al Viro         2007-07-22  486  

:::::: The code at line 484 was first introduced by commit
:::::: 74c3cbe33bc077ac1159cadfea608b501e100344 [PATCH] audit: watching subtrees

:::::: TO: Al Viro <viro@zeniv.linux.org.uk>
:::::: CC: Al Viro <viro@zeniv.linux.org.uk>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Jan Kara July 3, 2018, 2:18 p.m. UTC | #3
Hi Dan!

On Mon 02-07-18 09:05:49, Dan Carpenter wrote:
> url:    https://github.com/0day-ci/linux/commits/Jan-Kara/audit-Fix-various-races-when-tagging-and-untagging-mounts/20180629-043337
> 
> smatch warnings:
> kernel/audit_tree.c:484 tag_chunk() warn: inconsistent returns 'mutex:&audit_tree_group->mark_mutex'.
>   Locked on:   line 400
>   Unlocked on: line 411
> 
> # https://github.com/0day-ci/linux/commit/86c9c9a738e409c85891519c17d94043b7f434d5
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout 86c9c9a738e409c85891519c17d94043b7f434d5
> vim +484 kernel/audit_tree.c
> 
> 74c3cbe33 Al Viro         2007-07-22  386  
> 74c3cbe33 Al Viro         2007-07-22  387  /* the first tagged inode becomes root of tree */
> 74c3cbe33 Al Viro         2007-07-22  388  static int tag_chunk(struct inode *inode, struct audit_tree *tree)
> 74c3cbe33 Al Viro         2007-07-22  389  {
> e61ce8673 Eric Paris      2009-12-17  390  	struct fsnotify_mark *old_entry, *chunk_entry;
> 74c3cbe33 Al Viro         2007-07-22  391  	struct audit_tree *owner;
> 74c3cbe33 Al Viro         2007-07-22  392  	struct audit_chunk *chunk, *old;
> 74c3cbe33 Al Viro         2007-07-22  393  	struct node *p;
> 74c3cbe33 Al Viro         2007-07-22  394  	int n;
> 74c3cbe33 Al Viro         2007-07-22  395  
> 86c9c9a73 Jan Kara        2018-06-28  396  	mutex_lock(&audit_tree_group->mark_mutex);
> b1362edfe Jan Kara        2016-12-21  397  	old_entry = fsnotify_find_mark(&inode->i_fsnotify_marks,
> b1362edfe Jan Kara        2016-12-21  398  				       audit_tree_group);
> 28a3a7eb3 Eric Paris      2009-12-17  399  	if (!old_entry)
> 74c3cbe33 Al Viro         2007-07-22  400  		return create_chunk(inode, tree);
>                                                                ^^^^^^^^^^^^^^^^^^^^^^^^^
> Should we drop the lock before this return?

No, because create_chunk() drops &audit_tree_group->mark_mutex in all the
cases. It's a bit ugly to have a function entered with mutex held and
release it but in this case it's somewhat difficult to avoid...

								Honza

Patch
diff mbox

diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 1c82eb6674c4..de8d344d91b1 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -342,25 +342,29 @@  static void untag_chunk(struct node *p)
 	spin_lock(&hash_lock);
 }
 
+/* Call with group->mark_mutex held, releases it */
 static int create_chunk(struct inode *inode, struct audit_tree *tree)
 {
 	struct fsnotify_mark *entry;
 	struct audit_chunk *chunk = alloc_chunk(1);
-	if (!chunk)
+
+	if (!chunk) {
+		mutex_unlock(&audit_tree_group->mark_mutex);
 		return -ENOMEM;
+	}
 
 	entry = &chunk->mark;
-	if (fsnotify_add_inode_mark(entry, inode, 0)) {
+	if (fsnotify_add_inode_mark_locked(entry, inode, 0)) {
+		mutex_unlock(&audit_tree_group->mark_mutex);
 		fsnotify_put_mark(entry);
 		return -ENOSPC;
 	}
 
-	mutex_lock(&entry->group->mark_mutex);
 	spin_lock(&hash_lock);
 	if (tree->goner) {
 		spin_unlock(&hash_lock);
 		chunk->dead = 1;
-		mutex_unlock(&entry->group->mark_mutex);
+		mutex_unlock(&audit_tree_group->mark_mutex);
 		fsnotify_destroy_mark(entry, audit_tree_group);
 		fsnotify_put_mark(entry);
 		return 0;
@@ -375,7 +379,7 @@  static int create_chunk(struct inode *inode, struct audit_tree *tree)
 	}
 	insert_hash(chunk);
 	spin_unlock(&hash_lock);
-	mutex_unlock(&entry->group->mark_mutex);
+	mutex_unlock(&audit_tree_group->mark_mutex);
 	fsnotify_put_mark(entry);	/* drop initial reference */
 	return 0;
 }
@@ -389,6 +393,7 @@  static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 	struct node *p;
 	int n;
 
+	mutex_lock(&audit_tree_group->mark_mutex);
 	old_entry = fsnotify_find_mark(&inode->i_fsnotify_marks,
 				       audit_tree_group);
 	if (!old_entry)
@@ -401,6 +406,7 @@  static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 	for (n = 0; n < old->count; n++) {
 		if (old->owners[n].owner == tree) {
 			spin_unlock(&hash_lock);
+			mutex_unlock(&audit_tree_group->mark_mutex);
 			fsnotify_put_mark(old_entry);
 			return 0;
 		}
@@ -409,20 +415,20 @@  static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 
 	chunk = alloc_chunk(old->count + 1);
 	if (!chunk) {
+		mutex_unlock(&audit_tree_group->mark_mutex);
 		fsnotify_put_mark(old_entry);
 		return -ENOMEM;
 	}
 
 	chunk_entry = &chunk->mark;
 
-	mutex_lock(&old_entry->group->mark_mutex);
 	/*
 	 * mark_mutex protects mark from getting detached and thus also from
 	 * mark->connector->obj getting NULL.
 	 */
 	if (!(old_entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) {
 		/* old_entry is being shot, lets just lie */
-		mutex_unlock(&old_entry->group->mark_mutex);
+		mutex_unlock(&audit_tree_group->mark_mutex);
 		fsnotify_put_mark(old_entry);
 		fsnotify_put_mark(&chunk->mark);
 		return -ENOENT;
@@ -430,7 +436,7 @@  static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 
 	if (fsnotify_add_mark_locked(chunk_entry, old_entry->connector->obj,
 				     FSNOTIFY_OBJ_TYPE_INODE, 1)) {
-		mutex_unlock(&old_entry->group->mark_mutex);
+		mutex_unlock(&audit_tree_group->mark_mutex);
 		fsnotify_put_mark(chunk_entry);
 		fsnotify_put_mark(old_entry);
 		return -ENOSPC;
@@ -440,7 +446,7 @@  static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 	if (tree->goner) {
 		spin_unlock(&hash_lock);
 		chunk->dead = 1;
-		mutex_unlock(&old_entry->group->mark_mutex);
+		mutex_unlock(&audit_tree_group->mark_mutex);
 
 		fsnotify_destroy_mark(chunk_entry, audit_tree_group);
 
@@ -471,7 +477,7 @@  static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 		list_add(&tree->same_root, &chunk->trees);
 	}
 	spin_unlock(&hash_lock);
-	mutex_unlock(&old_entry->group->mark_mutex);
+	mutex_unlock(&audit_tree_group->mark_mutex);
 	fsnotify_destroy_mark(old_entry, audit_tree_group);
 	fsnotify_put_mark(chunk_entry);	/* drop initial reference */
 	fsnotify_put_mark(old_entry); /* pair to fsnotify_find mark_entry */