Re: [PATCH v3] 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: 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