Re: [PATCH] md: enforce MD_SB_DISKS bounds across v0.90 superblock paths

[sashiko-bot@kernel.org]

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