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

Message-ID: <6mjxlmvxs4p7k3rgs2cx3ny5u3o5tuikzpxxuqepq5yv6xcxk3@nvmzrpu2ooel>
Date: Wed, 5 Nov 2025 13:04:51 +0100
From: Jan Kara <jack@...e.cz>
To: sunyongjian1@...wei.com
Cc: linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org, 
	tytso@....edu, jack@...e.cz, 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

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
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR