[3/6] audit: Fix possible tagging failures
diff mbox

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

Commit Message

Jan Kara June 28, 2018, 4:40 p.m. UTC
Audit tree code is replacing marks attached to inodes in non-atomic way.
Thus fsnotify_find_mark() in tag_chunk() may find a mark that belongs to
a chunk that is no longer valid one and will soon be destroyed. Tags
added to such chunk will be simply lost.

Fix the problem by making sure old mark is marked as going away (through
fsnotify_detach_mark()) before dropping mark_mutex and thus in an atomic
way wrt tag_chunk(). Note that this does not fix the problem completely
as if tag_chunk() finds a mark that is going away, it fails with
-ENOENT. But at least the failure is not silent and currently there's no
way to search for another fsnotify mark attached to the inode. We'll fix
this problem in later patch.

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

Comments

Amir Goldstein June 29, 2018, 12:05 p.m. UTC | #1
On Thu, Jun 28, 2018 at 7:40 PM, Jan Kara <jack@suse.cz> wrote:
> Audit tree code is replacing marks attached to inodes in non-atomic way.
> Thus fsnotify_find_mark() in tag_chunk() may find a mark that belongs to
> a chunk that is no longer valid one and will soon be destroyed. Tags
> added to such chunk will be simply lost.
>
> Fix the problem by making sure old mark is marked as going away (through
> fsnotify_detach_mark()) before dropping mark_mutex and thus in an atomic
> way wrt tag_chunk(). Note that this does not fix the problem completely
> as if tag_chunk() finds a mark that is going away, it fails with
> -ENOENT. But at least the failure is not silent and currently there's no
> way to search for another fsnotify mark attached to the inode. We'll fix
> this problem in later patch.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---

This one too looks sane.
Without knowing anything about audit_watch, there seems to be
an fsnotify_destroy_mark() after unlock of audit_filter_mutex, so it
may require a similar fix.

Thanks,
Amir.
Jan Kara July 3, 2018, 2:21 p.m. UTC | #2
On Fri 29-06-18 15:05:07, Amir Goldstein wrote:
> On Thu, Jun 28, 2018 at 7:40 PM, Jan Kara <jack@suse.cz> wrote:
> > Audit tree code is replacing marks attached to inodes in non-atomic way.
> > Thus fsnotify_find_mark() in tag_chunk() may find a mark that belongs to
> > a chunk that is no longer valid one and will soon be destroyed. Tags
> > added to such chunk will be simply lost.
> >
> > Fix the problem by making sure old mark is marked as going away (through
> > fsnotify_detach_mark()) before dropping mark_mutex and thus in an atomic
> > way wrt tag_chunk(). Note that this does not fix the problem completely
> > as if tag_chunk() finds a mark that is going away, it fails with
> > -ENOENT. But at least the failure is not silent and currently there's no
> > way to search for another fsnotify mark attached to the inode. We'll fix
> > this problem in later patch.
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> 
> This one too looks sane.
> Without knowing anything about audit_watch, there seems to be
> an fsnotify_destroy_mark() after unlock of audit_filter_mutex, so it
> may require a similar fix.

Where? I don't see any call to fsnotify_destroy_mark() left after this
patch...

								Honza
Amir Goldstein July 3, 2018, 5:42 p.m. UTC | #3
On Tue, Jul 3, 2018 at 5:21 PM, Jan Kara <jack@suse.cz> wrote:
> On Fri 29-06-18 15:05:07, Amir Goldstein wrote:
>> On Thu, Jun 28, 2018 at 7:40 PM, Jan Kara <jack@suse.cz> wrote:
>> > Audit tree code is replacing marks attached to inodes in non-atomic way.
>> > Thus fsnotify_find_mark() in tag_chunk() may find a mark that belongs to
>> > a chunk that is no longer valid one and will soon be destroyed. Tags
>> > added to such chunk will be simply lost.
>> >
>> > Fix the problem by making sure old mark is marked as going away (through
>> > fsnotify_detach_mark()) before dropping mark_mutex and thus in an atomic
>> > way wrt tag_chunk(). Note that this does not fix the problem completely
>> > as if tag_chunk() finds a mark that is going away, it fails with
>> > -ENOENT. But at least the failure is not silent and currently there's no
>> > way to search for another fsnotify mark attached to the inode. We'll fix
>> > this problem in later patch.
>> >
>> > Signed-off-by: Jan Kara <jack@suse.cz>
>> > ---
>>
>> This one too looks sane.
>> Without knowing anything about audit_watch, there seems to be
>> an fsnotify_destroy_mark() after unlock of audit_filter_mutex, so it
>> may require a similar fix.
>
> Where? I don't see any call to fsnotify_destroy_mark() left after this
> patch...
>

Not directly related to this cleanup, but looking in other audit modules,
fsnotify_destroy_mark() in audit_remove_parent_watches() is called
outside audit_filter_mutex and audit_find_parent() in audit_add_watch()
is called inside audit_filter_mutex, so I was wondering if there was a
similar race window in that code. I didn't spend enough time looking
at audit_watch.c to understand what is going on in there.

