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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] md: incomplete MD_SB_DISKS validation in super_90_validate()
- [Medium] md: missing mddev->max_disks update in md_set_array_info()
  and MD_SB_DISKS check in update_raid_disks()
- [Medium] md: signed overflow of mddev->raid_disks in
  raid_disks_store() bypasses metadata_store() check
--

commit 5e97b94d742c20742fe700227c811d79a4c7ba7c
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 load,
validation, sync, sysfs configuration, and 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..71f536d6257cd 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
[ ... ]
> @@ -1587,6 +1588,8 @@ static int super_90_validate(struct mddev *mddev, struct md_rdev *freshest, stru
>  			return 0;
>  	}
>  
> +	if (rdev->desc_nr < 0 || rdev->desc_nr >= MD_SB_DISKS)
> +		return -EINVAL;
>  	desc = sb->disks + rdev->desc_nr;
>  
>  	if (desc->state & (1<<MD_DISK_FAULTY))

[Severity: High]
Does super_90_validate() still leave v0.90 superblock fields unvalidated
against MD_SB_DISKS?

First, the new rdev->desc_nr check is placed after the mddev->pers == NULL
branch, which already indexes sb->disks[rdev->desc_nr].state.  Second,
desc->raid_disk is copied into rdev->raid_disk and rdev->saved_raid_disk
without checking that desc->raid_disk < MD_SB_DISKS:

drivers/md/md.c:super_90_validate() {
    ...
	} else if (desc->state & (1<<MD_DISK_SYNC)) {
		set_bit(In_sync, &rdev->flags);
		rdev->raid_disk = desc->raid_disk;
		rdev->saved_raid_disk = desc->raid_disk;
    ...
}

When md_add_new_disk() -> add_bound_rdev() runs on an active array,
add_bound_rdev() ignores validate_super()'s return value and passes rdev
directly to mddev->pers->hot_add_disk() without going through analyze_sbs(),
allowing an out-of-range desc->raid_disk to index conf->mirrors[] out of
bounds in raid1_add_disk() or raid10_add_disk().

In addition, for minor_version >= 91, super_90_validate() copies
sb->delta_disks into mddev->delta_disks without bounds checking; a negative
sb->delta_disks (or desc->raid_disk >= 0x80000000 wrapping to a negative
int in rdev->raid_disk) bypasses the rdev->raid_disk >= (mddev->raid_disks -
min(0, mddev->delta_disks)) check in analyze_sbs().

[ ... ]

> @@ -4442,6 +4473,13 @@ raid_disks_store(struct mddev *mddev, const char *buf, size_t len)
>  	if (err)
>  		return err;
>  	noio_flags = memalloc_noio_save();
> +	if ((mddev->max_disks && n > mddev->max_disks) ||
> +	    (mddev->persistent && mddev->major_version == 0 &&
> +	     n > MD_SB_DISKS)) {
> +		err = -EINVAL;
> +		goto out_unlock;
> +	}
> +
>  	if (mddev->pers) {
>  		if (n != mddev->raid_disks)
>  			err = update_raid_disks(mddev, n);
[ ... ]
> @@ -5056,10 +5096,14 @@ metadata_store(struct mddev *mddev, const char *buf, size_t len)
>  	err = -ENOENT;
>  	if (major >= ARRAY_SIZE(super_types) || super_types[major].name == NULL)
>  		goto out_unlock;
> +	err = -EINVAL;
> +	if (major == 0 && mddev->raid_disks > MD_SB_DISKS)
> +		goto out_unlock;
>  	mddev->major_version = major;
>  	mddev->minor_version = minor;
>  	mddev->persistent = 1;
>  	mddev->external = 0;
> +	mddev->max_disks = (major == 0) ? MD_SB_DISKS : 0;
>  	err = 0;
>  out_unlock:
>  	mddev_unlock(mddev);

[Severity: Medium]
Can an unsigned value >= 2147483648 written to raid_disks_store() overflow
mddev->raid_disks and bypass the check in metadata_store()?

raid_disks_store() parses buf into unsigned int n with kstrtouint().  When
raid_disks is written while metadata_version is still "none", both
mddev->max_disks and mddev->persistent are 0, so n > INT_MAX is stored into
the signed int mddev->raid_disks as a negative number.  Writing "0.90" to
metadata_store() then passes the signed comparison mddev->raid_disks >
MD_SB_DISKS, leaving the v0.90 array configured with a negative
mddev->raid_disks.

> @@ -7951,6 +7995,14 @@ 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->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]
Does the info->raid_disks == 0 branch in md_set_array_info() leave
mddev->max_disks unset?

When SET_ARRAY_INFO is called with info->raid_disks == 0,
md_set_array_info() sets mddev->major_version and mddev->persistent and
returns early without updating mddev->max_disks.  If userspace then sets
raid_disks via sysfs and starts the array, md_run() skips analyze_sbs()
because mddev->raid_disks is already non-zero, so mddev->max_disks remains 0
on a persistent v0.90 array.  As a result, bind_rdev_to_array() and
update_array_info() -> update_raid_disks() (which only checks
mddev->max_disks) do not enforce MD_SB_DISKS.

Conversely, if metadata_store() set mddev->max_disks = MD_SB_DISKS and
md_set_array_info() is later called with info->not_persistent == 1 or with
info->raid_disks == 0 and info->major_version != 0, mddev->max_disks is
never cleared to 0.

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