| Message ID | 20180628164014.4925-5-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: > Currently chunk hash key (which is in fact pointer to the inode) is > derived as chunk->mark.conn->obj. It is tricky to make this dereference > reliable for hash table lookups only under RCU as mark can get detached > from the connector and connector gets freed independently of the > running lookup. Thus there is a possible use after free / NULL ptr > dereference issue: > > CPU1 CPU2 > untag_chunk() > ... > audit_tree_lookup() > list_for_each_entry_rcu(p, list, hash) { > list_del_rcu(&chunk->hash); > fsnotify_destroy_mark(entry); > fsnotify_put_mark(entry) > chunk_to_key(p) > if (!chunk->mark.connector) > ... > hlist_del_init_rcu(&mark->obj_list); > if (hlist_empty(&conn->list)) { > inode = fsnotify_detach_connector_from_object(conn); > mark->connector = NULL; > ... > frees connector from workqueue > chunk->mark.connector->obj > > This race is probably impossible to hit in practice as the race window > on CPU1 is very narrow and CPU2 has a lot of code to execute. Still it's > better to have this fixed. Since the inode the chunk is attached to is > constant during chunk's lifetime it is easy to cache the key in the > chunk itself and thus avoid these issues. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > kernel/audit_tree.c | 26 +++++++------------------- > 1 file changed, 7 insertions(+), 19 deletions(-) > > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > index 7f9df8c66227..89ad8857a578 100644 > --- a/kernel/audit_tree.c > +++ b/kernel/audit_tree.c > @@ -24,6 +24,7 @@ struct audit_tree { > > struct audit_chunk { > struct list_head hash; > + unsigned long key; > struct fsnotify_mark mark; > struct list_head trees; /* with root here */ > int dead; > @@ -172,21 +173,6 @@ static unsigned long inode_to_key(const struct inode *inode) > return (unsigned long)&inode->i_fsnotify_marks; > } > > -/* > - * Function to return search key in our hash from chunk. Key 0 is special and > - * should never be present in the hash. > - */ > -static unsigned long chunk_to_key(struct audit_chunk *chunk) > -{ > - /* > - * We have a reference to the mark so it should be attached to a > - * connector. > - */ > - if (WARN_ON_ONCE(!chunk->mark.connector)) > - return 0; > - return (unsigned long)chunk->mark.connector->obj; > -} > - > static inline struct list_head *chunk_hash(unsigned long key) > { > unsigned long n = key / L1_CACHE_BYTES; > @@ -196,12 +182,11 @@ static inline struct list_head *chunk_hash(unsigned long key) > /* hash_lock & entry->group->mark_mutex is held by caller */ > static void insert_hash(struct audit_chunk *chunk) > { > - unsigned long key = chunk_to_key(chunk); > struct list_head *list; > Suggest to add check (WARN_ON_ONCE(!chunk->key)) because its quite hard to assert by code review alone that no execution path was missed where chunk->key should have been set. Thanks, Amir.
On Fri 29-06-18 15:53:20, Amir Goldstein wrote: > On Thu, Jun 28, 2018 at 7:40 PM, Jan Kara <jack@suse.cz> wrote: > > Currently chunk hash key (which is in fact pointer to the inode) is > > derived as chunk->mark.conn->obj. It is tricky to make this dereference > > reliable for hash table lookups only under RCU as mark can get detached > > from the connector and connector gets freed independently of the > > running lookup. Thus there is a possible use after free / NULL ptr > > dereference issue: > > > > CPU1 CPU2 > > untag_chunk() > > ... > > audit_tree_lookup() > > list_for_each_entry_rcu(p, list, hash) { > > list_del_rcu(&chunk->hash); > > fsnotify_destroy_mark(entry); > > fsnotify_put_mark(entry) > > chunk_to_key(p) > > if (!chunk->mark.connector) > > ... > > hlist_del_init_rcu(&mark->obj_list); > > if (hlist_empty(&conn->list)) { > > inode = fsnotify_detach_connector_from_object(conn); > > mark->connector = NULL; > > ... > > frees connector from workqueue > > chunk->mark.connector->obj > > > > This race is probably impossible to hit in practice as the race window > > on CPU1 is very narrow and CPU2 has a lot of code to execute. Still it's > > better to have this fixed. Since the inode the chunk is attached to is > > constant during chunk's lifetime it is easy to cache the key in the > > chunk itself and thus avoid these issues. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > kernel/audit_tree.c | 26 +++++++------------------- > > 1 file changed, 7 insertions(+), 19 deletions(-) > > > > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > > index 7f9df8c66227..89ad8857a578 100644 > > --- a/kernel/audit_tree.c > > +++ b/kernel/audit_tree.c > > @@ -24,6 +24,7 @@ struct audit_tree { > > > > struct audit_chunk { > > struct list_head hash; > > + unsigned long key; > > struct fsnotify_mark mark; > > struct list_head trees; /* with root here */ > > int dead; > > @@ -172,21 +173,6 @@ static unsigned long inode_to_key(const struct inode *inode) > > return (unsigned long)&inode->i_fsnotify_marks; > > } > > > > -/* > > - * Function to return search key in our hash from chunk. Key 0 is special and > > - * should never be present in the hash. > > - */ > > -static unsigned long chunk_to_key(struct audit_chunk *chunk) > > -{ > > - /* > > - * We have a reference to the mark so it should be attached to a > > - * connector. > > - */ > > - if (WARN_ON_ONCE(!chunk->mark.connector)) > > - return 0; > > - return (unsigned long)chunk->mark.connector->obj; > > -} > > - > > static inline struct list_head *chunk_hash(unsigned long key) > > { > > unsigned long n = key / L1_CACHE_BYTES; > > @@ -196,12 +182,11 @@ static inline struct list_head *chunk_hash(unsigned long key) > > /* hash_lock & entry->group->mark_mutex is held by caller */ > > static void insert_hash(struct audit_chunk *chunk) > > { > > - unsigned long key = chunk_to_key(chunk); > > struct list_head *list; > > > > Suggest to add check (WARN_ON_ONCE(!chunk->key)) > because its quite hard to assert by code review alone that no execution > path was missed where chunk->key should have been set. OK, added. FWIW, create_chunk() is the only place that uses insert_hash() and that clearly sets the key. But it doesn't hurt to check here for future-proofing. Honza
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index 7f9df8c66227..89ad8857a578 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -24,6 +24,7 @@ struct audit_tree { struct audit_chunk { struct list_head hash; + unsigned long key; struct fsnotify_mark mark; struct list_head trees; /* with root here */ int dead; @@ -172,21 +173,6 @@ static unsigned long inode_to_key(const struct inode *inode) return (unsigned long)&inode->i_fsnotify_marks; } -/* - * Function to return search key in our hash from chunk. Key 0 is special and - * should never be present in the hash. - */ -static unsigned long chunk_to_key(struct audit_chunk *chunk) -{ - /* - * We have a reference to the mark so it should be attached to a - * connector. - */ - if (WARN_ON_ONCE(!chunk->mark.connector)) - return 0; - return (unsigned long)chunk->mark.connector->obj; -} - static inline struct list_head *chunk_hash(unsigned long key) { unsigned long n = key / L1_CACHE_BYTES; @@ -196,12 +182,11 @@ static inline struct list_head *chunk_hash(unsigned long key) /* hash_lock & entry->group->mark_mutex is held by caller */ static void insert_hash(struct audit_chunk *chunk) { - unsigned long key = chunk_to_key(chunk); struct list_head *list; if (!(chunk->mark.flags & FSNOTIFY_MARK_FLAG_ATTACHED)) return; - list = chunk_hash(key); + list = chunk_hash(chunk->key); list_add_rcu(&chunk->hash, list); } @@ -213,7 +198,7 @@ struct audit_chunk *audit_tree_lookup(const struct inode *inode) struct audit_chunk *p; list_for_each_entry_rcu(p, list, hash) { - if (chunk_to_key(p) == key) { + if (p->key == key) { atomic_long_inc(&p->refs); return p; } @@ -295,6 +280,7 @@ static void untag_chunk(struct node *p) chunk->dead = 1; spin_lock(&hash_lock); + new->key = chunk->key; list_replace_init(&chunk->trees, &new->trees); if (owner->root == chunk) { list_del_init(&owner->same_root); @@ -380,6 +366,7 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree) tree->root = chunk; list_add(&tree->same_root, &chunk->trees); } + chunk->key = inode_to_key(inode); insert_hash(chunk); spin_unlock(&hash_lock); mutex_unlock(&audit_tree_group->mark_mutex); @@ -456,6 +443,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) fsnotify_put_mark(old_entry); return 0; } + chunk->key = old->key; list_replace_init(&old->trees, &chunk->trees); for (n = 0, p = chunk->owners; n < old->count; n++, p++) { struct audit_tree *s = old->owners[n].owner; @@ -652,7 +640,7 @@ void audit_trim_trees(void) /* this could be NULL if the watch is dying else where... */ node->index |= 1U<<31; if (iterate_mounts(compare_root, - (void *)chunk_to_key(chunk), + (void *)(chunk->key), root_mnt)) node->index &= ~(1U<<31); }
Currently chunk hash key (which is in fact pointer to the inode) is derived as chunk->mark.conn->obj. It is tricky to make this dereference reliable for hash table lookups only under RCU as mark can get detached from the connector and connector gets freed independently of the running lookup. Thus there is a possible use after free / NULL ptr dereference issue: CPU1 CPU2 untag_chunk() ... audit_tree_lookup() list_for_each_entry_rcu(p, list, hash) { list_del_rcu(&chunk->hash); fsnotify_destroy_mark(entry); fsnotify_put_mark(entry) chunk_to_key(p) if (!chunk->mark.connector) ... hlist_del_init_rcu(&mark->obj_list); if (hlist_empty(&conn->list)) { inode = fsnotify_detach_connector_from_object(conn); mark->connector = NULL; ... frees connector from workqueue chunk->mark.connector->obj This race is probably impossible to hit in practice as the race window on CPU1 is very narrow and CPU2 has a lot of code to execute. Still it's better to have this fixed. Since the inode the chunk is attached to is constant during chunk's lifetime it is easy to cache the key in the chunk itself and thus avoid these issues. Signed-off-by: Jan Kara <jack@suse.cz> --- kernel/audit_tree.c | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-)