Re: [PATCH v3 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:

New issues:
- [Critical] Moving the replacement and freeing of `conf->disks` to after Step 4 introduces a severe Use-After-Free race with the main IO thread (`raid5d`).

Pre-existing issues:
- [High] Incomplete UAF fix: `conf->disks` is still vulnerable to a concurrent use-after-free by `raid5_error()` and `raid5_calc_degraded()`.
--

commit bd1f3c6216c041887763e5099827c3cd4343c423
Author: Cao Guanghui <[email protected]>

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

This commit addresses a memory leak when stripe slot allocation fails, and
attempts to fix a concurrent use-after-free by swapping the conf->disks array
under mddev->lock. The array replacement is deferred until after new stripe
slots are successfully allocated.

> 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]
By deferring the kfree() of old_disks until after Step 4, does this code
introduce a race with the main IO thread?

During Step 4 (just above this code), resize_stripes() calls
raid5_release_stripe(), which returns stripes to service and wakes up the
raid5d IO thread.

Once awakened, raid5d resumes processing stripes and will locklessly access
the disks array:

raid5d()
  handle_stripe()
    analyse_stripe()
      rdev = conf->disks[i].replacement;

Because raid5d does not acquire mddev->lock, won't it dereference the freed
old_disks array when this code executes concurrently?


[Severity: High]
This isn't a bug introduced by this patch, but does protecting the pointer
swap with mddev->lock completely prevent concurrent use-after-free of
conf->disks?

The commit message notes that raid5_status() reads conf->disks under
mddev->lock. However, a concurrent block IO error can trigger raid5_error(),
which calls raid5_calc_degraded():

raid5_error()
  raid5_calc_degraded()
    struct md_rdev *rdev = READ_ONCE(conf->disks[i].rdev);

In raid5_error(), this is protected by conf->device_lock, not mddev->lock.
If raid5_error() executes concurrently with this kfree(), won't it
dereference the freed old_disks array?

>  		conf->pool_size = newsize;
> +	} else if (ndisks) {
> +		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