Re: [PATCH] md: reject v0.90 arrays with too many devices

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

Pre-existing issues:
- [High] The bounds check on `raid_disks` is incomplete and allows out-of-bounds writes in `super_90_sync()`.
- [High] Missing bounds check on `sb->this_disk.number` allows arbitrary out-of-bounds memory reads leading to kernel panic.
--

--- Patch [1]: [PATCH] md: reject v0.90 arrays with too many devices ---
commit 29dd6c25e23c9e9c7bcdc332429c7ae58ceccde2
Author: Jérémy Jean <[email protected]>
Subject: md: reject v0.90 arrays with too many devices

The commit updates super_90_load to reject arrays where the raid_disks
value exceeds MD_SB_DISKS. This prevents out-of-bounds writes during
superblock metadata updates when processing a crafted v0.90 format
superblock.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 680b34a63cb3d..154ee5a65cb79 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -1393,7 +1393,7 @@ 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)

[Severity: High]
This isn't a bug introduced by this patch, but does allowing sb->raid_disks
to equal MD_SB_DISKS (which is 27) leave the array vulnerable to an
out-of-bounds write?

If sb->raid_disks is exactly MD_SB_DISKS, and the array has missing active
disks but contains spare or faulty devices, super_90_sync() will initialize
next_spare to mddev->raid_disks (27).

For each spare or faulty device, it unconditionally assigns
desc_nr = next_spare++ and writes to sb->disks[desc_nr]:

drivers/md/md.c:super_90_sync() {
    ...
    desc_nr = next_spare++;
    ...
    d = &sb->disks[rdev2->desc_nr];
    ...
}

Since sb->disks has exactly 27 elements, this means next_spare starts out
of bounds. Successive spare devices could cause writes to sb->disks[28],
sb->disks[29], exceeding the 4KB superblock page. Should this validation
ensure raid_disks is strictly less than MD_SB_DISKS?

[Severity: High]
This is also a pre-existing issue, but further down in super_90_load(),
sb->this_disk.number is extracted into rdev->desc_nr:

drivers/md/md.c:super_90_load() {
    ...
    rdev->desc_nr = sb->this_disk.number;
    ...
}

Is it possible for an untrusted value to be extracted here without verifying
it is strictly less than MD_SB_DISKS?

Later, when array assembly calls md_run(), it executes super_90_validate()
which uses this unvalidated value as an array index:

drivers/md/md.c:super_90_validate() {
    ...
    desc = sb->disks + rdev->desc_nr;
    
    if (desc->state & (1<<MD_DISK_FAULTY))
    ...
}

Since rdev->desc_nr can be arbitrarily large, could this result in reading
memory far beyond the sb_page and trigger a kernel panic if it lands on an
unmapped page?

>  		goto abort;
>  
>  	if (md_csum_fold(calc_sb_csum(sb)) != md_csum_fold(sb->sb_csum)) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1