linux-ext4 - Re: [PATCH v2 4/9] ext4: fix slab-out-of-bounds in ext4_mb_find_good_group_avg_frag

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]

Message-ID: <50f9333b-831a-8b4b-a6f2-ae79ab46a88b@huawei.com>
Date: Thu, 14 Mar 2024 19:24:56 +0800
From: Baokun Li <libaokun1@...wei.com>
To: Jan Kara <jack@...e.cz>
CC: <linux-ext4@...r.kernel.org>, <tytso@....edu>, <adilger.kernel@...ger.ca>,
	<ritesh.list@...il.com>, <ojaswin@...ux.ibm.com>, <adobriyan@...il.com>,
	<linux-kernel@...r.kernel.org>, <yi.zhang@...wei.com>,
	<yangerkun@...wei.com>, <stable@...r.kernel.org>, Baokun Li
	<libaokun1@...wei.com>
Subject: Re: [PATCH v2 4/9] ext4: fix slab-out-of-bounds in
 ext4_mb_find_good_group_avg_frag_lists()

Hi Jan,

On 2024/3/14 18:30, Jan Kara wrote:
> On Tue 27-02-24 17:11:43, Baokun Li wrote:
>
>
> At 4k block size, the length of the s_mb_avg_fragment_size list is 14,
> but an oversized s_mb_group_prealloc is set, causing slab-out-of-bounds
> to be triggered by an attempt to access an element at index 29.
>
> Add a new attr_id attr_clusters_in_group with values in the range
> [0, sbi->s_clusters_per_group] and declare mb_group_prealloc as
> that type to fix the issue. In addition avoid returning an order
> from mb_avg_fragment_size_order() greater than MB_NUM_ORDERS(sb)
> and reduce some useless loops.
>
> Fixes: 7e170922f06b ("ext4: Add allocation criteria 1.5 (CR1_5)")
> CC: stable@...r.kernel.org
> Signed-off-by: Baokun Li <libaokun1@...wei.com>
> Looks good. Just one nit below. Otherwise feel free to add:
>
> Reviewed-by: Jan Kara <jack@...e.cz>
>
>> ---
>>   fs/ext4/mballoc.c |  6 ++++++
>>   fs/ext4/sysfs.c   | 13 ++++++++++++-
>>   2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>> index 85a91a61b761..7ad089df2408 100644
>> --- a/fs/ext4/mballoc.c
>> +++ b/fs/ext4/mballoc.c
>> @@ -831,6 +831,8 @@ static int mb_avg_fragment_size_order(struct super_block *sb, ext4_grpblk_t len)
>>   		return 0;
>>   	if (order == MB_NUM_ORDERS(sb))
>>   		order--;
>> +	if (WARN_ON_ONCE(order > MB_NUM_ORDERS(sb)))
>> +		order = MB_NUM_ORDERS(sb) - 1;
>>   	return order;
>>   }
>>   
>> @@ -1057,6 +1059,10 @@ static void ext4_mb_choose_next_group_best_avail(struct ext4_allocation_context
>>   			ac->ac_flags |= EXT4_MB_CR_BEST_AVAIL_LEN_OPTIMIZED;
>>   			return;
>>   		}
>> +
>> +		/* Skip some unnecessary loops. */
>> +		if (unlikely(i > MB_NUM_ORDERS(ac->ac_sb)))
>> +			i = MB_NUM_ORDERS(ac->ac_sb);
> How can this possibly trigger now? MB_NUM_ORDERS is sb->s_blocksize_bits +
> 2. 'i' is starting at fls(ac->ac_g_ex.fe_len) and ac_g_ex.fe_len shouldn't
> be larger than clusters per group, hence fls() should be less than
> sb->s_blocksize_bits? Am I missing something? And if yes, we should rather
> make sure 'order' is never absurdly big?
>
> I suspect this code is defensive upto a point of being confusing :)
>
> Honza 

Yes, this is indeed defensive code! Only walk into this branch when
WARN_ON_ONCE(order > MB_NUM_ORDERS(sb)) is triggered.
As previously mentioned by ojaswin in the following link:

"The reason for this is that otherwise when order is large eg 29,
we would unnecessarily loop from i=29 to i=13 while always
looking at the same avg_fragment_list[13]."

Link：https://lore.kernel.org/lkml/ZdQ7FEA7KC4eAMpg@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com/

Thank you so much for the review! ღ( ´･ᴗ･` )
-- 
With Best Regards,
Baokun Li
.