[PATCH] fsnotify: Fix NULL ptr deref in fanotify_get_fsid()

[Jan Kara <jack@xxxxxxx>]

fanotify_get_fsid() is reading mark->connector->fsid under srcu. It can
happen that it sees mark not fully initialized and thus mark->connector
is still NULL leading to NULL ptr dereference. Fix the problem by
setting mark->connector before adding mark to the object's list and use
appropriate memory barriers to make sure proper mark->connector value is
seen for any mark added to the list.

Reported-by: syzbot+15927486a4f1bfcbaf91@xxxxxxxxxxxxxxxxxxxxxxxxx
Fixes: 77115225acc6 ("fanotify: cache fsid in fsnotify_mark_connector")
Signed-off-by: Jan Kara <jack@xxxxxxx>
---
 fs/notify/fanotify/fanotify.c |  2 +-
 fs/notify/mark.c              | 43 +++++++++++++++++++++++++++++--------------
 2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 6b9c27548997..f34f0667ea60 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -349,7 +349,7 @@ static __kernel_fsid_t fanotify_get_fsid(struct fsnotify_iter_info *iter_info)
 		if (!fsnotify_iter_should_report_type(iter_info, type))
 			continue;
 
-		fsid = iter_info->marks[type]->connector->fsid;
+		fsid = READ_ONCE(iter_info->marks[type]->connector)->fsid;
 		if (WARN_ON_ONCE(!fsid.val[0] && !fsid.val[1]))
 			continue;
 		return fsid;
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index d593d4269561..a10abf90f1bc 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -239,13 +239,13 @@ static void fsnotify_drop_object(unsigned int type, void *objp)
 
 void fsnotify_put_mark(struct fsnotify_mark *mark)
 {
-	struct fsnotify_mark_connector *conn;
+	struct fsnotify_mark_connector *conn = READ_ONCE(mark->connector);
 	void *objp = NULL;
 	unsigned int type = FSNOTIFY_OBJ_TYPE_DETACHED;
 	bool free_conn = false;
 
 	/* Catch marks that were actually never attached to object */
-	if (!mark->connector) {
+	if (!conn) {
 		if (refcount_dec_and_test(&mark->refcnt))
 			fsnotify_final_mark_destroy(mark);
 		return;
@@ -255,10 +255,9 @@ void fsnotify_put_mark(struct fsnotify_mark *mark)
 	 * We have to be careful so that traversals of obj_list under lock can
 	 * safely grab mark reference.
 	 */
-	if (!refcount_dec_and_lock(&mark->refcnt, &mark->connector->lock))
+	if (!refcount_dec_and_lock(&mark->refcnt, &conn->lock))
 		return;
 
-	conn = mark->connector;
 	hlist_del_init_rcu(&mark->obj_list);
 	if (hlist_empty(&conn->list)) {
 		objp = fsnotify_detach_connector_from_object(conn, &type);
@@ -543,6 +542,23 @@ static struct fsnotify_mark_connector *fsnotify_grab_connector(
 	return conn;
 }
 
+/*
+ * We need to assign mark->connector before adding mark to the list so that
+ * RCU readers can safely dereference it but do not want to set it before
+ * we are really sure we are adding the mark to the connector's list so that
+ * someone cannot see connector value that was reset back to NULL in the end.
+ */
+#define fanotify_attach_obj_list(where, mark, conn, head) \
+	do {\
+		mark->connector = conn;\
+		/*
+		 * Make sure RCU readers of object list see connector
+		 * initialized.  Pairs in READ_ONCE for unlocked readers.
+		 */\
+		smp_wmb();\
+		hlist_add_##where##_rcu(&mark->obj_list, head);\
+	} while (0)
+
 /*
  * Add mark into proper place in given list of marks. These marks may be used
  * for the fsnotify backend to determine which event types should be delivered
@@ -589,13 +605,13 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
 				    fsid->val[0], fsid->val[1],
 				    conn->fsid.val[0], conn->fsid.val[1]);
 		err = -EXDEV;
-		goto out_err;
+		goto out;
 	}
 
 	/* is mark the first mark? */
 	if (hlist_empty(&conn->list)) {
-		hlist_add_head_rcu(&mark->obj_list, &conn->list);
-		goto added;
+		fanotify_attach_obj_list(head, mark, conn, &conn->list);
+		goto out;
 	}
 
 	/* should mark be in the middle of the current list? */
@@ -606,22 +622,21 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
 		    (lmark->flags & FSNOTIFY_MARK_FLAG_ATTACHED) &&
 		    !allow_dups) {
 			err = -EEXIST;
-			goto out_err;
+			goto out;
 		}
 
 		cmp = fsnotify_compare_groups(lmark->group, mark->group);
 		if (cmp >= 0) {
-			hlist_add_before_rcu(&mark->obj_list, &lmark->obj_list);
-			goto added;
+			fanotify_attach_obj_list(before, mark, conn,
+						 &lmark->obj_list);
+			goto out;
 		}
 	}
 
 	BUG_ON(last == NULL);
 	/* mark should be the last entry.  last is the current last entry */
-	hlist_add_behind_rcu(&mark->obj_list, &last->obj_list);
-added:
-	mark->connector = conn;
-out_err:
+	fanotify_attach_obj_list(behind, mark, conn, &last->obj_list);
+out:
 	spin_unlock(&conn->lock);
 	spin_unlock(&mark->lock);
 	return err;
-- 
2.16.4