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

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

[Chandradhar Kumar]

The legacy v0.90 superblock format (mdp_super_t) embeds a fixed-size
array of MD_SB_DISKS (27) disk descriptors (sb->disks[27]). Currently,
several configuration and metadata load paths fail to enforce this
upper bound, allowing mddev->raid_disks, rdev->raid_disk, or
rdev->desc_nr to exceed MD_SB_DISKS - 1 and trigger an out-of-bounds
array access in super_90_sync():

  UBSAN: array-index-out-of-bounds in drivers/md/md.c:1740:29
  index 124 is out of range for type 'mdp_disk_t [27]'

Fix this across all entry points and harden super_90_sync():

1. In super_90_load(), validate sb->this_disk.number, sb->delta_disks
   (for v0.91+), and active/sync raid_disk indices against MD_SB_DISKS.
   In super_90_validate(), check rdev->desc_nr at the start of the
   function before any sb->disks[] dereference and guard desc->raid_disk
   assignments.
2. In metadata_store() and md_set_array_info(), validate existing
   mddev->raid_disks against MD_SB_DISKS for persistent v0.90 arrays
   and maintain mddev->max_disks across both persistent and
   non-persistent transitions.
3. In raid_disks_store() and slot_store(), reject values overflowing
   INT_MAX or exceeding MD_SB_DISKS / mddev->max_disks across all
   configuration and reshape branches.
4. In super_90_sync(), bounds-check desc_nr and mddev->raid_disks
   before indexing sb->disks[], and synthesize a faulty/removed
   this_disk descriptor if rdev->desc_nr is out of range rather than
   indexing out-of-bounds or zeroing this_disk (which would alias
   slot 0).

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzbot+9e3014263a35700ab49b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9e3014263a35700ab49b
Signed-off-by: Chandradhar Kumar <chandradhar.2003@gmail.com>
---
Changes in v3:
- In super_90_load(), also validate sb->delta_disks (for minor >= 91)
  and active/sync disk raid_disk indices against MD_SB_DISKS.
- Move the rdev->desc_nr bounds check to the top of super_90_validate()
  before the mddev->pers == NULL branch indexes sb->disks[], and check
  desc->raid_disk < MD_SB_DISKS before assigning rdev->raid_disk.
- Reject n > INT_MAX in raid_disks_store() to prevent signed overflow of
  mddev->raid_disks prior to metadata_store().
- Update mddev->max_disks in the info->raid_disks == 0 branch and the
  non-persistent branch of md_set_array_info().

Changes in v2:
- Drop early return in super_90_sync() so rdev->sb_size and sb_page are
  initialized before sync_sbs() sets rdev->sb_loaded = 1.
- Do not reset rdev2->desc_nr = -1 or zero sb->this_disk on skip; instead
  populate sb->this_disk as faulty/removed with rdev->desc_nr.
- Validate mddev->raid_disks and set mddev->max_disks in metadata_store(),
  and move the raid_disks_store() check above the reshape/active branches.

 drivers/md/md.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 80 insertions(+), 3 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 03e35386a802..e733dcdd13a5 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -1407,7 +1407,18 @@ 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 >= MD_SB_DISKS)
