[PATCH 0/1] btrfs: strengthen integer overflow protection in batch allocation

[PATCH 0/1] btrfs: strengthen integer overflow protection in batch allocation

[kmpfqgdwxucqz9]

From: KernelKraze <admin@mail.free-proletariat.dpdns.org>

Hi,

This patch improves robustness in the btrfs filesystem by adding integer
overflow protection during batch allocation in flush_dir_items_batch().

The improvement was identified during a systematic code review of kernel
subsystems. Without proper bounds checking, theoretical integer overflow
could occur with extremely large directory item counts.

The fix implements proper overflow checking using the kernel's overflow
detection helpers and adds a reasonable upper limit consistent with other
btrfs batch operations.

This has been compile-tested and the fix aligns with existing patterns
in the btrfs codebase (log_delayed_insertion_items uses the same 195 limit).
The patch passes checkpatch.pl with no errors or warnings.

I've CC'd the btrfs maintainers for review.

Thanks,
KernelKraze

KernelKraze (1):
  btrfs: add integer overflow protection to flush_dir_items_batch allocation

 fs/btrfs/tree-log.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

-- 
2.48.1

[PATCH 1/1] btrfs: add integer overflow protection to flush_dir_items_batch allocation

[kmpfqgdwxucqz9]

From: KernelKraze <admin@mail.free-proletariat.dpdns.org>

The flush_dir_items_batch() function performs memory allocation using
count * sizeof(u32) + count * sizeof(struct btrfs_key) without proper
integer overflow checking. When count is large enough, this multiplication
can overflow, resulting in an allocation smaller than expected, leading to
buffer overflows during subsequent array access.

In extreme cases with very large directory item counts, this could
theoretically lead to undersized memory allocation, though such scenarios
are unlikely in normal filesystem usage.

Fix this by:
1. Adding a reasonable upper limit (195) to the batch size, consistent
   with the limit used in log_delayed_insertion_items()
2. Using check_mul_overflow() and check_add_overflow() to detect integer
   overflows before performing the allocation
3. Returning -EOVERFLOW when overflow is detected
4. Adding appropriate warning and error messages for debugging

This ensures that memory allocations are always sized correctly and
prevents potential issues from integer overflow conditions, improving
overall code robustness.

