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

[Jan Kara]

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