Re: f2fs_empty_dir() can be extremely slow on malicious disk images

[Chao Yu]

On 2022/11/2 14:17, Eric Biggers wrote:
> [+f2fs list and maintainers]

Thanks for the forwarding.

> [changed subject from "INFO: task hung in fscrypt_ioctl_set_policy"]
>
> On Mon, Oct 31, 2022 at 10:18:02PM +0800, Wei Chen wrote:
> > Dear Linux developers,
> >
> > Here is the link to the reproducers.
> >
> > C reproducer: https://drive.google.com/file/d/1mduYsYuoOKemH3qkvpDQwnAHAaaLUp0Y/view?usp=share_link
> > Syz reproducer:
> > https://drive.google.com/file/d/1mu-_w7dy_562vWRlQvTRbcBjG4_G7b2L/view?usp=share_link
> >
> > The bug persists in the latest commit, v5.15.76 (4f5365f77018). I hope
> > it is helpful to you.
> >
> > [ 1782.137186][   T30] INFO: task a.out:6910 blocked for more than 143 seconds.
> > [ 1782.139217][   T30]       Not tainted 5.15.76 #5
> > [ 1782.140388][   T30] "echo 0 >
> > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> > [ 1782.142524][   T30] task:a.out           state:D stack:14296 pid:
> > 6910 ppid:  6532 flags:0x00004004
> > [ 1782.144799][   T30] Call Trace:
> > [ 1782.145623][   T30]  <TASK>
> > [ 1782.146316][   T30]  __schedule+0x3e8/0x1850
> > [ 1782.152029][   T30]  ? mark_held_locks+0x49/0x70
> > [ 1782.153533][   T30]  ? mark_held_locks+0x10/0x70
> > [ 1782.154759][   T30]  ? __down_write_common.part.14+0x31f/0x7b0
> > [ 1782.156159][   T30]  schedule+0x4e/0xe0
> > [ 1782.158314][   T30]  __down_write_common.part.14+0x324/0x7b0
> > [ 1782.159704][   T30]  ? fscrypt_ioctl_set_policy+0xe0/0x200
> > [ 1782.161050][   T30]  fscrypt_ioctl_set_policy+0xe0/0x200
> > [ 1782.162330][   T30]  __f2fs_ioctl+0x9d6/0x45e0
> > [ 1782.163417][   T30]  f2fs_ioctl+0x64/0x240
> > [ 1782.164404][   T30]  ? __f2fs_ioctl+0x45e0/0x45e0
> > [ 1782.165554][   T30]  __x64_sys_ioctl+0xb6/0x100
> > [ 1782.166662][   T30]  do_syscall_64+0x34/0xb0
> > [ 1782.169947][   T30]  entry_SYSCALL_64_after_hwframe+0x61/0xcb
>
> Well, the quality of this bug report has a lot to be desired (not on upstream
> kernel, reproducer is full of totally irrelevant stuff, not sent to the mailing
> list of the filesystem whose disk image is being fuzzed, etc.).  But what is
> going on is that f2fs_empty_dir() doesn't consider the case of a directory with
> an extremely large i_size on a malicious disk image.
>
> Specifically, the reproducer mounts an f2fs image with a directory that has an
> i_size of 14814520042850357248, then calls FS_IOC_SET_ENCRYPTION_POLICY on it.
> That results in a call to f2fs_empty_dir() to check whether the directory is
> empty.  f2fs_empty_dir() then iterates through all 3616826182336513 blocks the
> directory allegedly contains to check whether any contain anything.  i_rwsem is
> held during this, so anything else that tries to take it will hang.
>
> I'll look into this more if needed, but Jaegeuk and Chao, do you happen to have
> any ideas for how f2fs_empty_dir() should be fixed?  Is there an easy way to
> just iterate through the blocks that are actually allocated?

I send this just for requesting comments, no test now.

Thoughts?

From 38ea5f172c47853536a9f70857e4438a69d16f39 Mon Sep 17 00:00:00 2001
From: Chao Yu <chao@xxxxxxxxxx>
Date: Wed, 2 Nov 2022 12:02:08 +0800
Subject: [RFC PATCH] f2fs: speed up f2fs_empty_dir()

Signed-off-by: Chao Yu <chao@xxxxxxxxxx>
---
 fs/f2fs/dir.c | 48 ++++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 21960a899b6a..45f52b34ed1f 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -956,38 +956,42 @@ void f2fs_delete_entry(struct f2fs_dir_entry *dentry, struct page *page,

 bool f2fs_empty_dir(struct inode *dir)
 {
-	unsigned long bidx;
 	struct page *dentry_page;
 	unsigned int bit_pos;
 	struct f2fs_dentry_block *dentry_blk;
-	unsigned long nblock = dir_blocks(dir);
+	struct f2fs_map_blocks map;
+	int ret;

 	if (f2fs_has_inline_dentry(dir))
 		return f2fs_empty_inline_dir(dir);

-	for (bidx = 0; bidx < nblock; bidx++) {
-		dentry_page = f2fs_get_lock_data_page(dir, bidx, false);
-		if (IS_ERR(dentry_page)) {
-			if (PTR_ERR(dentry_page) == -ENOENT)
-				continue;
-			else
-				return false;
-		}
+	dentry_page = f2fs_get_lock_data_page(dir, 0, false);
+	if (IS_ERR(dentry_page)) {
+		if (PTR_ERR(dentry_page) == -ENOENT)
+			return true;
+		return false;
+	}

-		dentry_blk = page_address(dentry_page);
-		if (bidx == 0)
-			bit_pos = 2;
-		else
-			bit_pos = 0;
-		bit_pos = find_next_bit_le(&dentry_blk->dentry_bitmap,
-						NR_DENTRY_IN_BLOCK,
-						bit_pos);
+	dentry_blk = page_address(dentry_page);
+	bit_pos = find_next_bit_le(&dentry_blk->dentry_bitmap,
+						NR_DENTRY_IN_BLOCK, 2);
+	f2fs_put_page(dentry_page, 1);
+	
+	if (bit_pos < NR_DENTRY_IN_BLOCK)
+		return false;

-		f2fs_put_page(dentry_page, 1);
+	memset(&map, 0, sizeof(map));
+	map.m_lblk = 1;
+	map.m_len = dir_blocks(dir) - 1;
+	map.m_seg_type = NO_CHECK_TYPE;
+
+	ret = f2fs_map_blocks(dir, &map, 0, F2FS_GET_BLOCK_FIEMAP);
+	if (ret)
+		return false;
+
+	if (map.m_flags & F2FS_MAP_FLAGS)
+		return false;

-		if (bit_pos < NR_DENTRY_IN_BLOCK)
-			return false;
-	}
 	return true;
 }

--
2.36.1



> - Eric