linux-kernel - Re: [PATCH] ext4: No need to continue when the number of entries is 1

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] [day] [month] [year] [list]

Message-ID: <0dbb903b-7d13-494f-bba4-962820d561b6@huawei.com>
Date: Sat, 24 Aug 2024 12:20:26 +0800
From: Baokun Li <libaokun1@...wei.com>
To: Theodore Ts'o <tytso@....edu>
CC: Edward Adam Davis <eadavis@...com>,
	<syzbot+ae688d469e36fb5138d0@...kaller.appspotmail.com>,
	<adilger.kernel@...ger.ca>, <linux-ext4@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <syzkaller-bugs@...glegroups.com>
Subject: Re: [PATCH] ext4: No need to continue when the number of entries is 1

On 2024/8/24 0:05, Theodore Ts'o wrote:
> On Fri, Aug 23, 2024 at 10:22:19AM +0800, Baokun Li wrote:
>> I think this patch is wrong and it will hide the real problem.
>>
>> 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.
>>
>> The problem reported by syzbot was actually caused by a missing check in
>> make_indexed_dir(). The issue has been fixed:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=50ea741def58
>>
>> So unless ext4_dx_add_entry() and make_indexed_dir(), or some other function
>> has a bug, 'split == 0' will not occur.
>>
>> If we want to defend against future changes that introduce bugs, I think
>> it's better to add a WARN_ON_ONCE to make sure that the problem isn't hidden
>> and that it doesn't trigger serious bugs like out-of-bounds access.
> I agree that given your patch (50ea741def58: "ext4: check dot and
> dotdot of dx_root before making dir indexed") split should never be
> zero.  (Although there are two ways this could happen --- either count
> could be 0, or count == max).  But this patch isn't wrong per se
> because in the case where split == 0, we do want to prevent the
> out-of-bounds memory access bug.


Agreed, it is correct to avoid serious problems by judging the split,

I was thinking that it is wrong to report no error or hint when split == 0.

> That being said; adding a WARN_ON_ONCE(split == 0) might be a good
> idea, although I'd probably also print more debugging information so
> we can take a look at the file system and understand what might have
> happened.  Maybe something like this?
>
> 	if (WARN_ON_ONCE(split == 0)) {
> 	   	/* should never happen, but... */
> 		ext4_error_inode_block(dir, (*bh)->b_blocknr, 0,
> 				"bad indexed directory? hash=%08x:%08x "
> 				"count=%d move=%u", hinfo->hash, hinfo->minor_hash,
> 				count, move);
> 		brelse(*bh);
> 		brelse(bh2);
> 		*bh = 0;
> 		return ERR_PTR(-EFSCORRUPTED);
> 	}
>
> I haven't checked to make sure all of the error code paths / error
> handling right, but something like this might be useful for debugging
> purposes --- if the file system developer could get access to the file
> system moment the error is logged.  If the data center automation
> causes the file system to get fsck'ed or reformatted right away (which
> is the only scalable thing to do if there are millions of file systems
> in production :-), something like this is probably not going to help
> all that much.  Still, it certainly wouldn't hurt.

Totally agree! These printouts are very useful for debugging.

The modification above looks good. I tested it and it works fine.

But I think we could reuse the error handling code like this:


diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index e6769b97a970..0187910108c4 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1997,6 +1997,15 @@ static struct ext4_dir_entry_2 *do_split(handle_t 
*handle, struct inode *dir,
         else
                 split = count/2;

+       if (WARN_ON_ONCE(split == 0)) {
+               /* should never happen, but... */
+               ext4_error_inode_block(dir, (*bh)->b_blocknr, 0,
+                               "bad indexed directory? hash=%08x:%08x 
count=%d move=%u",
+                               hinfo->hash, hinfo->minor_hash, count, 
move);
+               err = -EFSCORRUPTED;
+               goto out;
+       }
+
         hash2 = map[split].hash;
         continued = hash2 == map[split - 1].hash;
         dxtrace(printk(KERN_INFO "Split block %lu at %x, %i/%i\n",
@@ -2040,10 +2049,11 @@ static struct ext4_dir_entry_2 
*do_split(handle_t *handle, struct inode *dir,
         return de;

  journal_error:
+       ext4_std_error(dir->i_sb, err);
+out:
         brelse(*bh);
         brelse(bh2);
         *bh = NULL;
-       ext4_std_error(dir->i_sb, err);
         return ERR_PTR(err);
  }

>
> If someone does think this would be helpful for them, I wouldn't
> object to adding a patch something like this.
>
> Cheers,
>
> 						- Ted
>
I think it's very helpful.

Thank you very much for your detailed explanation!


Cheers,
Baokun