Thanks,
Amir.
Jan Kara July 4, 2018, 8:28 a.m. UTC | #4
On Tue 03-07-18 20:42:26, Amir Goldstein wrote:
> On Tue, Jul 3, 2018 at 5:21 PM, Jan Kara <jack@suse.cz> wrote:
> > On Fri 29-06-18 15:05:07, Amir Goldstein wrote:
> >> On Thu, Jun 28, 2018 at 7:40 PM, Jan Kara <jack@suse.cz> wrote:
> >> > Audit tree code is replacing marks attached to inodes in non-atomic way.
> >> > Thus fsnotify_find_mark() in tag_chunk() may find a mark that belongs to
> >> > a chunk that is no longer valid one and will soon be destroyed. Tags
> >> > added to such chunk will be simply lost.
> >> >
> >> > Fix the problem by making sure old mark is marked as going away (through
> >> > fsnotify_detach_mark()) before dropping mark_mutex and thus in an atomic
> >> > way wrt tag_chunk(). Note that this does not fix the problem completely
> >> > as if tag_chunk() finds a mark that is going away, it fails with
> >> > -ENOENT. But at least the failure is not silent and currently there's no
> >> > way to search for another fsnotify mark attached to the inode. We'll fix
> >> > this problem in later patch.
> >> >
> >> > Signed-off-by: Jan Kara <jack@suse.cz>
> >> > ---
> >>
> >> This one too looks sane.
> >> Without knowing anything about audit_watch, there seems to be
> >> an fsnotify_destroy_mark() after unlock of audit_filter_mutex, so it
> >> may require a similar fix.
> >
> > Where? I don't see any call to fsnotify_destroy_mark() left after this
> > patch...
> >
> 
> Not directly related to this cleanup, but looking in other audit modules,
> fsnotify_destroy_mark() in audit_remove_parent_watches() is called
> outside audit_filter_mutex and audit_find_parent() in audit_add_watch()
> is called inside audit_filter_mutex, so I was wondering if there was a
> similar race window in that code. I didn't spend enough time looking
> at audit_watch.c to understand what is going on in there.

Oh, those are completely different fsnotify marks :) They belong to
audit_watch_group (while these to audit_tree_group) and these patches have
no ambition to change anything there.

								Honza

Patch
diff mbox

diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index de8d344d91b1..7f9df8c66227 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -278,8 +278,9 @@  static void untag_chunk(struct node *p)
 		list_del_init(&p->list);
 		list_del_rcu(&chunk->hash);
 		spin_unlock(&hash_lock);
+		fsnotify_detach_mark(entry);
 		mutex_unlock(&entry->group->mark_mutex);
-		fsnotify_destroy_mark(entry, audit_tree_group);
+		fsnotify_free_mark(entry);
 		goto out;
 	}
 
@@ -320,8 +321,9 @@  static void untag_chunk(struct node *p)
 	list_for_each_entry(owner, &new->trees, same_root)
 		owner->root = new;
 	spin_unlock(&hash_lock);
+	fsnotify_detach_mark(entry);
 	mutex_unlock(&entry->group->mark_mutex);
-	fsnotify_destroy_mark(entry, audit_tree_group);
+	fsnotify_free_mark(entry);
 	fsnotify_put_mark(&new->mark);	/* drop initial reference */
 	goto out;
 
@@ -364,8 +366,9 @@  static int create_chunk(struct inode *inode, struct audit_tree *tree)
 	if (tree->goner) {
 		spin_unlock(&hash_lock);
 		chunk->dead = 1;
+		fsnotify_detach_mark(entry);
 		mutex_unlock(&audit_tree_group->mark_mutex);
-		fsnotify_destroy_mark(entry, audit_tree_group);
+		fsnotify_free_mark(entry);
 		fsnotify_put_mark(entry);
 		return 0;
 	}
@@ -446,10 +449,9 @@  static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 	if (tree->goner) {
 		spin_unlock(&hash_lock);
 		chunk->dead = 1;
+		fsnotify_detach_mark(chunk_entry);
 		mutex_unlock(&audit_tree_group->mark_mutex);
-
-		fsnotify_destroy_mark(chunk_entry, audit_tree_group);
-
+		fsnotify_free_mark(chunk_entry);
 		fsnotify_put_mark(chunk_entry);
 		fsnotify_put_mark(old_entry);
 		return 0;
@@ -477,8 +479,9 @@  static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 		list_add(&tree->same_root, &chunk->trees);
 	}
 	spin_unlock(&hash_lock);
+	fsnotify_detach_mark(old_entry);
 	mutex_unlock(&audit_tree_group->mark_mutex);
-	fsnotify_destroy_mark(old_entry, audit_tree_group);
+	fsnotify_free_mark(old_entry);
 	fsnotify_put_mark(chunk_entry);	/* drop initial reference */
 	fsnotify_put_mark(old_entry); /* pair to fsnotify_find mark_entry */
 	return 0;