Re: [PATCH] md: fix out-of-bounds access to superblock disk array

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Skipping devices with an out-of-bounds `desc_nr` in `super_90_sync()` leaves `rdev2->desc_nr` stale, silently corrupting the superblock's `this_disk` field.

Pre-existing issues:
- [High] The patch fails to validate `sb->this_disk.number` in `super_90_load()`, allowing an unvalidated value to be assigned to `rdev->desc_nr`, which subsequently leads to out-of-bounds array reads in `super_90_validate()`.
--

commit b64c4503567571cb6b3f8a25617dfc053378beea
Author: Deepanshu Kartikey <kartikey406@gmail.com>
md: fix out-of-bounds access to superblock disk array

This commit adds bounds checks for raid_disks and desc_nr when loading and
syncing v0.90 superblocks. It aims to prevent out-of-bounds array accesses
to the 27-element sb->disks array caused by maliciously crafted or
corrupted metadata.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 680b34a63cb3d..81588c3fee49a 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -1393,7 +1393,8 @@ static int super_90_load(struct md_rdev *rdev, struct md_rdev *refdev, int minor
>  		goto abort;
>  	}
>  
> -	if (sb->raid_disks <= 0)
> +	if (sb->raid_disks <= 0 ||
> +	    sb->raid_disks > MD_SB_DISKS)
>  		goto abort;
>  
>  	if (md_csum_fold(calc_sb_csum(sb)) != md_csum_fold(sb->sb_csum)) {

[Severity: High]
This is a pre-existing issue, but while we are adding bounds checks to
super_90_load(), does this function still leave a critical path vulnerable?

Slightly further down in super_90_load(), the code does:

    rdev->desc_nr = sb->this_disk.number;

Since sb->this_disk.number is read from the untrusted superblock without
validation against MD_SB_DISKS, an unvalidated value is assigned directly
to rdev->desc_nr. Later during array assembly, super_90_validate() 
unconditionally uses rdev->desc_nr to index the 27-element sb->disks array:

    desc = sb->disks + rdev->desc_nr;
    if (desc->state & (1<<MD_DISK_FAULTY))

Can a maliciously crafted MD superblock still trigger an out-of-bounds
memory read through this unvalidated rdev->desc_nr?

[ ... ]

> @@ -1693,6 +1694,13 @@ 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\n",
> +				mdname(mddev), desc_nr, rdev2->bdev);
> +			continue;
> +		}
> +

[Severity: High]
Does skipping out-of-bounds devices here leave rdev2->desc_nr with a stale
value?

By using continue to early-exit the loop, we skip the subsequent
rdev2->desc_nr = desc_nr assignment. Later in this same function, the
superblock's this_disk field is populated using rdev->desc_nr:

    if (rdev->desc_nr >= 0 && rdev->desc_nr < MD_SB_DISKS)
        sb->this_disk = sb->disks[rdev->desc_nr];

If the target device was skipped during the rdev_for_each loop, will it
reuse its stale desc_nr and copy the descriptor of whichever other active
device took that slot, potentially causing metadata corruption?

>  		rdev2->desc_nr = desc_nr;
>  		d = &sb->disks[rdev2->desc_nr];
>  		nr_disks++;

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