linux-kernel - Re: [f2fs-dev] [PATCH 7/7] f2fs: should check the remaining dentry bits

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 27 Apr 2016 22:22:20 +0800
From:	Chao Yu <chao@...nel.org>
To:	Jaegeuk Kim <jaegeuk@...nel.org>, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org,
	linux-f2fs-devel@...ts.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH 7/7] f2fs: should check the remaining dentry
 bits

Hi Jaegeuk, Yunlei,

On 2016/4/26 8:07, Jaegeuk Kim wrote:
> Let's consider a race condition between f2fs_add_regular_entry and
> find_target_dentry.
> 
> 1.
> - f2fs_add_regular_entry updated len: 24 first.
>        |
> Bits:  0 0 0 1
> Lens: 24 0 0 3 (name: foo)
>        |->
> - find_target_dentry checks the first bit to find "foo", then ++pointer.
> 
> 2.
> - f2fs_add_regular_entry updates bits.
>        |>|>|
> Bits:  1 1 1 1
> Lens: 24 0 0 3 (name: foo)
>          |
> - find_target_dentry is checking second bit, but it's len is zero, which
> makes the process being terminated.

As Pengyang reminded, there are no racing condition between find_target_dentry
and f2fs_add_regular_entry since i_mutex lock make each of operations being
atomical. So seems above condition can not happen.

But still we should handle dirent with zero-sized length correctly, as it may
cause deadloop. So how do you think of following patch?

From: Chao Yu <yuchao0@...wei.com>
Subject: [PATCH] f2fs: be aware of invalid filename length

The filename length in dirent of may become zero-sized after random junk
data injection, once encounter such dirent, find_target_dentry or
f2fs_add_inline_entries will run into an infinite loop. So let f2fs being
aware of that to avoid deadloop.

Signed-off-by: Chao Yu <yuchao0@...wei.com>
---
 fs/f2fs/dir.c    | 14 +++++---------
 fs/f2fs/inline.c | 14 ++++++--------
 2 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index e90380d..3b1c14e 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -101,11 +101,6 @@ static struct f2fs_dir_entry *find_in_block(struct page
*dentry_page,
 	else
 		kunmap(dentry_page);

-	/*
-	 * For the most part, it should be a bug when name_len is zero.
-	 * We stop here for figuring out where the bugs has occurred.
-	 */
-	f2fs_bug_on(F2FS_P_SB(dentry_page), d.max < 0);
 	return de;
 }

@@ -130,6 +125,11 @@ struct f2fs_dir_entry *find_target_dentry(struct
fscrypt_name *fname,

 		de = &d->dentry[bit_pos];

+		if (unlikely(!de->name_len)) {
+			bit_pos++;
+			continue;
+		}
+
 		/* encrypted case */
 		de_name.name = d->filename[bit_pos];
 		de_name.len = le16_to_cpu(de->name_len);
@@ -147,10 +147,6 @@ struct f2fs_dir_entry *find_target_dentry(struct
fscrypt_name *fname,
 			*max_slots = max_len;
 		max_len = 0;

-		/* remain bug on condition */
-		if (unlikely(!de->name_len))
-			d->max = -1;
-
 		bit_pos += GET_DENTRY_SLOTS(le16_to_cpu(de->name_len));
 	}

diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index 7720565..e61084c 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -303,11 +303,6 @@ struct f2fs_dir_entry *find_in_inline_dir(struct inode *dir,
 	else
 		f2fs_put_page(ipage, 0);

-	/*
-	 * For the most part, it should be a bug when name_len is zero.
-	 * We stop here for figuring out where the bugs has occurred.
-	 */
-	f2fs_bug_on(sbi, d.max < 0);
 	return de;
 }

@@ -437,6 +432,12 @@ static int f2fs_add_inline_entries(struct inode *dir,
 		}

 		de = &d.dentry[bit_pos];
+
+		if (unlikely(!de->name_len)) {
+			bit_pos++;
+			continue;
+		}
+
 		new_name.name = d.filename[bit_pos];
 		new_name.len = de->name_len;

@@ -448,9 +449,6 @@ static int f2fs_add_inline_entries(struct inode *dir,
 		if (err)
 			goto punch_dentry_pages;

-		if (unlikely(!de->name_len))
-			d.max = -1;
-
 		bit_pos += GET_DENTRY_SLOTS(le16_to_cpu(de->name_len));
 	}
 	return 0;
-- 
2.7.2