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@xxxxxxxxxx>
---
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
