Re: [PATCH v2 2/3] md/raid5: fix leak and use-after-free in resize_stripes error path

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

Pre-existing issues:
- [Critical] Concurrent lockless read of `conf->disks` in IO and error paths races with synchronous `kfree(old_disks)`.
- [High] Memory leak of `stripe_head` pages (`nsh->dev[i].page`) when Step 4 of `resize_stripes()` fails.
--

commit 42a858726c79b14d53c892c406bd10d16e4932f8
Author: Cao Guanghui <[email protected]>

md/raid5: fix leak and use-after-free in resize_stripes error path

This commit attempts to fix a memory leak and a use-after-free issue
in the resize_stripes error path. It defers the conf->disks replacement
and performs it under mddev->lock to prevent concurrent reads in
raid5_status from seeing a freed array.

Cc: [email protected]

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 4efaef622fc05..e481c48b45093 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c

[ ... ]

> @@ -2685,8 +2683,20 @@ static int resize_stripes(struct r5conf *conf, int newsize)
>  	}
>  	/* critical section pass, GFP_NOIO no longer needed */
>  
> -	if (!err)
> +	if (!err && ndisks) {
> +		struct disk_info *old_disks = conf->disks;
> +
> +		spin_lock_irq(&conf->mddev->lock);
> +		conf->disks = ndisks;
> +		spin_unlock_irq(&conf->mddev->lock);
> +		kfree(old_disks);

[Severity: Critical]
This is a pre-existing issue, but the patch's attempt to fix the conf->disks
use-after-free is incomplete and leaves critical race windows open. This
kfree() is synchronous and doesn't wait for an RCU grace period.

Because check_reshape() invokes resize_stripes() before reshape_position is
updated, chunk_aligned_read is still enabled. Does this allow
raid5_read_one_chunk() to run concurrently and read the freed conf->disks
array completely locklessly?

drivers/md/raid5.c:raid5_read_one_chunk() {
    ...
    rdev = conf->disks[dd_idx].replacement;
    if (!rdev || test_bit(Faulty, &rdev->flags) ||
        READ_ONCE(rdev->recovery_offset) < end_sector) {
        rdev = conf->disks[dd_idx].rdev;
    ...
}

Additionally, raid5_calc_degraded() iterates over conf->disks holding only
rcu_read_lock(). Will this synchronous kfree() cause a use-after-free for
those RCU readers as well?

drivers/md/raid5.c:raid5_calc_degraded() {
    ...
    struct md_rdev *rdev = READ_ONCE(conf->disks[i].rdev);
    ...
}

>  		conf->pool_size = newsize;
> +	} else if (ndisks) {

[Severity: High]
This isn't a bug introduced by this patch, but the patch attempts to fix
leaks in this error path and structurally misses this one. When Step 4 of
resize_stripes() fails to allocate a page for the new stripe slots
(nsh->dev[i].page), err is set to -ENOMEM and the pool_size update is
skipped here.

During array teardown, shrink_buffers() iterates only up to
sh->raid_conf->pool_size:

drivers/md/raid5.c:shrink_buffers() {
    ...
    int num = sh->raid_conf->pool_size;

#if PAGE_SIZE == DEFAULT_STRIPE_SIZE
    for (i = 0; i < num ; i++) {
    ...
}

Are the successfully allocated pages at indices >= pool_size permanently
leaked when free_stripe() destroys the slab object, because the cleanup loop
misses them?

> +		for (i = conf->pool_size; i < newsize; i++)
> +			if (ndisks[i].extra_page)
> +				put_page(ndisks[i].extra_page);
> +		kfree(ndisks);
> +	}
>  	mutex_unlock(&conf->cache_size_mutex);
>  
>  	return err;

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