+		goto abort;
+
+	if (sb->minor_version >= 91 &&
+	    ((s64)sb->raid_disks - sb->delta_disks <= 0 ||
+	     (s64)sb->raid_disks - sb->delta_disks > MD_SB_DISKS))
+		goto abort;
+
+	if ((sb->disks[sb->this_disk.number].state &
+	     ((1 << MD_DISK_SYNC) | (1 << MD_DISK_ACTIVE))) &&
+	    sb->disks[sb->this_disk.number].raid_disk >= MD_SB_DISKS)
 		goto abort;
 
 	if (md_csum_fold(calc_sb_csum(sb)) != md_csum_fold(sb->sb_csum)) {
@@ -1501,6 +1512,9 @@ static int super_90_validate(struct mddev *mddev, struct md_rdev *freshest, stru
 	clear_bit(Bitmap_sync, &rdev->flags);
 	clear_bit(WriteMostly, &rdev->flags);
 
+	if (rdev->desc_nr < 0 || rdev->desc_nr >= MD_SB_DISKS)
+		return -EINVAL;
+
 	if (mddev->raid_disks == 0) {
 		mddev->major_version = 0;
 		mddev->minor_version = sb->minor_version;
@@ -1592,6 +1606,8 @@ static int super_90_validate(struct mddev *mddev, struct md_rdev *freshest, stru
 	if (desc->state & (1<<MD_DISK_FAULTY))
 		set_bit(Faulty, &rdev->flags);
 	else if (desc->state & (1<<MD_DISK_SYNC)) {
+		if (desc->raid_disk >= MD_SB_DISKS)
+			return -EINVAL;
 		set_bit(In_sync, &rdev->flags);
 		rdev->raid_disk = desc->raid_disk;
 		rdev->saved_raid_disk = desc->raid_disk;
@@ -1601,6 +1617,8 @@ static int super_90_validate(struct mddev *mddev, struct md_rdev *freshest, stru
 		 * that is, so set to zero for now
 		 */
 		if (mddev->minor_version >= 91) {
+			if (desc->raid_disk >= MD_SB_DISKS)
+				return -EINVAL;
 			rdev->recovery_offset = 0;
 			rdev->raid_disk = desc->raid_disk;
 		}
@@ -1708,6 +1726,12 @@ static void super_90_sync(struct mddev *mddev, struct md_rdev *rdev)
 		else
 			desc_nr = next_spare++;
 		rdev2->desc_nr = desc_nr;
+
+		if (desc_nr < 0 || desc_nr >= MD_SB_DISKS) {
+			pr_warn("md: %s: desc_nr %d out of range for rdev %pg, skipping\n",
+				mdname(mddev), desc_nr, rdev2->bdev);
+			continue;
+		}
 		d = &sb->disks[rdev2->desc_nr];
 		nr_disks++;
 		d->number = rdev2->desc_nr;
@@ -1736,7 +1760,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 +1776,17 @@ 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 {
+		sb->this_disk.number = rdev->desc_nr;
+		sb->this_disk.major = MAJOR(rdev->bdev->bd_dev);
+		sb->this_disk.minor = MINOR(rdev->bdev->bd_dev);
+		sb->this_disk.raid_disk = rdev->desc_nr;
+		sb->this_disk.state = (1 << MD_DISK_FAULTY) |
+				      (1 << MD_DISK_REMOVED);
+	}
+
 	sb->sb_csum = calc_sb_csum(sb);
 }
 
@@ -3353,6 +3387,12 @@ 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->max_disks &&
+		     slot >= rdev->mddev->max_disks) ||
+		    (rdev->mddev->persistent &&
+		     rdev->mddev->major_version == 0 &&
+		     slot >= MD_SB_DISKS))
+			return -ENOSPC;
 
 		rdev->raid_disk = slot;
 		if (test_bit(In_sync, &rdev->flags))
@@ -3374,6 +3414,12 @@ 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->max_disks &&
+		     slot >= rdev->mddev->max_disks) ||
+		    (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);
@@ -4437,11 +4483,20 @@ raid_disks_store(struct mddev *mddev, const char *buf, size_t len)
 	err = kstrtouint(buf, 10, &n);
 	if (err < 0)
 		return err;
+	if (n > INT_MAX)
+		return -EINVAL;
 
 	err = mddev_suspend_and_lock(mddev);
 	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);
@@ -5029,6 +5084,7 @@ metadata_store(struct mddev *mddev, const char *buf, size_t len)
 		mddev->external = 0;
 		mddev->major_version = 0;
 		mddev->minor_version = 90;
+		mddev->max_disks = 0;
 		goto out_unlock;
 	}
 	if (strncmp(buf, "external:", 9) == 0) {
@@ -5043,6 +5099,7 @@ metadata_store(struct mddev *mddev, const char *buf, size_t len)
 		mddev->external = 1;
 		mddev->major_version = 0;
 		mddev->minor_version = 90;
+		mddev->max_disks = 0;
 		goto out_unlock;
 	}
 	major = simple_strtoul(buf, &e, 10);
@@ -5056,10 +5113,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);
@@ -7941,16 +8002,30 @@ 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))
+			return -EINVAL;
 		mddev->major_version = info->major_version;
 		mddev->minor_version = info->minor_version;
 		mddev->patch_version = info->patch_version;
 		mddev->persistent = !info->not_persistent;
+		mddev->max_disks = (mddev->persistent &&
+				    info->major_version == 0) ?
+				   MD_SB_DISKS : 0;
 		/* ensure mddev_put doesn't delete this now that there
 		 * is some minimal configuration.
 		 */
 		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;
@@ -7980,6 +8055,8 @@ int md_set_array_info(struct mddev *mddev, struct mdu_array_info_s *info)
 		mddev->max_disks = MD_SB_DISKS;
 		mddev->flags = 0;
 		mddev->sb_flags = 0;
