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 <[email protected]> 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/[email protected]?part=1
