Re: [PATCH] md: validate raid_disks range before loading personality

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] md: md_run() cap at MAX_RAID_DEVICES breaks valid arrays with >
  253 disks
- [Medium] md: unchecked delta_disks in md_run() allows OOM warn and
  divide-by-zero
- [Medium] md: missing raid_disks upper bound in update_raid_disks() on
  online reshape
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] md/raid1: missing raid_disks upper bounds check in
  raid1_reshape() [1]
--

commit fc41ec678e6b01e865c547317a0225da7de795ee
Author: Ran Hongyun <ranhongyun1@huawei.com>

md: validate raid_disks range before loading personality

This commit moves MAX_RAID_DEVICES (253) from dm-raid.c to md.h and rejects
mddev->raid_disks values outside [1, MAX_RAID_DEVICES] in md_run() before
invoking the personality run callback.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 03e35386a8028..7b202b483e98c 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -6703,6 +6703,13 @@ int md_run(struct mddev *mddev)
>  			return -EINVAL;
>  	}
>  
> +	if (mddev->raid_disks <= 0 ||
> +	    mddev->raid_disks > MAX_RAID_DEVICES) {
> +		pr_warn("md: %s: raid_disks %d out of range [1, %d]\n",
> +			mdname(mddev), mddev->raid_disks, MAX_RAID_DEVICES);
> +		return -EINVAL;
> +	}
> +
>  	if (mddev->level != LEVEL_NONE)
>  		request_module("md-level-%d", mddev->level);
>  	else if (mddev->clevel[0])

[Severity: High]
Does capping mddev->raid_disks at MAX_RAID_DEVICES (253) in md_run() break
assembling valid native MD arrays with more than 253 member devices?

MAX_RAID_DEVICES was a dm-raid target limit, whereas native MD v1.x
superblocks support up to 384 devices in a 1 KiB superblock and up to 1920
devices in a 4 KiB superblock:

drivers/md/md.c:super_1_validate() {
	...
	mddev->max_disks =  (4096-256)/2;
	...
}

In addition, raid6_takeover() converts a 253-disk RAID5 array into a
254-disk RAID6 array by incrementing mddev->raid_disks to 254, which
md_run() will now refuse to assemble with -EINVAL.

[Severity: Medium]
When an array is started with an active reshape (mddev->reshape_position !=
MaxSector), can an unvalidated mddev->delta_disks bypass this check and
still trigger the oversized allocations or a divide-by-zero?

On an inactive array, writing a large value to sysfs raid_disks, setting
reshape_position, and then writing a small valid value to raid_disks (or
loading a superblock with a crafted delta_disks) leaves mddev->raid_disks
within [1, MAX_RAID_DEVICES] while mddev->delta_disks is arbitrarily
negative.  raid5 setup_conf() and raid10 setup_conf() size their disk
allocations from mddev->raid_disks - mddev->delta_disks:

drivers/md/raid10.c:setup_conf() {
	...
	conf->mirrors = kzalloc_objs(struct raid10_info,
				     mddev->raid_disks + max(0, -mddev->delta_disks));
	...
}

This still hits the MAX_PAGE_ORDER warning (and overflows a signed int if
mddev->delta_disks == INT_MIN).  Furthermore, if mddev->delta_disks equals
mddev->raid_disks, conf->prev.raid_disks (mddev->raid_disks -
mddev->delta_disks) becomes 0 and raid10_run() -> enough() -> _enough()
divides by zero at (this + 1) % disks.

Checking that mddev->raid_disks - mddev->delta_disks is also within the
valid range when mddev->reshape_position != MaxSector would close this gap.

[Severity: Medium]
Does online reshape via update_raid_disks() bypass md_run() and leave
raid1, raid5, and raid10 exposed to the same unbounded allocations?

When reshaping an already-running array via sysfs raid_disks or
ioctl(SET_ARRAY_INFO), raid_disks_store() and update_array_info() call
update_raid_disks() directly without going through md_run():

drivers/md/md.c:update_raid_disks() {
	...
	if (raid_disks <= 0 ||
	    (mddev->max_disks && raid_disks >= mddev->max_disks))
		return -EINVAL;
	...
	mddev->delta_disks = raid_disks - mddev->raid_disks;
	...
	rv = mddev->pers->check_reshape(mddev);
	...
}

When mddev->max_disks is 0 (such as on non-persistent or external-metadata
arrays), any positive raid_disks up to INT_MAX is accepted and passed to
pers->check_reshape(), hitting the MAX_PAGE_ORDER and INT_MAX allocation
warnings in raid1_reshape(), raid10_check_reshape(), and raid5
check_reshape().

[Severity: Medium]
This problem wasn't introduced by this patch, but raid1_reshape() [1]
lacks an upper bounds check on raid_disks when reshaping a running array
with mddev->max_disks == 0:

drivers/md/raid1.c:raid1_reshape() {
	...
	raid_disks = mddev->raid_disks + mddev->delta_disks;
	...
	new_r1bio_size = offsetof(struct r1bio, bios[raid_disks * 2]);
	newpool = mempool_create_kmalloc_pool(NR_RAID_BIOS, new_r1bio_size);
	if (!newpool) {
		return -ENOMEM;
	}
	newmirrors = kzalloc(array3_size(sizeof(struct raid1_info),
					 raid_disks, 2),
			     GFP_KERNEL);
	...
}

Because update_raid_disks() does not bound raid_disks when mddev->max_disks
is 0, a large raid_disks value overflows raid_disks * 2 in signed 32-bit
arithmetic or exceeds KMALLOC_MAX_SIZE in mempool_create_kmalloc_pool() and
kzalloc(), triggering a slab warning in ___kmalloc_large_node().

[1] https://sashiko.dev/#/bug/linux-9d642fe4-31fe-4998-ac3d-c07a30507ae7

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109025621.2031722-1-ranhongyun1@huawei.com?part=1