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 [off-list ref]

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.

[ ... ]

                ^^^^^^^^^^^^^^^^^

[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/20260827063235.85375-1-ghuicao@163.com?part=2