Re: [PATCH v5 04/29] md/raid5: reject zero-sector reshape chunks

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Incomplete Fix / Integer Overflow and Validation Bypass
- [Critical] State Corruption / Incorrect Error Handling during Reshape Resume
--

--- Patch [4]: [PATCH v5 04/29] md/raid5: reject zero-sector reshape chunks ---
commit 7e4615eab2a0600f99886ba69199c0ee0d8cd320
Author: Yu Kuai <yukuai@fygo.io>

md/raid5: reject zero-sector reshape chunks
    
Sashiko reported that RAID5 can accept a reshape chunk size that becomes
zero sectors. chunk_size_store() stores the sysfs byte value as n >> 9, so
writing a value below 512 bytes sets mddev->new_chunk_sectors to zero.
RAID5 then accepted that pending reshape geometry and raid5_start_reshape()
installed it into conf->chunk_sectors, letting reshape code divide by zero.

[ ... ]

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index e2c5a7072aca0..d128d238e1daa 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -8548,6 +8548,8 @@ static int check_reshape(struct mddev *mddev)
>  		return 0; /* nothing to do */
>  	if (has_failed(conf))
>  		return -EINVAL;
> +	if (!mddev->new_chunk_sectors)
> +		return -EINVAL;

[Severity: Critical]
This is a pre-existing issue, but does this check adequately prevent invalid
chunk sizes if mddev->new_chunk_sectors is negative?

If a privileged user writes an excessively large value to the chunk_size sysfs
attribute (like 0x10000000200), chunk_size_store() in drivers/md/md.c shifts
it by 9 and stores it into the 32-bit signed integer mddev->new_chunk_sectors.
This can result in a negative value (e.g., -2147483647).

Since negative values are non-zero, they bypass this check. Will this negative
value then bypass the validation in raid5_check_reshape() which only checks
positive values?

drivers/md/raid5.c:raid5_check_reshape() {
    ...
    if (new_chunk > 0) {
        if (!is_power_of_2(new_chunk))
            return -EINVAL;
    ...
}

Could this also cause a 32-bit shift overflow in check_stripe_cache(),
passing the limit checks, and eventually lead to a divide-by-zero panic in
make_discard_request() due to a 32-bit multiplication overflow when
calculating stripe_sectors?

[Severity: Critical]
This isn't a bug introduced by this patch, but could returning an error
code like -EINVAL here cause state corruption during a reshape resume?

Looking at mddev_start_reshape() in drivers/md/md.c, it evaluates the return
value of check_reshape() in a boolean context:

drivers/md/md.c:mddev_start_reshape() {
    ...
    if (mddev->reshape_position == MaxSector ||
        mddev->pers->check_reshape == NULL ||
        mddev->pers->check_reshape(mddev)) {
        clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
        ret = mddev->pers->start_reshape(mddev);
    ...
}

If check_reshape() returns -EINVAL (or -ENOMEM), the condition evaluates
to true in C, causing mddev_start_reshape() to call
mddev->pers->start_reshape(mddev).

If this happens during a resume, won't calling raid5_start_reshape()
incorrectly reset conf->reshape_progress and double-apply the delta_disks?

drivers/md/raid5.c:raid5_start_reshape() {
    ...
    conf->previous_raid_disks = conf->raid_disks;
    conf->raid_disks += mddev->delta_disks;
    ...
}

Since conf->raid_disks is already set to the target size in setup_conf()
during resume, would this result in an out-of-bounds access on conf->disks?

>  	if (mddev->delta_disks < 0 && mddev->reshape_position == MaxSector) {
>  		/* We might be able to shrink, but the devices must

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260802195038.164272-1-yukuai@kernel.org?part=4