linux-ext4 - Re: [PATCH 2/2] ext4: improve integrity checking in __mb_check

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: <2d7f50d1-36f0-452c-9bbe-4baaf7da34ce@huawei.com>
Date: Thu, 6 Nov 2025 10:59:22 +0800
From: Sun Yongjian <sunyongjian1@...wei.com>
To: Jan Kara <jack@...e.cz>
CC: <linux-ext4@...r.kernel.org>, <linux-fsdevel@...r.kernel.org>,
	<tytso@....edu>, <yangerkun@...wei.com>, <yi.zhang@...wei.com>,
	<libaokun1@...wei.com>, <chengzhihao1@...wei.com>
Subject: Re: [PATCH 2/2] ext4: improve integrity checking in __mb_check_buddy
 by enhancing order-0 validation



在 2025/11/5 20:04, Jan Kara 写道:
> On Wed 05-11-25 15:42:50, Yongjian Sun wrote:
>> From: Yongjian Sun <sunyongjian1@...wei.com>
>>
>> When the MB_CHECK_ASSERT macro is enabled, we found that the
>> current validation logic in __mb_check_buddy has a gap in
>> detecting certain invalid buddy states, particularly related
>> to order-0 (bitmap) bits.
>>
>> The original logic consists of three steps:
>> 1. Validates higher-order buddies: if a higher-order bit is
>> set, at most one of the two corresponding lower-order bits
>> may be free; if a higher-order bit is clear, both lower-order
>> bits must be allocated (and their bitmap bits must be 0).
>> 2. For any set bit in order-0, ensures all corresponding
>> higher-order bits are not free.
>> 3. Verifies that all preallocated blocks (pa) in the group
>> have pa_pstart within bounds and their bitmap bits marked as
>> allocated.
>>
>> However, this approach fails to properly validate cases where
>> order-0 bits are incorrectly cleared (0), allowing some invalid
>> configurations to pass:
>>
>>                 corrupt            integral
>>
>> order 3           1                  1
>> order 2       1       1          1       1
>> order 1     1   1   1   1      1   1   1   1
>> order 0    0 0 1 1 1 1 1 1    1 1 1 1 1 1 1 1
>>
>> Here we get two adjacent free blocks at order-0 with inconsistent
>> higher-order state, and the right one shows the correct scenario.
>>
>> The root cause is insufficient validation of order-0 zero bits.
>> To fix this and improve completeness without significant performance
>> cost, we refine the logic:
>>
>> 1. Maintain the top-down higher-order validation, but we no longer
>> check the cases where the higher-order bit is 0, as this case will
>> be covered in step 2.
>> 2. Enhance order-0 checking by examining pairs of bits:
>>     - If either bit in a pair is set (1), all corresponding
>>       higher-order bits must not be free.
>>     - If both bits are clear (0), then exactly one of the
>>       corresponding higher-order bits must be free
>> 3. Keep the preallocation (pa) validation unchanged.
>>
>> This change closes the validation gap, ensuring illegal buddy states
>> involving order-0 are correctly detected, while removing redundant
>> checks and maintaining efficiency.
>>
>> Fixes: c9de560ded61f ("ext4: Add multi block allocator for ext4")
>> Signed-off-by: Yongjian Sun <sunyongjian1@...wei.com>
>> Reviewed-by: Baokun Li <libaokun1@...wei.com>
> 
> The idea looks good but I have one question regarding the implementation...
> 
>> @@ -747,15 +756,29 @@ static void __mb_check_buddy(struct ext4_buddy *e4b, char *file,
>>   				fragments++;
>>   				fstart = i;
>>   			}
>> -			continue;
>> +		} else {
>> +			fstart = -1;
>>   		}
>> -		fstart = -1;
>> -		/* check used bits only */
>> -		for (j = 0; j < e4b->bd_blkbits + 1; j++) {
>> -			buddy2 = mb_find_buddy(e4b, j, &max2);
>> -			k = i >> j;
>> -			MB_CHECK_ASSERT(k < max2);
>> -			MB_CHECK_ASSERT(mb_test_bit(k, buddy2));
>> +		if (!(i & 1)) {
>> +			int in_use, zero_bit_count;
>> +
>> +			in_use = mb_test_bit(i, buddy) || mb_test_bit(i + 1, buddy);
>> +			zero_bit_count = 0;
>> +			for (j = 1; j < e4b->bd_blkbits + 2; j++) {
>> +				buddy2 = mb_find_buddy(e4b, j, &max2);
>> +				k = i >> j;
>> +				MB_CHECK_ASSERT(k < max2);
>> +				if (in_use) {
>> +					/* can not contain any 0 at all orders */
>> +					MB_CHECK_ASSERT(mb_test_bit(k, buddy2));
>> +				} else {
>> +					/* there is and can only be one 0 at all orders */
>> +					if (!mb_test_bit(k, buddy2)) {
>> +						zero_bit_count++;
>> +						MB_CHECK_ASSERT(zero_bit_count == 1);
>> +					}
>> +				}
> 
> Your variant doesn't seem to properly assert that at least 1 bit in the
> buddy is 0 above 0 bit in the bitmap because the MB_CHECK_ASSERT() doesn't
> get executed in that case at all AFAICT.  I think it would be more
> understandable to have the loop like:
> 
> 			for (j = 1; j < e4b->bd_blkbits + 2; j++) {
> 				buddy2 = mb_find_buddy(e4b, j, &max2);
> 				k = i >> j;
> 				MB_CHECK_ASSERT(k < max2);
> 				if (!mb_test_bit(k, buddy2))
> 					zero_bit_count++;
> 			}
> 			MB_CHECK_ASSERT(zero_bit_count == !in_use);
> 
> 									Honza

Thanks a lot for pointing out the logical flaw! Yes, you’re right—if 
order-0 bit pair is clear, then without a single 0 showing up at any 
higher order we’ll never enter the `if` branch to run `MB_CHECK_ASSERT`. 
The code you proposed is indeed a better, more elegant implementation!

Thanks,
Yongjian