* [PATCH 1/2] ext4: check dot and dotdot of dx_root before making dir indexed
2024-07-02 13:23 [PATCH 0/2] ext4: fix unable to handle kernel paging request in do_split() libaokun
@ 2024-07-02 13:23 ` libaokun
2024-07-03 9:29 ` Jan Kara
2024-07-02 13:23 ` [PATCH 2/2] ext4: make sure the first directory block is not a hole libaokun
2024-07-11 13:20 ` [PATCH 0/2] ext4: fix unable to handle kernel paging request in do_split() Theodore Ts'o
2 siblings, 1 reply; 6+ messages in thread
From: libaokun @ 2024-07-02 13:23 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
yangerkun, libaokun, Baokun Li, syzbot+ae688d469e36fb5138d0,
stable
From: Baokun Li <libaokun1@huawei.com>
Syzbot reports a issue as follows:
============================================
BUG: unable to handle page fault for address: ffffed11022e24fe
PGD 23ffee067 P4D 23ffee067 PUD 0
Oops: Oops: 0000 [#1] PREEMPT SMP KASAN PTI
CPU: 0 PID: 5079 Comm: syz-executor306 Not tainted 6.10.0-rc5-g55027e689933 #0
Call Trace:
<TASK>
make_indexed_dir+0xdaf/0x13c0 fs/ext4/namei.c:2341
ext4_add_entry+0x222a/0x25d0 fs/ext4/namei.c:2451
ext4_rename fs/ext4/namei.c:3936 [inline]
ext4_rename2+0x26e5/0x4370 fs/ext4/namei.c:4214
[...]
============================================
The immediate cause of this problem is that there is only one valid dentry
for the block to be split during do_split, so split==0 results in out of
bounds accesses to the map triggering the issue.
do_split
unsigned split
dx_make_map
count = 1
split = count/2 = 0;
continued = hash2 == map[split - 1].hash;
---> map[4294967295]
The maximum length of a filename is 255 and the minimum block size is 1024,
so it is always guaranteed that the number of entries is greater than or
equal to 2 when do_split() is called.
But syzbot's crafted image has no dot and dotdot in dir, and the dentry
distribution in dirblock is as follows:
bus dentry1 hole dentry2 free
|xx--|xx-------------|...............|xx-------------|...............|
0 12 (8+248)=256 268 256 524 (8+256)=264 788 236 1024
So when renaming dentry1 increases its name_len length by 1, neither hole
nor free is sufficient to hold the new dentry, and make_indexed_dir() is
called.
In make_indexed_dir() it is assumed that the first two entries of the
dirblock must be dot and dotdot, so bus and dentry1 are left in dx_root
because they are treated as dot and dotdot, and only dentry2 is moved
to the new leaf block. That's why count is equal to 1.
Therefore add the ext4_check_dx_root() helper function to add more sanity
checks to dot and dotdot before starting the conversion to avoid the above
issue.
Reported-by: syzbot+ae688d469e36fb5138d0@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=ae688d469e36fb5138d0
Fixes: ac27a0ec112a ("[PATCH] ext4: initial copy of files from ext3")
Cc: stable@kernel.org
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
fs/ext4/namei.c | 56 ++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 51 insertions(+), 5 deletions(-)
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index e6769b97a970..35881e3dd880 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -2172,6 +2172,52 @@ static int add_dirent_to_buf(handle_t *handle, struct ext4_filename *fname,
return err ? err : err2;
}
+static bool ext4_check_dx_root(struct inode *dir, struct dx_root *root)
+{
+ struct fake_dirent *fde;
+ const char *error_msg;
+ unsigned int rlen;
+ unsigned int blocksize = dir->i_sb->s_blocksize;
+ char *blockend = (char *)root + dir->i_sb->s_blocksize;
+
+ fde = &root->dot;
+ if (unlikely(fde->name_len != 1)) {
+ error_msg = "invalid name_len for '.'";
+ goto corrupted;
+ }
+ if (unlikely(strncmp(root->dot_name, ".", fde->name_len))) {
+ error_msg = "invalid name for '.'";
+ goto corrupted;
+ }
+ rlen = ext4_rec_len_from_disk(fde->rec_len, blocksize);
+ if (unlikely((char *)fde + rlen >= blockend)) {
+ error_msg = "invalid rec_len for '.'";
+ goto corrupted;
+ }
+
+ fde = &root->dotdot;
+ if (unlikely(fde->name_len != 2)) {
+ error_msg = "invalid name_len for '..'";
+ goto corrupted;
+ }
+ if (unlikely(strncmp(root->dotdot_name, "..", fde->name_len))) {
+ error_msg = "invalid name for '..'";
+ goto corrupted;
+ }
+ rlen = ext4_rec_len_from_disk(fde->rec_len, blocksize);
+ if (unlikely((char *)fde + rlen >= blockend)) {
+ error_msg = "invalid rec_len for '..'";
+ goto corrupted;
+ }
+
+ return true;
+
+corrupted:
+ EXT4_ERROR_INODE(dir, "Corrupt dir, %s, running e2fsck is recommended",
+ error_msg);
+ return false;
+}
+
/*
* This converts a one block unindexed directory to a 3 block indexed
* directory, and adds the dentry to the indexed directory.
@@ -2206,17 +2252,17 @@ static int make_indexed_dir(handle_t *handle, struct ext4_filename *fname,
brelse(bh);
return retval;
}
+
root = (struct dx_root *) bh->b_data;
+ if (!ext4_check_dx_root(dir, root)) {
+ brelse(bh);
+ return -EFSCORRUPTED;
+ }
/* The 0th block becomes the root, move the dirents out */
fde = &root->dotdot;
de = (struct ext4_dir_entry_2 *)((char *)fde +
ext4_rec_len_from_disk(fde->rec_len, blocksize));
- if ((char *) de >= (((char *) root) + blocksize)) {
- EXT4_ERROR_INODE(dir, "invalid rec_len for '..'");
- brelse(bh);
- return -EFSCORRUPTED;
- }
len = ((char *) root) + (blocksize - csum_size) - (char *) de;
/* Allocate new block for the 0th block's dirents */
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] ext4: check dot and dotdot of dx_root before making dir indexed
2024-07-02 13:23 ` [PATCH 1/2] ext4: check dot and dotdot of dx_root before making dir indexed libaokun
@ 2024-07-03 9:29 ` Jan Kara
0 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2024-07-03 9:29 UTC (permalink / raw)
To: libaokun
Cc: linux-ext4, tytso, adilger.kernel, jack, ritesh.list,
linux-kernel, yi.zhang, yangerkun, Baokun Li,
syzbot+ae688d469e36fb5138d0, stable
On Tue 02-07-24 21:23:48, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
>
> Syzbot reports a issue as follows:
> ============================================
> BUG: unable to handle page fault for address: ffffed11022e24fe
> PGD 23ffee067 P4D 23ffee067 PUD 0
> Oops: Oops: 0000 [#1] PREEMPT SMP KASAN PTI
> CPU: 0 PID: 5079 Comm: syz-executor306 Not tainted 6.10.0-rc5-g55027e689933 #0
> Call Trace:
> <TASK>
> make_indexed_dir+0xdaf/0x13c0 fs/ext4/namei.c:2341
> ext4_add_entry+0x222a/0x25d0 fs/ext4/namei.c:2451
> ext4_rename fs/ext4/namei.c:3936 [inline]
> ext4_rename2+0x26e5/0x4370 fs/ext4/namei.c:4214
> [...]
> ============================================
>
> The immediate cause of this problem is that there is only one valid dentry
> for the block to be split during do_split, so split==0 results in out of
> bounds accesses to the map triggering the issue.
>
> do_split
> unsigned split
> dx_make_map
> count = 1
> split = count/2 = 0;
> continued = hash2 == map[split - 1].hash;
> ---> map[4294967295]
>
> The maximum length of a filename is 255 and the minimum block size is 1024,
> so it is always guaranteed that the number of entries is greater than or
> equal to 2 when do_split() is called.
>
> But syzbot's crafted image has no dot and dotdot in dir, and the dentry
> distribution in dirblock is as follows:
>
> bus dentry1 hole dentry2 free
> |xx--|xx-------------|...............|xx-------------|...............|
> 0 12 (8+248)=256 268 256 524 (8+256)=264 788 236 1024
>
> So when renaming dentry1 increases its name_len length by 1, neither hole
> nor free is sufficient to hold the new dentry, and make_indexed_dir() is
> called.
>
> In make_indexed_dir() it is assumed that the first two entries of the
> dirblock must be dot and dotdot, so bus and dentry1 are left in dx_root
> because they are treated as dot and dotdot, and only dentry2 is moved
> to the new leaf block. That's why count is equal to 1.
>
> Therefore add the ext4_check_dx_root() helper function to add more sanity
> checks to dot and dotdot before starting the conversion to avoid the above
> issue.
>
> Reported-by: syzbot+ae688d469e36fb5138d0@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=ae688d469e36fb5138d0
> Fixes: ac27a0ec112a ("[PATCH] ext4: initial copy of files from ext3")
> Cc: stable@kernel.org
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
Thanks! The patch looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/ext4/namei.c | 56 ++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 51 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index e6769b97a970..35881e3dd880 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -2172,6 +2172,52 @@ static int add_dirent_to_buf(handle_t *handle, struct ext4_filename *fname,
> return err ? err : err2;
> }
>
> +static bool ext4_check_dx_root(struct inode *dir, struct dx_root *root)
> +{
> + struct fake_dirent *fde;
> + const char *error_msg;
> + unsigned int rlen;
> + unsigned int blocksize = dir->i_sb->s_blocksize;
> + char *blockend = (char *)root + dir->i_sb->s_blocksize;
> +
> + fde = &root->dot;
> + if (unlikely(fde->name_len != 1)) {
> + error_msg = "invalid name_len for '.'";
> + goto corrupted;
> + }
> + if (unlikely(strncmp(root->dot_name, ".", fde->name_len))) {
> + error_msg = "invalid name for '.'";
> + goto corrupted;
> + }
> + rlen = ext4_rec_len_from_disk(fde->rec_len, blocksize);
> + if (unlikely((char *)fde + rlen >= blockend)) {
> + error_msg = "invalid rec_len for '.'";
> + goto corrupted;
> + }
> +
> + fde = &root->dotdot;
> + if (unlikely(fde->name_len != 2)) {
> + error_msg = "invalid name_len for '..'";
> + goto corrupted;
> + }
> + if (unlikely(strncmp(root->dotdot_name, "..", fde->name_len))) {
> + error_msg = "invalid name for '..'";
> + goto corrupted;
> + }
> + rlen = ext4_rec_len_from_disk(fde->rec_len, blocksize);
> + if (unlikely((char *)fde + rlen >= blockend)) {
> + error_msg = "invalid rec_len for '..'";
> + goto corrupted;
> + }
> +
> + return true;
> +
> +corrupted:
> + EXT4_ERROR_INODE(dir, "Corrupt dir, %s, running e2fsck is recommended",
> + error_msg);
> + return false;
> +}
> +
> /*
> * This converts a one block unindexed directory to a 3 block indexed
> * directory, and adds the dentry to the indexed directory.
> @@ -2206,17 +2252,17 @@ static int make_indexed_dir(handle_t *handle, struct ext4_filename *fname,
> brelse(bh);
> return retval;
> }
> +
> root = (struct dx_root *) bh->b_data;
> + if (!ext4_check_dx_root(dir, root)) {
> + brelse(bh);
> + return -EFSCORRUPTED;
> + }
>
> /* The 0th block becomes the root, move the dirents out */
> fde = &root->dotdot;
> de = (struct ext4_dir_entry_2 *)((char *)fde +
> ext4_rec_len_from_disk(fde->rec_len, blocksize));
> - if ((char *) de >= (((char *) root) + blocksize)) {
> - EXT4_ERROR_INODE(dir, "invalid rec_len for '..'");
> - brelse(bh);
> - return -EFSCORRUPTED;
> - }
> len = ((char *) root) + (blocksize - csum_size) - (char *) de;
>
> /* Allocate new block for the 0th block's dirents */
> --
> 2.39.2
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] ext4: make sure the first directory block is not a hole
2024-07-02 13:23 [PATCH 0/2] ext4: fix unable to handle kernel paging request in do_split() libaokun
2024-07-02 13:23 ` [PATCH 1/2] ext4: check dot and dotdot of dx_root before making dir indexed libaokun
@ 2024-07-02 13:23 ` libaokun
2024-07-03 9:36 ` Jan Kara
2024-07-11 13:20 ` [PATCH 0/2] ext4: fix unable to handle kernel paging request in do_split() Theodore Ts'o
2 siblings, 1 reply; 6+ messages in thread
From: libaokun @ 2024-07-02 13:23 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
yangerkun, libaokun, Baokun Li, syzbot+ae688d469e36fb5138d0,
stable
From: Baokun Li <libaokun1@huawei.com>
The syzbot constructs a directory that has no dirblock but is non-inline,
i.e. the first directory block is a hole. And no errors are reported when
creating files in this directory in the following flow.
ext4_mknod
...
ext4_add_entry
// Read block 0
ext4_read_dirblock(dir, block, DIRENT)
bh = ext4_bread(NULL, inode, block, 0)
if (!bh && (type == INDEX || type == DIRENT_HTREE))
// The first directory block is a hole
// But type == DIRENT, so no error is reported.
After that, we get a directory block without '.' and '..' but with a valid
dentry. This may cause some code that relies on dot or dotdot (such as
make_indexed_dir()) to crash.
Therefore when ext4_read_dirblock() finds that the first directory block
is a hole report that the filesystem is corrupted and return an error to
avoid loading corrupted data from disk causing something bad.
Reported-by: syzbot+ae688d469e36fb5138d0@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=ae688d469e36fb5138d0
Fixes: 4e19d6b65fb4 ("ext4: allow directory holes")
Cc: stable@kernel.org
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
fs/ext4/namei.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 35881e3dd880..6a95713f9193 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -151,10 +151,11 @@ static struct buffer_head *__ext4_read_dirblock(struct inode *inode,
return bh;
}
- if (!bh && (type == INDEX || type == DIRENT_HTREE)) {
+ /* The first directory block must not be a hole. */
+ if (!bh && (type == INDEX || type == DIRENT_HTREE || block == 0)) {
ext4_error_inode(inode, func, line, block,
- "Directory hole found for htree %s block",
- (type == INDEX) ? "index" : "leaf");
+ "Directory hole found for htree %s block %u",
+ (type == INDEX) ? "index" : "leaf", block);
return ERR_PTR(-EFSCORRUPTED);
}
if (!bh)
@@ -3084,10 +3085,7 @@ bool ext4_empty_dir(struct inode *inode)
EXT4_ERROR_INODE(inode, "invalid size");
return false;
}
- /* The first directory block must not be a hole,
- * so treat it as DIRENT_HTREE
- */
- bh = ext4_read_dirblock(inode, 0, DIRENT_HTREE);
+ bh = ext4_read_dirblock(inode, 0, EITHER);
if (IS_ERR(bh))
return false;
@@ -3529,10 +3527,7 @@ static struct buffer_head *ext4_get_first_dir_block(handle_t *handle,
struct ext4_dir_entry_2 *de;
unsigned int offset;
- /* The first directory block must not be a hole, so
- * treat it as DIRENT_HTREE
- */
- bh = ext4_read_dirblock(inode, 0, DIRENT_HTREE);
+ bh = ext4_read_dirblock(inode, 0, EITHER);
if (IS_ERR(bh)) {
*retval = PTR_ERR(bh);
return NULL;
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] ext4: make sure the first directory block is not a hole
2024-07-02 13:23 ` [PATCH 2/2] ext4: make sure the first directory block is not a hole libaokun
@ 2024-07-03 9:36 ` Jan Kara
0 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2024-07-03 9:36 UTC (permalink / raw)
To: libaokun
Cc: linux-ext4, tytso, adilger.kernel, jack, ritesh.list,
linux-kernel, yi.zhang, yangerkun, Baokun Li,
syzbot+ae688d469e36fb5138d0, stable
On Tue 02-07-24 21:23:49, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
>
> The syzbot constructs a directory that has no dirblock but is non-inline,
> i.e. the first directory block is a hole. And no errors are reported when
> creating files in this directory in the following flow.
>
> ext4_mknod
> ...
> ext4_add_entry
> // Read block 0
> ext4_read_dirblock(dir, block, DIRENT)
> bh = ext4_bread(NULL, inode, block, 0)
> if (!bh && (type == INDEX || type == DIRENT_HTREE))
> // The first directory block is a hole
> // But type == DIRENT, so no error is reported.
>
> After that, we get a directory block without '.' and '..' but with a valid
> dentry. This may cause some code that relies on dot or dotdot (such as
> make_indexed_dir()) to crash.
>
> Therefore when ext4_read_dirblock() finds that the first directory block
> is a hole report that the filesystem is corrupted and return an error to
> avoid loading corrupted data from disk causing something bad.
>
> Reported-by: syzbot+ae688d469e36fb5138d0@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=ae688d469e36fb5138d0
> Fixes: 4e19d6b65fb4 ("ext4: allow directory holes")
> Cc: stable@kernel.org
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/ext4/namei.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 35881e3dd880..6a95713f9193 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -151,10 +151,11 @@ static struct buffer_head *__ext4_read_dirblock(struct inode *inode,
>
> return bh;
> }
> - if (!bh && (type == INDEX || type == DIRENT_HTREE)) {
> + /* The first directory block must not be a hole. */
> + if (!bh && (type == INDEX || type == DIRENT_HTREE || block == 0)) {
> ext4_error_inode(inode, func, line, block,
> - "Directory hole found for htree %s block",
> - (type == INDEX) ? "index" : "leaf");
> + "Directory hole found for htree %s block %u",
> + (type == INDEX) ? "index" : "leaf", block);
> return ERR_PTR(-EFSCORRUPTED);
> }
> if (!bh)
> @@ -3084,10 +3085,7 @@ bool ext4_empty_dir(struct inode *inode)
> EXT4_ERROR_INODE(inode, "invalid size");
> return false;
> }
> - /* The first directory block must not be a hole,
> - * so treat it as DIRENT_HTREE
> - */
> - bh = ext4_read_dirblock(inode, 0, DIRENT_HTREE);
> + bh = ext4_read_dirblock(inode, 0, EITHER);
> if (IS_ERR(bh))
> return false;
>
> @@ -3529,10 +3527,7 @@ static struct buffer_head *ext4_get_first_dir_block(handle_t *handle,
> struct ext4_dir_entry_2 *de;
> unsigned int offset;
>
> - /* The first directory block must not be a hole, so
> - * treat it as DIRENT_HTREE
> - */
> - bh = ext4_read_dirblock(inode, 0, DIRENT_HTREE);
> + bh = ext4_read_dirblock(inode, 0, EITHER);
> if (IS_ERR(bh)) {
> *retval = PTR_ERR(bh);
> return NULL;
> --
> 2.39.2
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] ext4: fix unable to handle kernel paging request in do_split()
2024-07-02 13:23 [PATCH 0/2] ext4: fix unable to handle kernel paging request in do_split() libaokun
2024-07-02 13:23 ` [PATCH 1/2] ext4: check dot and dotdot of dx_root before making dir indexed libaokun
2024-07-02 13:23 ` [PATCH 2/2] ext4: make sure the first directory block is not a hole libaokun
@ 2024-07-11 13:20 ` Theodore Ts'o
2 siblings, 0 replies; 6+ messages in thread
From: Theodore Ts'o @ 2024-07-11 13:20 UTC (permalink / raw)
To: linux-ext4, libaokun
Cc: Theodore Ts'o, adilger.kernel, jack, ritesh.list,
linux-kernel, yi.zhang, yangerkun, Baokun Li
On Tue, 02 Jul 2024 21:23:47 +0800, libaokun@huaweicloud.com wrote:
> "kvm-xfstests -c ext4/all -g auto" has been executed with no new failures.
>
> Baokun Li (2):
> ext4: check dot and dotdot of dx_root before making dir indexed
> ext4: make sure the first directory block is not a hole
>
> fs/ext4/namei.c | 73 ++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 57 insertions(+), 16 deletions(-)
>
> [...]
Applied, thanks!
[1/2] ext4: check dot and dotdot of dx_root before making dir indexed
commit: 50ea741def587a64e08879ce6c6a30131f7111e7
[2/2] ext4: make sure the first directory block is not a hole
commit: f9ca51596bbfd0f9c386dd1c613c394c78d9e5e6
Best regards,
--
Theodore Ts'o <tytso@mit.edu>
^ permalink raw reply [flat|nested] 6+ messages in thread