Skip to contentSkip to conversation

ocfs2: zero-initialize recovery bitmap to prevent uninit-value in find_next_bit

PATCH In review Daniel Hodges 2 participants 2 messages Feb 9

Subsystems (2)
Stable branch OCFS2
lore
Share & tools
lore

Daniel Hodges

author
#0 lore raw

PATCH 1/1 ocfs2: zero-initialize recovery bitmap to prevent uninit-value in find_next_bit

ocfs2_add_recovery_chunk() allocates a bitmap buffer of sb->s_blocksize bytes using kmalloc() but only copies (ol_chunk_entries(sb) + 7) >> 3 bytes into it from the on-disk quota chunk. When the number of chunk entries is not aligned to a long boundary (64 bits on 64-bit systems), find_next_bit() reads uninitialized memory from the trailing bytes of the last word in the bitmap.

Fix this by using kzalloc() to zero-initialize the entire bitmap allocation, ensuring that any bits beyond the copied region are clean zeros.

Reported-by [email protected]
Fixes 2205363dce74 ("ocfs2: Implement quota recovery")
Cc [email protected]
Signed-off-by Daniel Hodges <[email protected]>
---
 fs/ocfs2/quota_local.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/ocfs2/quota_local.c +1 -1
index de7f12858729..bd3eb098097f 100644
@@ -302,7 +302,7 @@ static int ocfs2_add_recovery_chunk(struct super_block *sb,
 	if (!rc)
 		return -ENOMEM;
 	rc->rc_chunk = chunk;
-	rc->rc_bitmap = kmalloc(sb->s_blocksize, GFP_NOFS);
+	rc->rc_bitmap = kzalloc(sb->s_blocksize, GFP_NOFS);
 	if (!rc->rc_bitmap) {
 		kfree(rc);
 		return -ENOMEM;
Signature · 1 line
2.52.0
Reviewed-by on v1 1/1
#1 lore raw
Quoted 6 lines · Daniel Hodges

ocfs2_add_recovery_chunk() allocates a bitmap buffer of sb->s_blocksize bytes using kmalloc() but only copies (ol_chunk_entries(sb) + 7) >> 3 bytes into it from the on-disk quota chunk. When the number of chunk entries is not aligned to a long boundary (64 bits on 64-bit systems), find_next_bit() reads uninitialized memory from the trailing bytes of the last word in the bitmap.

OK, but AFAICS it does not impact the functionality in any way because we properly use:

for_each_set_bit(bit, rchunk->rc_bitmap, ol_chunk_entries(sb))

so the uninitialized bits are actually never used. This would be good to stress in the changelog. Since this isn't really a performance sensitive code I agree with the fix to make the checker happy anyway but I wanted to make sure... So feel free to add:

Reviewed-by Jan Kara <[email protected]>

Honza

Quoted 28 lines

Fix this by using kzalloc() to zero-initialize the entire bitmap allocation, ensuring that any bits beyond the copied region are clean zeros.

Reported-by [email protected]
Fixes 2205363dce74 ("ocfs2: Implement quota recovery")
Cc [email protected]
Signed-off-by Daniel Hodges <[email protected]>
---
 fs/ocfs2/quota_local.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/ocfs2/quota_local.c +1 -1
index de7f12858729..bd3eb098097f 100644
@@ -302,7 +302,7 @@ static int ocfs2_add_recovery_chunk(struct super_block *sb,
 	if (!rc)
 		return -ENOMEM;
 	rc->rc_chunk = chunk;
-	rc->rc_bitmap = kmalloc(sb->s_blocksize, GFP_NOFS);
+	rc->rc_bitmap = kzalloc(sb->s_blocksize, GFP_NOFS);
 	if (!rc->rc_bitmap) {
 		kfree(rc);
 		return -ENOMEM;
Signature · 1 line
2.52.0
Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR