Re: [PATCH 1/3] ext4: Fix an error handling path in ext4_mb_init_cache()

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

Le 06/01/2025 à 12:35, Dan Carpenter a écrit :
> On Fri, Jan 03, 2025 at 02:59:16PM +0100, Christophe JAILLET wrote:
>> 'bhs' is an un-initialized pointer.
>> If 'groups_per_page' == 1, 'bh' is assigned its address.
>>
>> Then, in the for loop below, if we early exit, either because
>> "group >= ngroups" or if ext4_get_group_info() fails, then it is still left
>> un-initialized.
>>
>> It can then be used.
>> NULL tests could fail and lead to unexpected behavior. Also, should the
>> error handling path be called, brelse() would be passed a potentially
>> invalid value.
>>
>> Better safe than sorry, just make sure it is correctly initialized to NULL.
>>
>> Fixes: c9de560ded61 ("ext4: Add multi block allocator for ext4")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> Compile tested only.
>>
>> The scenario looks possible, but I don't know if it can really happen...
> 

Hi Dan,

> A pointer to the stack can't ever equal the address of the heap so this
> can't happen and it should not have a Fixes tag.

Not sure to understand what you mean.

I agree with your statement, but my point is that a pointer in the stack 
(and not *to* the stack) (i.e. 'bhs'), if not initialized, could in 
theory be anything. Let consider its value is 0xdeadbeef.

Then, if groups_per_page == 1, 'bh' points to the stack. Its value is 
"&bhs". And "bh[0]" is 0xdeadbeef.


Should ext4_get_group_info() fail on the first (and only) iteration of 
the for loop, then we 'continue'.
So the loop is done, and bh[0] is never updated, so still points to a 
memory holding 0xdeadbeef.

On the next for loop, on the first (and only) iteration, bh[0] is not 
NULL (it is 0xdeadbeef), so we call:
	ext4_wait_block_bitmap(..., 0xdeadbeef);

If we branch to the error handling path, it would also lead to calling
	brelse(bh[0]), that is to say brelse(0xdeadbeef);


Hoping my analysis is correct, I hope my reasoning is clearer.


That's the theory.
In practice, see below. Certainly harmless thanks to compilers, but 
still a UB for me, so should need a Fixes and a backport (it can't hurt 
anyway) to fix the theory.

> Setting the pointer to NULL probably silences a static checker warning
> and these days everyone automatically zeroes stack data so it doesn't
> affect the compiled code.

Agreed, but unless we have a explicit gcc flag to ask for that behavior 
(I've not checked if it is already the case), it looks like an UB for me.

> However generally we generally say that we
> should fix the checker instead.

In this particular case, the checker is just me, not an static analysis 
tool :).

I looked at this place because one of my coccinelle script spotted:

	/* allocate buffer_heads to read bitmaps */
	if (groups_per_page > 1) {
		i = sizeof(struct buffer_head *) * groups_per_page;
		bh = kzalloc(i, gfp);

as a candidate for kcalloc().

The rest of the story is just by reading the code around it.

> 
> I've thought about this in Smatch for a while, and I think what I would
> do is say that kmalloc() returns memory that is unique.  Smatch tracks if
> variables are equal to each other and unique variables wouldn't be equal
> to anything that came earlier.  But I haven't actually tried to implement
> this.
> 
> regards,
> dan carpenter
>