* Re: [PATCH] md: enforce MD_SB_DISKS bounds across v0.90 superblock paths
2026-11-13 15:23 [PATCH] md: enforce MD_SB_DISKS bounds across v0.90 superblock paths Chandradhar Kumar
@ 2026-11-13 15:44 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-11-13 15:44 UTC (permalink / raw)
To: Chandradhar Kumar, linux-raid; +Cc: yukuai
Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] md: super_90_sync() zeroes this_disk and leaves desc_nr at -1
on skip
- [Medium] md: md_set_array_info() MD_SB_DISKS check bypassed when
major_version != 0
- [Medium] md: incomplete MD_SB_DISKS checks in sysfs and reshape
configuration paths
- [Medium] md: early return in super_90_sync() leaves sb_size and
sb_page uninitialized
- [Low] md: unsigned < 0 check in super_90_load() and code style issues
in super_90_sync()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] md: out-of-bounds write in super_90_sync() and
super_1_sync() [1]
--
commit 37ce5ab8e4435993a82dee25db9358c85f6fa1d3
Author: Chandradhar Kumar <chandradhar.2003@gmail.com>
md: enforce MD_SB_DISKS bounds across v0.90 superblock paths
This commit adds MD_SB_DISKS bounds checks across v0.90 superblock loading,
synchronization, sysfs configuration, and SET_ARRAY_INFO ioctl paths to
prevent out-of-bounds accesses on sb->disks[].
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 03e35386a8028..a72f98cdad549 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -1407,7 +1407,9 @@ static int super_90_load(struct md_rdev *rdev, struct md_rdev *refdev, int minor
> goto abort;
> }
>
> - if (sb->raid_disks <= 0 || sb->raid_disks > MD_SB_DISKS)
> + if (sb->raid_disks <= 0 || sb->raid_disks > MD_SB_DISKS ||
> + sb->this_disk.number < 0 ||
> + sb->this_disk.number >= MD_SB_DISKS)
> goto abort;
>
> if (md_csum_fold(calc_sb_csum(sb)) != md_csum_fold(sb->sb_csum)) {
[Severity: Low]
In mdp_disk_t, number is __u32, so sb->this_disk.number < 0 is always false
and triggers a -Wtype-limits warning under W=1 builds.
Also, in super_90_sync() below, the new WARN_ON_ONCE() statement is placed
before the local variable declarations (int i; int active=0, ...), which
triggers -Wdeclaration-after-statement, and the new pr_warn() format string
for out-of-range desc_nr is missing a trailing newline.
> @@ -1621,6 +1623,10 @@ static void super_90_sync(struct mddev *mddev, struct md_rdev *rdev)
> struct md_rdev *rdev2;
> int next_spare = mddev->raid_disks;
>
> + if (WARN_ON_ONCE(mddev->raid_disks <= 0 ||
> + mddev->raid_disks > MD_SB_DISKS))
> + return;
> +
> /* make rdev->sb match mddev data..
> *
> * 1/ zero out disks
[Severity: Medium]
Does returning early from super_90_sync() leave rdev->sb_size and
rdev->sb_page uninitialized while sync_sbs() still marks the superblock as
loaded?
sync_sbs() unconditionally sets rdev->sb_loaded = 1 after calling
sync_super(mddev, rdev):
drivers/md/md.c:sync_sbs() {
...
} else {
sync_super(mddev, rdev);
rdev->sb_loaded = 1;
}
...
}
When md_update_sb() subsequently calls md_write_metadata() for every rdev
with sb_loaded == 1, it either submits a write bio with a zero-length
segment (if rdev->sb_size is still 0) or writes uninitialized or stale
sb_page contents to disk.
> @@ -1693,6 +1699,8 @@ static void super_90_sync(struct mddev *mddev, struct md_rdev *rdev)
> int desc_nr;
> int is_active = test_bit(In_sync, &rdev2->flags);
>
> + rdev2->desc_nr = -1;
> +
> if (rdev2->raid_disk >= 0 &&
> sb->minor_version >= 91)
> /* we have nowhere to store the recovery_offset,
> @@ -1707,6 +1715,12 @@ static void super_90_sync(struct mddev *mddev, struct md_rdev *rdev)
> desc_nr = rdev2->raid_disk;
> else
> desc_nr = next_spare++;
> +
> + if (desc_nr < 0 || desc_nr >= MD_SB_DISKS) {
> + pr_warn("md: %s: desc_nr %d out of range for rdev %pg, skipping",
> + mdname(mddev), desc_nr, rdev2->bdev);
> + continue;
> + }
> rdev2->desc_nr = desc_nr;
> d = &sb->disks[rdev2->desc_nr];
> nr_disks++;
> @@ -1736,7 +1750,7 @@ static void super_90_sync(struct mddev *mddev, struct md_rdev *rdev)
> d->state |= (1<<MD_DISK_FAILFAST);
> }
> /* now set the "removed" and "faulty" bits on any missing devices */
> - for (i=0 ; i < mddev->raid_disks ; i++) {
> + for (i = 0 ; i < mddev->raid_disks && i < MD_SB_DISKS; i++) {
> mdp_disk_t *d = &sb->disks[i];
> if (d->state == 0 && d->number == 0) {
> d->number = i;
> @@ -1752,7 +1766,10 @@ static void super_90_sync(struct mddev *mddev, struct md_rdev *rdev)
> sb->failed_disks = failed;
> sb->spare_disks = spare;
>
> - sb->this_disk = sb->disks[rdev->desc_nr];
> + if (rdev->desc_nr >= 0 && rdev->desc_nr < MD_SB_DISKS)
> + sb->this_disk = sb->disks[rdev->desc_nr];
> + else
> + memset(&sb->this_disk, 0, sizeof(sb->this_disk));
> sb->sb_csum = calc_sb_csum(sb);
> }
[Severity: High]
Can zeroing sb->this_disk when rdev->desc_nr is -1 corrupt on-disk RAID
metadata and cause a skipped spare or faulty disk to impersonate disk 0?
super_90_sync() initializes next_spare = mddev->raid_disks and increments it
for each non-active (faulty or spare) rdev, so even an array with at most
MD_SB_DISKS (27) total bound devices reaches desc_nr >= MD_SB_DISKS whenever
raid_disks plus the number of faulty/spare devices exceeds 27 (for example, a
27-disk array with one failed slot). When that rdev is skipped, its
rdev->desc_nr is left at -1, and super_90_sync() zeroes sb->this_disk
(setting sb->this_disk.number = 0) with a valid checksum before md_update_sb()
writes that superblock to the skipped device.
On the next array assembly, super_90_load() sets rdev->desc_nr =
sb->this_disk.number (0) and super_90_validate() indexes sb->disks[0],
treating the skipped faulty or spare device as active in-sync slot 0. In
addition, leaving rdev->desc_nr == -1 on a bound rdev in memory causes
super_90_validate() to read sb->disks[-1] out of bounds if called on that
rdev.
> @@ -3374,6 +3391,10 @@ slot_store(struct md_rdev *rdev, const char *buf, size_t len)
> if (slot >= rdev->mddev->raid_disks &&
> slot >= rdev->mddev->raid_disks + rdev->mddev->delta_disks)
> return -ENOSPC;
> + if (rdev->mddev->persistent &&
> + rdev->mddev->major_version == 0 &&
> + slot >= MD_SB_DISKS)
> + return -ENOSPC;
> rdev->raid_disk = slot;
> /* assume it is working */
> clear_bit(Faulty, &rdev->flags);
> @@ -4462,8 +4483,14 @@ raid_disks_store(struct mddev *mddev, const char *buf, size_t len)
> mddev->delta_disks = n - olddisks;
> mddev->raid_disks = n;
> mddev->reshape_backwards = (mddev->delta_disks < 0);
> - } else
> - mddev->raid_disks = n;
> + } else {
> + if ((mddev->persistent && mddev->major_version == 0 &&
> + n > MD_SB_DISKS) ||
> + (mddev->max_disks && n > mddev->max_disks))
> + err = -EINVAL;
> + else
> + mddev->raid_disks = n;
> + }
> out_unlock:
> memalloc_noio_restore(noio_flags);
> mddev_unlock_and_resume(mddev);
[Severity: Medium]
Can userspace still configure a persistent v0.90 array with raid_disks >
MD_SB_DISKS or slot >= MD_SB_DISKS through other sysfs paths?
First, in raid_disks_store(), the new bounds check is only in the final else
branch; if mddev->reshape_position != MaxSector on an inactive array, the
else-if branch still assigns mddev->raid_disks = n without checking
MD_SB_DISKS or mddev->max_disks. Second, if userspace writes raid_disks (or
slot) before writing "0.90" to metadata_version, mddev->persistent is still 0
during raid_disks_store(), and metadata_store() then enables persistent v0.90
metadata without validating mddev->raid_disks <= MD_SB_DISKS or setting
mddev->max_disks = MD_SB_DISKS. Because mddev->max_disks remains 0 for a
sysfs-configured v0.90 array, resizing an active array via
update_raid_disks() (and hot-adding a slot in the rdev->mddev->pers != NULL
branch of slot_store()) also misses the MD_SB_DISKS limit.
[Severity: Critical]
This problem wasn't introduced by this patch, but raid_disks_store() and
metadata_store() still allow an unvalidated raid_disks value [1] to
cause an out-of-bounds write in super_1_sync().
When an inactive array has mddev->max_disks == 0 (such as a newly created
array using v1.x metadata, or before metadata_store() sets the metadata
version), raid_disks_store() accepts an arbitrarily large n into
mddev->raid_disks. When a device is bound, bind_rdev_to_array() assigns
rdev->desc_nr >= mddev->raid_disks, and super_1_sync() then uses
rdev->desc_nr + 1 as max_dev to write past the end of sb->dev_roles[] and
the allocated superblock page:
drivers/md/md.c:super_1_sync() {
...
max_dev = 0;
rdev_for_each(rdev2, mddev)
if (rdev2->desc_nr+1 > max_dev)
max_dev = rdev2->desc_nr+1;
if (max_dev > le32_to_cpu(sb->max_dev))
sb->max_dev = cpu_to_le32(max_dev);
for (i=0; i<max_dev;i++)
sb->dev_roles[i] = cpu_to_le16(MD_DISK_ROLE_SPARE);
...
}
> @@ -7951,6 +7978,15 @@ int md_set_array_info(struct mddev *mddev, struct mdu_array_info_s *info)
> mddev->ctime = ktime_get_real_seconds();
> return 0;
> }
> +
> + if (info->raid_disks < 0 ||
> + (!info->not_persistent && info->major_version == 0 &&
> + info->raid_disks > MD_SB_DISKS)) {
> + pr_warn("md: %s: invalid raid_disks %d for v0.90 superblock\n",
> + mdname(mddev), info->raid_disks);
> + return -EINVAL;
> + }
> +
> mddev->major_version = MD_MAJOR_VERSION;
> mddev->minor_version = MD_MINOR_VERSION;
> mddev->patch_version = MD_PATCHLEVEL_VERSION;
[Severity: Medium]
Can a caller bypass this MD_SB_DISKS check by passing a non-zero
info->major_version when info->raid_disks != 0?
Immediately after this check, md_set_array_info() unconditionally sets
mddev->major_version = MD_MAJOR_VERSION (0) and mddev->minor_version =
MD_MINOR_VERSION (90), ignoring info->major_version. Passing
info->major_version != 0 with info->not_persistent == 0 and info->raid_disks
> MD_SB_DISKS therefore skips the check and still configures a persistent
v0.90 array with mddev->raid_disks > MD_SB_DISKS.
[1] https://sashiko.dev/#/bug/linux-9f7b36a9-e962-4efb-bbdc-83b85f2d807a
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261113152311.13949-1-chandradhar.2003@gmail.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread