From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com ([209.132.183.28]:47070 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726445AbeINBWc (ORCPT ); Thu, 13 Sep 2018 21:22:32 -0400 Date: Thu, 13 Sep 2018 16:06:40 -0400 From: Richard Guy Briggs To: Jan Kara Cc: Paul Moore , Al Viro , linux-audit@redhat.com, linux-fsdevel@vger.kernel.org, Amir Goldstein Subject: Re: [PATCH 04/11] audit: Embed key into chunk Message-ID: <20180913200640.zhxdnlg7zqjbehhu@madcap2.tricolour.ca> References: <20180904160632.21210-1-jack@suse.cz> <20180904160632.21210-5-jack@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180904160632.21210-5-jack@suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 2018-09-04 18:06, Jan Kara 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 > --- > kernel/audit_tree.c | 27 ++++++++------------------- > 1 file changed, 8 insertions(+), 19 deletions(-) > > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > index c194dbd53753..bac5dd90c629 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,12 @@ 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); > + WARN_ON_ONCE(!chunk->key); > + list = chunk_hash(chunk->key); > list_add_rcu(&chunk->hash, list); > } > > @@ -213,7 +199,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 +281,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 +367,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); Is there a patch missing that somehow converts from chunk_to_key() to inode_to_key() and from chunk->mark.connector->obj to chunk->mark.connector->inode that I've missed? Yes. 36f10f55ff1d 2018-06-23 ("fsnotify: let connector point to an abstract object"). I was looking at audit/next rather than v4.19-rc1. > insert_hash(chunk); > spin_unlock(&hash_lock); > mutex_unlock(&audit_tree_group->mark_mutex); > @@ -456,6 +444,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; > @@ -654,7 +643,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); > } > -- > 2.16.4 - RGB -- Richard Guy Briggs Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635