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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] md: signed overflow and missing negative checks in v0.90
  metadata_store()
--

commit 6c7fdd3676be5c17e28344695572829adecbf502
Author: Chandradhar Kumar <chandradhar.2003@gmail.com>

md: enforce MD_SB_DISKS bounds across v0.90 superblock paths

This commit enforces MD_SB_DISKS bounds across v0.90 superblock load,
validation, configuration, and sync paths to prevent out-of-bounds
accesses on sb->disks[].

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 03e35386a8028..14e8f93471760 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c

[ ... ]

> @@ -5056,10 +5113,16 @@ 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 ||
> +	     mddev->raid_disks - mddev->delta_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);
> @@ -7941,16 +8004,32 @@ int md_set_array_info(struct mddev *mddev, struct mdu_array_info_s *info)
>  				info->major_version);
>  			return -EINVAL;
>  		}
> +		if (!info->not_persistent && info->major_version == 0 &&
> +		    (mddev->raid_disks < 0 ||
> +		     mddev->raid_disks > MD_SB_DISKS ||
> +		     mddev->raid_disks - mddev->delta_disks > MD_SB_DISKS))
> +			return -EINVAL;

[Severity: Medium]
Can mddev->raid_disks - mddev->delta_disks overflow 32-bit signed int or
accept negative disk counts in metadata_store() and md_set_array_info()?

When md_run() -> analyze_sbs() -> super_1_validate() processes a crafted
v1.x superblock before get_pers() fails and the rdev is removed,
mddev->raid_disks and mddev->delta_disks retain arbitrary 32-bit signed
values while mddev->disks is empty:

drivers/md/md.c:super_1_validate() {
	if (mddev->raid_disks == 0) {
		...
		mddev->raid_disks = le32_to_cpu(sb->raid_disks);
		...
		if ((le32_to_cpu(sb->feature_map) & MD_FEATURE_RESHAPE_ACTIVE)) {
			mddev->reshape_position = le64_to_cpu(sb->reshape_position);
			mddev->delta_disks = le32_to_cpu(sb->delta_disks);
}

Unlike super_90_load(), which casts to s64, evaluating mddev->raid_disks -
mddev->delta_disks in 32-bit signed int overflows when mddev->raid_disks is
in [0, MD_SB_DISKS] and mddev->delta_disks is a large negative value such
as INT_MIN, wrapping to a negative number that bypasses the > MD_SB_DISKS
check.  In addition, metadata_store() does not check mddev->raid_disks < 0,
and neither function rejects negative pre-reshape disk counts, allowing
negative mddev->raid_disks or invalid olddisks values to persist on a v0.90
array.

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