Re: [PATCH 4/6] audit: Embed key into chunk

On Thu, Jun 28, 2018 at 7:40 PM, Jan Kara <jack(a)suse.cz&gt; 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(a)suse.cz&gt; > --- > 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.