Re: [PATCH] md/raid1: Prevent excessively large memory allocations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] md/raid1: incorrect allocation size calculation in setup_conf()
--

commit c8cc718382c763b1d6426dcf4d4968c6557c8e89
Author: Gowtham S Bhushan <gowtham398gs@gmail.com>

md/raid1: Prevent excessively large memory allocations

This commit adds a bounds check on mddev->raid_disks in setup_conf() to
prevent integer overflow and allocations exceeding KMALLOC_MAX_SIZE.

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 894efb50c6d58..8ed871792bf48 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -3020,6 +3020,16 @@ static struct r1conf *setup_conf(struct mddev *mddev)
>  	struct md_rdev *rdev;
>  	size_t r1bio_size;
>  	int err = -ENOMEM;
> +	size_t calculated_size;
> +
> +	/* prevent baseline integer wrap-around anomalies first*/
> +	/* Safely calculate size; overflows automatically saturate to SIZE_MAX */
> +
> +	calculated_size = size_add(sizeof(struct r1conf),
> +				   size_mul(mddev->raid_disks, sizeof(struct r1bio)));
> +
> +	if (mddev->raid_disks <= 0 || calculated_size >= KMALLOC_MAX_SIZE)
> +		return ERR_PTR(-EINVAL);

[Severity: Medium]
Does calculated_size match the actual allocations in drivers/md/raid1.c that
scale with mddev->raid_disks?

struct r1conf is a fixed-size structure with no flexible array member, and
struct r1bio is not allocated as an array of mddev->raid_disks elements. The
per-disk allocations are:

drivers/md/raid1.c:setup_conf() {
	...
	conf->mirrors = kzalloc(array3_size(sizeof(struct raid1_info),
					    mddev->raid_disks, 2),
				GFP_KERNEL);
	...
	r1bio_size = offsetof(struct r1bio, bios[mddev->raid_disks * 2]);
	conf->r1bio_pool = mempool_create_kmalloc_pool(NR_RAID_BIOS, r1bio_size);
	...
}

and during resync initialization via raid1_sync_request() -> init_resync() ->
mempool_init() -> r1buf_pool_alloc():

drivers/md/raid1.c:r1buf_pool_alloc() {
	...
	rps = kmalloc_objs(struct resync_pages, conf->raid_disks * 2, gfp_flags);
	...
}

Because calculated_size only checks mddev->raid_disks * sizeof(struct r1bio),
several allocations can still exceed KMALLOC_MAX_SIZE and trigger the
WARN_ON_ONCE_GFP(size > KMALLOC_MAX_SIZE, flags) warning in mm/slub.c:

1. On 64-bit systems with 4KB pages, sizeof(struct r1bio) is 80 bytes while
   2 * sizeof(struct resync_pages) is 272 bytes per disk. A raid_disks value
   such as 20,000 passes setup_conf() (~1.6 MB) and then requests ~5.44 MB in
   r1buf_pool_alloc(), exceeding a 4 MB KMALLOC_MAX_SIZE.

2. On 32-bit systems, sizeof(struct r1bio) is 48 bytes while
   2 * sizeof(struct raid1_info) is 56 or 64 bytes per disk. A raid_disks
   value such as 70,000 or 80,000 passes calculated_size < KMALLOC_MAX_SIZE
   and then exceeds a 4 MB KMALLOC_MAX_SIZE when allocating conf->mirrors in
   setup_conf().

Also, since mddev->raid_disks is a signed int, calling
size_mul(mddev->raid_disks, ...) before checking mddev->raid_disks <= 0
implicitly converts negative values to a large size_t first.

Could mddev->raid_disks <= 0 be checked first, and the upper bound be
validated against the actual allocation sizes (or a reasonable maximum device
count)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028152755.383821-1-gowtham398gs@gmail.com?part=1