Re: [PATCH] dm-raid: bound array_position before indexing failed_devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dm-raid: wrong role bound and continue bypass in super_init_validation()
--

commit 46b04454594856163e75cd4dccd01fd7ff3efea6
Author: Jay Vadayath <jay@artiphishell.com>

dm-raid: bound array_position before indexing failed_devices

This commit bounds the on-disk array_position (role) by rs->raid_disks in
super_init_validation() before using it as a bit index into the on-stack
failed_devices bitmap.

> diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
> index ac019a0f5a929..b973181bdce24 100644
> --- a/drivers/md/dm-raid.c
> +++ b/drivers/md/dm-raid.c
> @@ -2420,7 +2420,8 @@ static int super_init_validation(struct raid_set *rs, struct md_rdev *rdev)
>  		 */
>  		if (!test_bit(FirstUse, &r->flags) && (r->raid_disk >= 0)) {
>  			role = le32_to_cpu(sb2->array_position);
> -			if (role < 0)
> +			/* Bound @role before it indexes the failed_devices stack bitmap. */
> +			if (role < 0 || role >= rs->raid_disks)
>  				continue;

[Severity: High]
Does using continue when role >= rs->raid_disks in
drivers/md/dm-raid.c:super_init_validation() introduce two regressions during
table load (dm_ctl_ioctl() -> table_load() -> dm_table_add_target() ->
raid_ctr() -> analyse_superblocks() -> super_validate() ->
super_init_validation())?

First, the commit message states that this change rejects anything outside
[0, rs->raid_disks), but executing continue only skips the remainder of the
per-rdev loop body and allows super_init_validation() to return 0:

drivers/md/dm-raid.c:super_init_validation() {
    ...
		if (!test_bit(FirstUse, &r->flags) && (r->raid_disk >= 0)) {
			role = le32_to_cpu(sb2->array_position);
			/* Bound @role before it indexes the failed_devices stack bitmap. */
			if (role < 0 || role >= rs->raid_disks)
				continue;

			if (role != r->raid_disk) {
				...
				} else if (!(rs_is_raid10(rs) && rt_is_raid0(rs->raid_type)) &&
					   !(rs_is_raid0(rs) && rt_is_raid10(rs->raid_type)) &&
					   !rt_is_raid1(rs->raid_type)) {
					rs->ti->error = "Cannot change device positions in raid set";
					return -EINVAL;
				}
    ...
}

Because r->raid_disk is in [0, rs->raid_disks), any device with
role >= rs->raid_disks has role != r->raid_disk. Previously, on raid levels
that forbid device reordering (such as raid4/5/6 or raid10 far/offset), a
device with role in [rs->raid_disks, 255] entered the role != r->raid_disk
block and failed validation with return -EINVAL. With this continue, shouldn't
an out-of-bounds role return -EINVAL instead of silently bypassing both the
device reordering check and the failed_devices check?

Second, is rs->raid_disks the right upper bound for role here? In
super_init_validation(), role is the device's previous position in the
on-disk array of size mddev->raid_disks (le32_to_cpu(sb->num_devices)), and
failed_devices is indexed by that previous role, whereas rs->raid_disks is
the device count of the newly loaded table.

dm-raid allows loading tables with rs->raid_disks < mddev->raid_disks when
removing legs from a raid1 array without delta_disks or when removing mirror
copies from a raid10_near array:

drivers/md/dm-raid.c:rs_prepare_reshape() {
	if (rs_is_raid10(rs)) {
		if (rs->raid_disks != mddev->raid_disks &&
		    __is_raid10_near(mddev->layout) &&
		    rs->raid10_copies &&
		    rs->raid10_copies != __raid10_near_copies(mddev->layout)) {
			...
			__reorder_raid_disk_indexes(rs);
			...
		}
	...
	} else if (rs_is_raid1(rs)) {
		if (rs->delta_disks) {
			...
		} else {
			/* Process raid1 without delta_disks */
			mddev->raid_disks = rs->raid_disks;
			reshape = false;
		}
	}
}

When a non-trailing leg or mirror copy is removed so that rs->raid_disks <
mddev->raid_disks (for example, removing leg 0 from a 3-disk raid1 array so
old device 2 moves to r->raid_disk = 1 with rs->raid_disks = 2, or reducing a
6-disk 3-copy raid10_near set to a 4-disk 2-copy set so old device 4 moves to
r->raid_disk = 3 with rs->raid_disks = 4), surviving devices legitimately
have rs->raid_disks <= role < mddev->raid_disks.

If role >= rs->raid_disks takes the continue branch, won't
super_init_validation() skip updating sb2->array_position for raid10_near and
skip testing failed_devices to set Faulty on a returning failed device,
preventing super_validate() from clearing In_sync and resetting
recovery_offset to 0?

>  
>  			if (role != r->raid_disk) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022181934.35685-1-jay@artiphishell.com?part=1