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.
Thanks,
Amir.