Signed-off-by: KernelKraze <admin@mail.free-proletariat.dpdns.org>
---
 fs/btrfs/tree-log.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 9f05d454b9df..19b443314db0 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3655,14 +3655,35 @@ static int flush_dir_items_batch(struct btrfs_trans_handle *trans,
 	} else {
 		struct btrfs_key *ins_keys;
 		u32 *ins_sizes;
+		size_t keys_size, sizes_size, total_size;
 
-		ins_data = kmalloc(count * sizeof(u32) +
-				   count * sizeof(struct btrfs_key), GFP_NOFS);
+		/*
+		 * Prevent integer overflow when calculating allocation size.
+		 * We use the same reasonable limit as log_delayed_insertion_items()
+		 * to prevent excessive memory allocation and potential DoS.
+		 */
+		if (count > 195) {
+			btrfs_warn(inode->root->fs_info,
+				   "dir items batch size %d exceeds safe limit, truncating",
+				   count);
+			count = 195;
+		}
+
+		/* Check for overflow in size calculations */
+		if (check_mul_overflow(count, sizeof(u32), &sizes_size) ||
+		    check_mul_overflow(count, sizeof(struct btrfs_key), &keys_size) ||
+		    check_add_overflow(sizes_size, keys_size, &total_size)) {
+			btrfs_err(inode->root->fs_info,
+				  "integer overflow in batch allocation size calculation");
+			return -EOVERFLOW;
+		}
+
+		ins_data = kmalloc(total_size, GFP_NOFS);
 		if (!ins_data)
 			return -ENOMEM;
 
 		ins_sizes = (u32 *)ins_data;
-		ins_keys = (struct btrfs_key *)(ins_data + count * sizeof(u32));
+		ins_keys = (struct btrfs_key *)(ins_data + sizes_size);
 		batch.keys = ins_keys;
 		batch.data_sizes = ins_sizes;
 		batch.total_data_size = 0;
-- 
2.48.1

Re: [PATCH 1/1] btrfs: add integer overflow protection to flush_dir_items_batch allocation

[Johannes Thumshirn]

On 7/30/25 6:44 AM, kmpfqgdwxucqz9@gmail.com wrote:
> iff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 9f05d454b9df..19b443314db0 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -3655,14 +3655,35 @@ static int flush_dir_items_batch(struct btrfs_trans_handle *trans,
>   	} else {
>   		struct btrfs_key *ins_keys;
>   		u32 *ins_sizes;
> +		size_t keys_size, sizes_size, total_size;
>   
> -		ins_data = kmalloc(count * sizeof(u32) +
> -				   count * sizeof(struct btrfs_key), GFP_NOFS);
> +		/*
> +		 * Prevent integer overflow when calculating allocation size.
> +		 * We use the same reasonable limit as log_delayed_insertion_items()
> +		 * to prevent excessive memory allocation and potential DoS.
> +		 */
> +		if (count > 195) {
> +			btrfs_warn(inode->root->fs_info,
> +				   "dir items batch size %d exceeds safe limit, truncating",
> +				   count);
> +			count = 195;
> +		}

Where does this number come from?


> +		/* Check for overflow in size calculations */
> +		if (check_mul_overflow(count, sizeof(u32), &sizes_size) ||
> +		    check_mul_overflow(count, sizeof(struct btrfs_key), &keys_size) ||
> +		    check_add_overflow(sizes_size, keys_size, &total_size)) {
> +			btrfs_err(inode->root->fs_info,
> +				  "integer overflow in batch allocation size calculation");
> +			return -EOVERFLOW;
> +		}
> +
> +		ins_data = kmalloc(total_size, GFP_NOFS);

Wouldn't kcalloc()  or kmalloc_array() be the better choice here? 
kcalloc() calls  kmalloc_array() which in term does overflow checking.

Re: [PATCH 1/1] btrfs: add integer overflow protection to flush_dir_items_batch allocation

[kmpfqgdwxucqz9]

From: KernelKraze <admin@mail.free-proletariat.dpdns.org>

Hi Johannes,

Thanks for the review.

On 7/30/25 6:35 AM, Johannes Thumshirn wrote:
> Where does this number come from?

It's from log_delayed_insertion_items() at line 6111:

/* 195 (4095 bytes of keys and sizes) fits in a single 4K page. */
const int max_batch_size = 195;

I reused this limit for consistency across btrfs batch operations.

> Wouldn't kcalloc() or kmalloc_array() be the better choice here?
> kcalloc() calls kmalloc_array() which in term does overflow checking.

Good point. The issue is we're allocating a mixed buffer:

[u32 sizes array][struct btrfs_key keys array]

kmalloc_array() handles single-type arrays, but we need:
- ins_sizes = (u32 *)ins_data
- ins_keys = (struct btrfs_key *)(ins_data + sizes_size)

Two options:
1. Keep current approach with manual overflow checks
2. Split into separate kmalloc_array() calls (potential cache miss cost)

Which would you prefer? I'm happy to rework it either way.

Thanks,
KernelKraze

Re: [PATCH 1/1] btrfs: add integer overflow protection to flush_dir_items_batch allocation

[Qu Wenruo]

在 2025/7/30 14:13, kmpfqgdwxucqz9@gmail.com 写道:
> From: KernelKraze <admin@mail.free-proletariat.dpdns.org>
> 
> The flush_dir_items_batch() function performs memory allocation using
> count * sizeof(u32) + count * sizeof(struct btrfs_key) without proper
> integer overflow checking. When count is large enough, this multiplication
> can overflow, resulting in an allocation smaller than expected, leading to
> buffer overflows during subsequent array access.
> 
> In extreme cases with very large directory item counts, this could
> theoretically lead to undersized memory allocation, though such scenarios
> are unlikely in normal filesystem usage.
> 
> Fix this by:
> 1. Adding a reasonable upper limit (195) to the batch size, consistent
>     with the limit used in log_delayed_insertion_items()
> 2. Using check_mul_overflow() and check_add_overflow() to detect integer
>     overflows before performing the allocation
> 3. Returning -EOVERFLOW when overflow is detected
> 4. Adding appropriate warning and error messages for debugging
> 
> This ensures that memory allocations are always sized correctly and
> prevents potential issues from integer overflow conditions, improving
> overall code robustness.
> 
> Signed-off-by: KernelKraze <admin@mail.free-proletariat.dpdns.org>
> ---
>   fs/btrfs/tree-log.c | 27 ++++++++++++++++++++++++---
>   1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 9f05d454b9df..19b443314db0 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -3655,14 +3655,35 @@ static int flush_dir_items_batch(struct btrfs_trans_handle *trans,
>   	} else {
>   		struct btrfs_key *ins_keys;
>   		u32 *ins_sizes;
> +		size_t keys_size, sizes_size, total_size;
>   
> -		ins_data = kmalloc(count * sizeof(u32) +
> -				   count * sizeof(struct btrfs_key), GFP_NOFS);
> +		/*
> +		 * Prevent integer overflow when calculating allocation size.
> +		 * We use the same reasonable limit as log_delayed_insertion_items()
> +		 * to prevent excessive memory allocation and potential DoS.
> +		 */
> +		if (count > 195) {
> +			btrfs_warn(inode->root->fs_info,
> +				   "dir items batch size %d exceeds safe limit, truncating",
> +				   count);
> +			count = 195;
> +		}

Just as Johannes said, explain this number.
> +
> +		/* Check for overflow in size calculations */
> +		if (check_mul_overflow(count, sizeof(u32), &sizes_size) ||
> +		    check_mul_overflow(count, sizeof(struct btrfs_key), &keys_size) ||
> +		    check_add_overflow(sizes_size, keys_size, &total_size)) {

And the magic number 195 won't cause any overflow anyway.

195 * (4 + 17) = 4095

Do you really think kmallocating a memory of 4096 bytes will cause extra 
problems?

Then you must have not reviewed any btrfs code.
We're going to use kmallocated memory for btrfs super block, which is 
exactly 4096 bytes, matching your "integer overflow" standard.

Your "integer overflow" looks completely impractical.


Further more, this 4095 magic number doesn't make sense on systems with 
larger page size.
If the system has a page size of 64K, limiting the kmalloc size to 4k 
make no sense.


Finally, have you checked the only caller of flush_dir_items_batch()?

The only caller is iterating all the item keys inside a leaf.

And a leaf has a very limited number of space for storing 
BTRFS_DIR_INDEX_KEY and its items.

At most there are nodesize / (dir_item_size + key_size + item_size) 
items inside a leaf. (this is already ignoring some extra overhead)

And using the default 16K nodesize, we can have over 200 dir items.

This means your flawed calculation is already going to cause false 
alerts on completely valid cases.


So a huge NO-ACK.

> +			btrfs_err(inode->root->fs_info,
> +				  "integer overflow in batch allocation size calculation");
> +			return -EOVERFLOW;
> +		}
> +
> +		ins_data = kmalloc(total_size, GFP_NOFS);
>   		if (!ins_data)
>   			return -ENOMEM;
>   
>   		ins_sizes = (u32 *)ins_data;
> -		ins_keys = (struct btrfs_key *)(ins_data + count * sizeof(u32));
> +		ins_keys = (struct btrfs_key *)(ins_data + sizes_size);
>   		batch.keys = ins_keys;
>   		batch.data_sizes = ins_sizes;
>   		batch.total_data_size = 0;

Re: [PATCH 1/1] btrfs: add integer overflow protection to flush_dir_items_batch allocation - WITHDRAWN

[kmpfqgdwxucqz9]

From: KernelKraze <admin@mail.free-proletariat.dpdns.org>

Hi Qu,

Thank you for the detailed technical review and correction. You are absolutely right, and I'm withdrawing this patch.

On 7/30/25 4:36 PM, Qu Wenruo wrote:
> Just as Johannes said, explain this number.
> And the magic number 195 won't cause any overflow anyway.
> [...]
> Finally, have you checked the only caller of flush_dir_items_batch()?

You've identified several fundamental issues with my patch:

1. **Misunderstanding of the caller context**: I failed to properly analyze that process_dir_items_leaf() iterates through leaf node items, where batch_size naturally corresponds to the number of directory entries in a leaf.

2. **Inappropriate limit**: The 195 limit would indeed cause false alerts on valid large directories, especially with 16K nodesize supporting 200+ directory items as you correctly calculated.

3. **Unnecessary complexity**: The integer overflow protection is not needed in this context where the count is bounded by filesystem structure limits, not user input.

4. **Architecture insensitivity**: I didn't consider systems with different page sizes where the 4K limit makes no sense.

I clearly should have done more thorough analysis of the btrfs architecture and actual usage patterns before proposing this change. Your feedback is invaluable for understanding how directory logging actually works in btrfs.

**This patch is hereby WITHDRAWN.**

Thank you for taking the time to provide such detailed technical education. I'll study the btrfs codebase more thoroughly before future contributions.

Best regards,
KernelKraze

Re: [PATCH 1/1] btrfs: add integer overflow protection to flush_dir_items_batch allocation

[Filipe Manana]

On Wed, Jul 30, 2025 at 5:48 AM <kmpfqgdwxucqz9@gmail.com> wrote:
>
> From: KernelKraze <admin@mail.free-proletariat.dpdns.org>
>
> The flush_dir_items_batch() function performs memory allocation using
> count * sizeof(u32) + count * sizeof(struct btrfs_key) without proper
> integer overflow checking. When count is large enough, this multiplication
> can overflow, resulting in an allocation smaller than expected, leading to
> buffer overflows during subsequent array access.
>
> In extreme cases with very large directory item counts, this could
> theoretically lead to undersized memory allocation, though such scenarios
> are unlikely in normal filesystem usage.
>
> Fix this by:
> 1. Adding a reasonable upper limit (195) to the batch size, consistent
>    with the limit used in log_delayed_insertion_items()
> 2. Using check_mul_overflow() and check_add_overflow() to detect integer
>    overflows before performing the allocation
> 3. Returning -EOVERFLOW when overflow is detected
> 4. Adding appropriate warning and error messages for debugging
>
> This ensures that memory allocations are always sized correctly and
> prevents potential issues from integer overflow conditions, improving
> overall code robustness.
>
> Signed-off-by: KernelKraze <admin@mail.free-proletariat.dpdns.org>
> ---
>  fs/btrfs/tree-log.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 9f05d454b9df..19b443314db0 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -3655,14 +3655,35 @@ static int flush_dir_items_batch(struct btrfs_trans_handle *trans,
>         } else {
>                 struct btrfs_key *ins_keys;
>                 u32 *ins_sizes;
> +               size_t keys_size, sizes_size, total_size;
>
> -               ins_data = kmalloc(count * sizeof(u32) +
> -                                  count * sizeof(struct btrfs_key), GFP_NOFS);
> +               /*
> +                * Prevent integer overflow when calculating allocation size.
> +                * We use the same reasonable limit as log_delayed_insertion_items()
> +                * to prevent excessive memory allocation and potential DoS.
> +                */
> +               if (count > 195) {
> +                       btrfs_warn(inode->root->fs_info,
> +                                  "dir items batch size %d exceeds safe limit, truncating",
> +                                  count);
> +                       count = 195;
> +               }

Adding to what was already mentioned by others....
This is so wrong that I'm not even sure where to begin.

But here truncating to 195 (or whatever value) is incredibly wrong
from a correctness point of view...
This means you are discarding beyond that limit, making us not log
index items that should be logged.

The 195 you saw in the other place is fine, because we split things in
batches up to that size and insert everything, but here we would just
skip anything beyond the limit.

Anyway there's no way we can have an overflow here in the first place....


> +
> +               /* Check for overflow in size calculations */
> +               if (check_mul_overflow(count, sizeof(u32), &sizes_size) ||
> +                   check_mul_overflow(count, sizeof(struct btrfs_key), &keys_size) ||
> +                   check_add_overflow(sizes_size, keys_size, &total_size)) {
> +                       btrfs_err(inode->root->fs_info,
> +                                 "integer overflow in batch allocation size calculation");
> +                       return -EOVERFLOW;
> +               }
> +
> +               ins_data = kmalloc(total_size, GFP_NOFS);
>                 if (!ins_data)
>                         return -ENOMEM;
>
>                 ins_sizes = (u32 *)ins_data;
> -               ins_keys = (struct btrfs_key *)(ins_data + count * sizeof(u32));
> +               ins_keys = (struct btrfs_key *)(ins_data + sizes_size);
>                 batch.keys = ins_keys;
>                 batch.data_sizes = ins_sizes;
>                 batch.total_data_size = 0;
> --
> 2.48.1
>
>