+	} else {
+		mddev->max_disks = 0;
 	}
 	set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
 
-- 
2.56.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] md: unsigned promotion of sb->delta_disks in super_90_load()
  breaks shrinking reshapes
- [Low] md: unresolvable Fixes trailer commit SHA 1da177e4c3f4 in commit
  message
- [Low] md: unvalidated pre-reshape disk count in metadata_store() for
  v0.90
--

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

> md: enforce MD_SB_DISKS bounds across v0.90 superblock paths
> 
> The legacy v0.90 superblock format (mdp_super_t) embeds a fixed-size
> array of MD_SB_DISKS (27) disk descriptors (sb->disks[27]). Currently,
> several configuration and metadata load paths fail to enforce this
> upper bound, allowing mddev->raid_disks, rdev->raid_disk, or
> rdev->desc_nr to exceed MD_SB_DISKS - 1 and trigger an out-of-bounds
> array access in super_90_sync():
> 
>   UBSAN: array-index-out-of-bounds in drivers/md/md.c:1740:29
>   index 124 is out of range for type 'mdp_disk_t [27]'
> 
> Fix this across all entry points and harden super_90_sync():
> 
> 1. In super_90_load(), validate sb->this_disk.number, sb->delta_disks
>    (for v0.91+), and active/sync raid_disk indices against MD_SB_DISKS.
>    In super_90_validate(), check rdev->desc_nr at the start of the
>    function before any sb->disks[] dereference and guard desc->raid_disk
>    assignments.
> 2. In metadata_store() and md_set_array_info(), validate existing
>    mddev->raid_disks against MD_SB_DISKS for persistent v0.90 arrays
>    and maintain mddev->max_disks across both persistent and
>    non-persistent transitions.
> 3. In raid_disks_store() and slot_store(), reject values overflowing
>    INT_MAX or exceeding MD_SB_DISKS / mddev->max_disks across all
>    configuration and reshape branches.
> 4. In super_90_sync(), bounds-check desc_nr and mddev->raid_disks
>    before indexing sb->disks[], and synthesize a faulty/removed
>    this_disk descriptor if rdev->desc_nr is out of range rather than
>    indexing out-of-bounds or zeroing this_disk (which would alias
>    slot 0).
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzbot+9e3014263a35700ab49b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=9e3014263a35700ab49b
> Signed-off-by: Chandradhar Kumar <chandradhar.2003@gmail.com>

[Severity: Low]
Commit 1da177e4c3f4 ("Linux-2.6.12-rc2") cannot be resolved in the
repository (and points to the initial git epoch commit rather than a
specific introducing commit such as 1a33a379e678 ("md: reject v0.90 arrays
with too many devices")).

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 03e35386a8028..e733dcdd13a5b 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -1407,7 +1407,18 @@ 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 >= MD_SB_DISKS)
> +		goto abort;
> +
> +	if (sb->minor_version >= 91 &&
> +	    ((s64)sb->raid_disks - sb->delta_disks <= 0 ||
> +	     (s64)sb->raid_disks - sb->delta_disks > MD_SB_DISKS))
> +		goto abort;

[Severity: High]
Does subtracting sb->delta_disks without casting it to s32 break loading
any v0.91 array undergoing a shrinking reshape?

struct mdp_superblock_s declares delta_disks as __u32 even though it stores
a signed two's-complement delta (assigned to signed int mddev->delta_disks
in super_90_validate()).  Promoting __u32 sb->delta_disks to s64
zero-extends negative values to [2147483648, 4294967295], so
(s64)sb->raid_disks - sb->delta_disks is always negative when shrinking an
array and super_90_load() rejects the superblock with -EINVAL, while the
> MD_SB_DISKS check is unreachable.  Casting sb->delta_disks to s32 before
subtracting avoids the unsigned zero-extension.

[ ... ]

> @@ -5056,10 +5113,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: Low]
Can switching an array to v0.90 metadata in metadata_store() (or
md_set_array_info() when info->raid_disks == 0) still leave the pre-reshape
disk count (mddev->raid_disks - mddev->delta_disks) above MD_SB_DISKS?

While an array is non-persistent and has no disks attached, writing a value
above MD_SB_DISKS to raid_disks_store(), setting reshape_position, and then
reducing raid_disks to at most MD_SB_DISKS records the original count in
mddev->raid_disks - mddev->delta_disks.  Switching to v0.90 metadata then
passes the mddev->raid_disks > MD_SB_DISKS check, causing super_90_sync()
to write a v0.91 superblock that super_90_load() rejects as invalid.

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