| Message ID | 3921e725586edaca611fd3de388f917e959dc85d.1735912719.git.christophe.jaillet@wanadoo.fr |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] ext4: Fix an error handling path in ext4_mb_init_cache() | expand |
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... 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. 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. However generally we generally say that we should fix the checker instead. 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
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 >
Ah. Sorry, I misunderstood the issue. I can't say for sure whether the bug is possible but it seems like a reasonable concern. regards, dan carpenter
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c index b25a27c86696..ff9a124f439b 100644 --- a/fs/ext4/mballoc.c +++ b/fs/ext4/mballoc.c @@ -1285,7 +1285,7 @@ static int ext4_mb_init_cache(struct folio *folio, char *incore, gfp_t gfp) ext4_group_t first_group, group; int first_block; struct super_block *sb; - struct buffer_head *bhs; + struct buffer_head *bhs = NULL; struct buffer_head **bh = NULL; struct inode *inode; char *data;
'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... --- fs/ext4/mballoc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)