On Fri 29-06-18 15:53:20, Amir Goldstein wrote:
> On Thu, Jun 28, 2018 at 7:40 PM, Jan Kara <jack@xxxxxxx> 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@xxxxxxx>
> > ---
> > 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
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
