| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9c7d5e66-4f5f-b984-c291-f19c3d568b85@huaweicloud.com>
Date: Thu, 12 Jun 2025 17:17:44 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: Wang Jinchao <wangjinchao600@...il.com>, Yu Kuai
<yukuai1@...weicloud.com>, Song Liu <song@...nel.org>
Cc: linux-raid@...r.kernel.org, linux-kernel@...r.kernel.org,
"yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH v2] md/raid1: Fix stack memory use after return in
raid1_reshape
Hi,
在 2025/06/12 15:45, Wang Jinchao 写道:
> 在 2025/6/12 15:31, Yu Kuai 写道:
>> Hi,
>>
>> 在 2025/06/11 16:55, Wang Jinchao 写道:
>>> In the raid1_reshape function, newpool is
>>> allocated on the stack and assigned to conf->r1bio_pool.
>>> This results in conf->r1bio_pool.wait.head pointing
>>> to a stack address.
>>> Accessing this address later can lead to a kernel panic.
>>>
>>> Example access path:
>>>
>>> raid1_reshape()
>>> {
>>> // newpool is on the stack
>>> mempool_t newpool, oldpool;
>>> // initialize newpool.wait.head to stack address
>>> mempool_init(&newpool, ...);
>>> conf->r1bio_pool = newpool;
>>> }
>>>
>>> raid1_read_request() or raid1_write_request()
>>> {
>>> alloc_r1bio()
>>> {
>>> mempool_alloc()
>>> {
>>> // if pool->alloc fails
>>> remove_element()
>>> {
>>> --pool->curr_nr;
>>> }
>>> }
>>> }
>>> }
>>>
>>> mempool_free()
>>> {
>>> if (pool->curr_nr < pool->min_nr) {
>>> // pool->wait.head is a stack address
>>> // wake_up() will try to access this invalid address
>>> // which leads to a kernel panic
>>> return;
>>> wake_up(&pool->wait);
>>> }
>>> }
>>>
>>> Fix:
>>> The solution is to avoid using a stack-based newpool.
>>> Instead, directly initialize conf->r1bio_pool.
>>>
>>> Signed-off-by: Wang Jinchao <wangjinchao600@...il.com>
>>> ---
>>> v1 -> v2:
>>> - change subject
>>> - use mempool_init(&conf->r1bio_pool) instead of reinitializing the
>>> list on stack
>>> ---
>>> drivers/md/raid1.c | 34 +++++++++++++++++++---------------
>>> 1 file changed, 19 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>>> index 19c5a0ce5a40..f2436262092a 100644
>>> --- a/drivers/md/raid1.c
>>> +++ b/drivers/md/raid1.c
>>> @@ -3366,7 +3366,7 @@ static int raid1_reshape(struct mddev *mddev)
>>> * At the same time, we "pack" the devices so that all the missing
>>> * devices have the higher raid_disk numbers.
>>> */
>>> - mempool_t newpool, oldpool;
>>> + mempool_t oldpool;
>>> struct pool_info *newpoolinfo;
>>> struct raid1_info *newmirrors;
>>> struct r1conf *conf = mddev->private;
>>> @@ -3375,9 +3375,6 @@ static int raid1_reshape(struct mddev *mddev)
>>> int d, d2;
>>> int ret;
>>> - memset(&newpool, 0, sizeof(newpool));
>>> - memset(&oldpool, 0, sizeof(oldpool));
>>> -
>>> /* Cannot change chunk_size, layout, or level */
>>> if (mddev->chunk_sectors != mddev->new_chunk_sectors ||
>>> mddev->layout != mddev->new_layout ||
>>> @@ -3408,26 +3405,33 @@ static int raid1_reshape(struct mddev *mddev)
>>> newpoolinfo->mddev = mddev;
>>> newpoolinfo->raid_disks = raid_disks * 2;
>>> - ret = mempool_init(&newpool, NR_RAID_BIOS, r1bio_pool_alloc,
>>> - rbio_pool_free, newpoolinfo);
>>> - if (ret) {
>>> - kfree(newpoolinfo);
>>> - return ret;
>>> - }
>>> newmirrors = kzalloc(array3_size(sizeof(struct raid1_info),
>>> - raid_disks, 2),
>>> - GFP_KERNEL);
>>> + raid_disks, 2),
>>> + GFP_KERNEL);
>>> if (!newmirrors) {
>>> kfree(newpoolinfo);
>>> - mempool_exit(&newpool);
>>> return -ENOMEM;
>>> }
>>> + /* stop everything before switching the pool */
>>> freeze_array(conf, 0);
>>> - /* ok, everything is stopped */
>>> + /* backup old pool in case restore is needed */
>>> oldpool = conf->r1bio_pool;
>>> - conf->r1bio_pool = newpool;
>>> +
>>> + ret = mempool_init(&conf->r1bio_pool, NR_RAID_BIOS,
>>> r1bio_pool_alloc,
>>> + rbio_pool_free, newpoolinfo);
>>> + if (ret) {
>>> + kfree(newpoolinfo);
>>> + kfree(newmirrors);
>>> + mempool_exit(&conf->r1bio_pool);
>>> + /* restore the old pool */
>>> + conf->r1bio_pool = oldpool;
>>> + unfreeze_array(conf);
>>> + pr_err("md/raid1:%s: cannot allocate r1bio_pool for reshape\n",
>>> + mdname(mddev));
>>> + return ret;
>>> + }
>>> for (d = d2 = 0; d < conf->raid_disks; d++) {
>>> struct md_rdev *rdev = conf->mirrors[d].rdev;
>>>
>>
>> Any specific reason not to use mempool_resize() and krealloc() here?
>> In the case if new raid_disks is greater than the old one.
> The element size is different between the old pool and the new pool.
> mempool_resize only resizes the pool size (i.e., the number of elements
> in pool->elements), but does not handle changes in element size, which
> occurs in raid1_reshape.
>
> Another reason may be to avoid modifying the old pool directly — in case
> initializing the new pool fails, the old one remains usable.
>
> If we modify the old pool directly and the operation fails, not only
> will the reshaped RAID be unusable, but the original RAID may also be
> corrupted.
Yes, you're right, thanks for the explanation.
I feel like raid1_reshape() need better coding, anyway, your fix looks
good.
Reviewed-by: Yu Kuai <yukuai3@...wei.com>
>>
>> Thanks,
>> Kuai
>>
>
> .
>
Powered by blists - more mailing lists