| 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
| ||
|
Message-ID: <5bd0a3a3-f38a-bd63-21e7-d649b2fb47b0@huawei.com>
Date: Fri, 21 Jul 2023 15:29:35 +0800
From: Baokun Li <libaokun1@...wei.com>
To: "Ritesh Harjani (IBM)" <ritesh.list@...il.com>,
<linux-ext4@...r.kernel.org>
CC: <tytso@....edu>, <adilger.kernel@...ger.ca>, <jack@...e.cz>,
<ojaswin@...ux.ibm.com>, <linux-kernel@...r.kernel.org>,
<yi.zhang@...wei.com>, <yangerkun@...wei.com>,
<yukuai3@...wei.com>, Baokun Li <libaokun1@...wei.com>
Subject: Re: [PATCH 2/4] ext4: fix BUG in ext4_mb_new_inode_pa() due to
overflow
On 2023/7/21 3:30, Ritesh Harjani (IBM) wrote:
>
>>> I would like to carefully review all such paths. I will soon review and
>>> get back.
>> Okay, thank you very much for your careful review.
>> The 2nd and 3rd cases of adjusting the best extent are impossible to
>> overflow,
>> so only the first case is converted here.
> I noticed them too during review. I think it would be safe to make the
> changes in other two places as well such that in future we never
> trip over such overlooked overflow bugs.
>
>>>
>>>> + BUG_ON(new_bex_end >
>>>> + fex_end(sbi, &ac->ac_g_ex, &ac->ac_orig_goal_len));
>>> I am not sure whether using fex_end or pa_end is any helpful.
>>> I think we can just typecast if needed and keep it simple rather
>>> than adding helpers functions for addition operation.
>>> (because of the fact that fex_end() can take a third parameter which
>>> sometimes you pass as NULL. Hence it doesn't look clean, IMO)
>> I added the helper functions here for two reasons:
>> 1. restricting the type of the return value.
>> 2. This avoids the ugly line breaks in most cases.
>>
>> The fex_end() indeed doesn't look as clean as the pa_end(), because we
>> might use
>> the start of the free extent plus some other length to get a new end,
>> like right in
>> ext4_mb_new_inode_pa(), which makes me have to add another extra length
>> argument, but I think it's worth it, and even with the addition of a
>> parameter
>> that will probably be unused, it still looks a lot shorter than the
>> original code.
> IMO, we don't need pa_end() and fex_end() at all. In several places in
> ext4 we always have taken care by directly typecasting to avoid
> overflows. Also it reads much simpler rather to typecast in place than
> having a helper function which is also not very elegant due to a third
> parameter. Hence I think we should drop those helpers.
I still think helper is useful, but my previous thinking is problematic.
I shouldn't
make fex_end() adapt to ext4_mb_new_inode_pa(), but should make
ext4_mb_new_inode_pa() adapt to fex_end(). After dropping the third argument
of fex_end(), modify the ext4_mb_new_inode_pa() function as follows:
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index a2475b8c9fb5..7492ba9062f4 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -5072,8 +5072,11 @@ ext4_mb_new_inode_pa(struct
ext4_allocation_context *ac)
pa = ac->ac_pa;
if (ac->ac_b_ex.fe_len < ac->ac_orig_goal_len) {
- int new_bex_start;
- int new_bex_end;
+ struct ext4_free_extent ex = {
+ .fe_logical = ac->ac_g_ex.fe_logical;
+ .fe_len = ac->ac_orig_goal_len;
+ }
+ loff_t orig_goal_end = fex_end(sbi, &ex);
/* we can't allocate as much as normalizer wants.
* so, found space must get proper lstart
@@ -5092,29 +5095,23 @@ ext4_mb_new_inode_pa(struct
ext4_allocation_context *ac)
* still cover original start
* 3. Else, keep the best ex at start of original request.
*/
- new_bex_end = ac->ac_g_ex.fe_logical +
- EXT4_C2B(sbi, ac->ac_orig_goal_len);
- new_bex_start = new_bex_end - EXT4_C2B(sbi,
ac->ac_b_ex.fe_len);
- if (ac->ac_o_ex.fe_logical >= new_bex_start)
- goto adjust_bex;
+ ex.fe_len = ac->ac_b_ex.fe_len;
- new_bex_start = ac->ac_g_ex.fe_logical;
- new_bex_end =
- new_bex_start + EXT4_C2B(sbi, ac->ac_b_ex.fe_len);
- if (ac->ac_o_ex.fe_logical < new_bex_end)
+ ex.fe_logical = orig_goal_end - EXT4_C2B(sbi, ex.fe_len);
+ if (ac->ac_o_ex.fe_logical >= ex.fe_logical)
goto adjust_bex;
- new_bex_start = ac->ac_o_ex.fe_logical;
- new_bex_end =
- new_bex_start + EXT4_C2B(sbi, ac->ac_b_ex.fe_len);
+ ex.fe_logical = ac->ac_g_ex.fe_logical;
+ if (ac->ac_o_ex.fe_logical < fex_end(sbi, &ex))
+ goto adjust_bex;
+ ex.fe_logical = ac->ac_o_ex.fe_logical;
adjust_bex:
- ac->ac_b_ex.fe_logical = new_bex_start;
+ ac->ac_b_ex.fe_logical = ex.fe_logical;
BUG_ON(ac->ac_o_ex.fe_logical < ac->ac_b_ex.fe_logical);
BUG_ON(ac->ac_o_ex.fe_len > ac->ac_b_ex.fe_len);
- BUG_ON(new_bex_end > (ac->ac_g_ex.fe_logical +
- EXT4_C2B(sbi, ac->ac_orig_goal_len)));
+ BUG_ON(fex_end(sbi, &ex) > orig_goal_end);
}
pa->pa_lstart = ac->ac_b_ex.fe_logical;
What do you think of this modification?
> Thanks once again for catching the overflows and coming up with a
> easy reproducer. I am surprised that this bug was never caught with LTP,
> fstests, smatch static checker.
> How did you find it? :)
>
> -ritesh
This problem is found in the internal test.
Thank you for your review!
--
With Best Regards,
Baokun Li
.
Powered by blists - more mailing lists