linux-kernel - Re: [PATCH v2] md/raid5: fix atomicity violation in raid5_cache

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8788068e-39ae-4307-b789-03d90d414421@gmail.com>
Date: Thu, 11 Jan 2024 10:09:34 +0800
From: Gui-Dong Han <2045gemini@...il.com>
To: song@...nel.org, yukuai3@...wei.com
Cc: linux-raid@...r.kernel.org, linux-kernel@...r.kernel.org,
 baijiaju1990@...look.com, BassCheck <bass@...a.edu.cn>
Subject: Re: [PATCH v2] md/raid5: fix atomicity violation in raid5_cache_count

Dear Kuai,

Due to unknown reasons, I did not see your response in my Gmail. I came 
across it on Patchwork and understand the need to pair READ_ONCE() with 
WRITE_ONCE(). I'll make these changes and submit patch v3 soon. Thank 
you for your guidance.

Best,
Han

On 11/1/2024 上午9:51, Gui-Dong Han wrote:
> Dear All:
>
> I hope this email finds you well. I hope you haven't missed my 
> previous email, as I understand that everyone has a busy schedule. I 
> just wanted to follow up on my previous message sent.
> I understand that you may be occupied with other tasks or priorities. 
> However, I would greatly appreciate it if you could spare a few 
> moments to check the patch in my previous email. Your prompt response 
> would be highly valuable to me.
> Thank you for your attention to this matter, and I look forward to 
> hearing from you soon.
>
> Thanks,
> Han
>
> On 22/12/2023 下午12:52, Gui-Dong Han wrote:
>> In raid5_cache_count():
>>     if (conf->max_nr_stripes < conf->min_nr_stripes)
>>         return 0;
>>     return conf->max_nr_stripes - conf->min_nr_stripes;
>> The current check is ineffective, as the values could change immediately
>> after being checked.
>>
>> In raid5_set_cache_size():
>>     ...
>>     conf->min_nr_stripes = size;
>>     ...
>>     while (size > conf->max_nr_stripes)
>>         conf->min_nr_stripes = conf->max_nr_stripes;
>>     ...
>>
>> Due to intermediate value updates in raid5_set_cache_size(), concurrent
>> execution of raid5_cache_count() and raid5_set_cache_size() may lead to
>> inconsistent reads of conf->max_nr_stripes and conf->min_nr_stripes.
>> The current checks are ineffective as values could change immediately
>> after being checked, raising the risk of conf->min_nr_stripes exceeding
>> conf->max_nr_stripes and potentially causing an integer overflow.
>>
>> This possible bug is found by an experimental static analysis tool
>> developed by our team. This tool analyzes the locking APIs to extract
>> function pairs that can be concurrently executed, and then analyzes the
>> instructions in the paired functions to identify possible concurrency 
>> bugs
>> including data races and atomicity violations. The above possible bug is
>> reported when our tool analyzes the source code of Linux 6.2.
>>
>> To resolve this issue, it is suggested to introduce local variables
>> 'min_stripes' and 'max_stripes' in raid5_cache_count() to ensure the
>> values remain stable throughout the check. Adding locks in
>> raid5_cache_count() fails to resolve atomicity violations, as
>> raid5_set_cache_size() may hold intermediate values of
>> conf->min_nr_stripes while unlocked. With this patch applied, our 
>> tool no
>> longer reports the bug, with the kernel configuration allyesconfig for
>> x86_64. Due to the lack of associated hardware, we cannot test the patch
>> in runtime testing, and just verify it according to the code logic.
>>
>> Fixes: edbe83ab4c27e ("md/raid5: allow the stripe_cache to grow and 
>> ...")
>> Reported-by: BassCheck <bass@...a.edu.cn>
>> Signed-off-by: Gui-Dong Han <2045gemini@...il.com>
>>
>> ---
>> v2:
>> * In this patch v2, we've updated to use READ_ONCE() instead of direct
>> reads for accessing max_nr_stripes and min_nr_stripes, since read and
>> write can concurrent.
>>    Thank Yu Kuai for helpful advice.
>> ---
>>   drivers/md/raid5.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index 8497880135ee..9037e46de0e2 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -7391,10 +7391,12 @@ static unsigned long raid5_cache_count(struct 
>> shrinker *shrink,
>>   {
>>       struct r5conf *conf = shrink->private_data;
>>   -    if (conf->max_nr_stripes < conf->min_nr_stripes)
>> +    int max_stripes = READ_ONCE(conf->max_nr_stripes);
>> +    int min_stripes = READ_ONCE(conf->min_nr_stripes);
>> +    if (max_stripes < min_stripes)
>>           /* unlikely, but not impossible */
>>           return 0;
>> -    return conf->max_nr_stripes - conf->min_nr_stripes;
>> +    return max_stripes - min_stripes;
>>   }
>>     static struct r5conf *setup_conf(struct mddev *mddev)