linux-kernel - Re: [PATCH 1/1] btrfs: add integer overflow protection to flush_dir_items

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAL3q7H5=cLEO3Cvty38bT1j5E6R0jJMuOWn+u9CkV6s58UhErQ@mail.gmail.com>
Date: Wed, 30 Jul 2025 11:20:31 +0100
From: Filipe Manana <fdmanana@...nel.org>
To: kmpfqgdwxucqz9@...il.com
Cc: David Sterba <dsterba@...e.com>, linux-btrfs@...r.kernel.org, 
	linux-kernel@...r.kernel.org, 
	KernelKraze <admin@...l.free-proletariat.dpdns.org>
Subject: Re: [PATCH 1/1] btrfs: add integer overflow protection to
 flush_dir_items_batch allocation

On Wed, Jul 30, 2025 at 5:48 AM <kmpfqgdwxucqz9@...il.com> wrote:
>
> From: KernelKraze <admin@...l.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@...l.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
>
>