[PATCH] affs: split hash-lock subclass and reject self-referencing dir entry

[Farhad Alemi]

affs_remove_header() acquires i_hash_lock on the parent directory at the
top of the function and again on the child inode inside the ST_USERDIR
arm, both via affs_lock_dir() which uses SINGLE_DEPTH_NESTING. Lockdep
sees two acquisitions of the same lock class in the same task at the
same subclass and warns:

  WARNING: possible recursive locking detected
  syz.2.17/3573 is trying to acquire lock:
  ffff888123db0778 (&ei->i_ext_lock/1){+.+.}-{4:4}, at: affs_lock_dir

affs_remove_header+0x72f fs/affs/amigaffs.c:296
  but task is already holding lock:
  ffff888123db0100 (&ei->i_ext_lock/1){+.+.}-{4:4}, at: affs_lock_dir

affs_remove_header+0x261 fs/affs/amigaffs.c:289
  Call Trace:
   affs_lock_dir fs/affs/affs.h:311 [inline]
   affs_remove_header+0x72f/0x1ab0 fs/affs/amigaffs.c:296
   vfs_rmdir+0x20b/0x6a0 fs/namei.c:4446
   do_rmdir+0x2ed/0x3c0 fs/namei.c:4524
   __x64_sys_unlinkat+0xf4/0x140 fs/namei.c:4692

With panic_on_warn this terminates the kernel. The two lock instances in
the report are distinct inodes (db0778 vs db0100), so what lockdep is
reporting is two acquisitions of the same lock class at the same
subclass -- a missing subclass distinction in the annotation. Trigger
requires the ability to mount a crafted image (CAP_SYS_ADMIN or
equivalent) and is reproduced by rmdir() of a subdirectory under a
crafted AFFS image.

The same locking pattern is also vulnerable to a strict self-deadlock
on a crafted image whose on-disk hash table contains an entry with i_ino
equal to its containing directory's own block number: affs_iget() would
return the same in-memory inode for both d_inode(dentry) and
d_inode(dentry->d_parent), and the two affs_lock_dir() calls would take
the same mutex twice. No lockdep distinction would prevent this since
the same lock instance is acquired twice.

Address both by:

  1. Adding affs_lock_subdir() that uses SINGLE_DEPTH_NESTING + 1, so
     lockdep can prove the nested parent->child acquisition is safe.

  2. Rejecting the case where the parsed child inode coincides with the
     parent before taking the second lock, returning -EIO under the
     existing done_unlock path so all previously-acquired locks are
     released cleanly.

Reported-by: Farhad Alemi <farhad.alemi@xxxxxxxxxxxx>
Closes: https://lore.kernel.org/lkml/CA+0ovCiA7huMwMxvWgC8km2P+gJwd-jax+ACo=EbGrJ6FVp55A@xxxxxxxxxxxxxx/
Signed-off-by: Farhad Alemi <farhad.alemi@xxxxxxxxxxxx>
---
 fs/affs/affs.h     | 6 ++++++
 fs/affs/amigaffs.c | 6 +++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/fs/affs/affs.h b/fs/affs/affs.h
index a0caf6ace860..c4af169294ed 100644
--- a/fs/affs/affs.h
+++ b/fs/affs/affs.h
@@ -313,6 +313,12 @@ affs_lock_dir(struct inode *inode)
 	mutex_lock_nested(&AFFS_I(inode)->i_hash_lock, SINGLE_DEPTH_NESTING);
 }
 static inline void
+affs_lock_subdir(struct inode *inode)
+{
+	mutex_lock_nested(&AFFS_I(inode)->i_hash_lock,
+			  SINGLE_DEPTH_NESTING + 1);
+}
+static inline void
 affs_unlock_dir(struct inode *inode)
 {
 	mutex_unlock(&AFFS_I(inode)->i_hash_lock);
diff --git a/fs/affs/amigaffs.c b/fs/affs/amigaffs.c
index bed4fc805e8e..fefc2bb11d43 100644
--- a/fs/affs/amigaffs.c
+++ b/fs/affs/amigaffs.c
@@ -293,7 +293,11 @@ affs_remove_header(struct dentry *dentry)
 		 * i_hash_lock of the inode must only be
 		 * taken after some checks
 		 */
-		affs_lock_dir(inode);
+		if (inode == dir) {
+			retval = -EIO;
+			goto done_unlock;
+		}
+		affs_lock_subdir(inode);
 		retval = affs_empty_dir(inode);
 		affs_unlock_dir(inode);
 		if (retval)
-- 
2.43.0