| Message ID | 20180628164014.4925-4-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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
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;
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(-)