linux-kernel - Re: [PATCH] srcu: The value may overflow

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: <c337cc2d-5ae6-5fcd-dce6-658613a4564d@efficios.com>
Date:   Sat, 2 Sep 2023 10:17:24 -0400
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     Denis Arefev <arefev@...mel.ru>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        "Paul E. McKenney" <paulmck@...nel.org>
Cc:     Josh Triplett <josh@...htriplett.org>,
        Steven Rostedt <rostedt@...dmis.org>, rcu@...r.kernel.org,
        lvc-project@...uxtesting.org, linux-kernel@...r.kernel.org,
        trufanov@...mel.ru, vfh@...mel.ru
Subject: Re: [PATCH] srcu: The value may overflow

On 9/1/23 09:31, Mathieu Desnoyers wrote:
> On 9/1/23 05:53, Denis Arefev wrote:
>> The value of an arithmetic expression 1 << (cpu - sdp->mynode->grplo)
>> is subject to overflow due to a failure to cast operands to a larger
>> data type before performing arithmetic
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Signed-off-by: Denis Arefev <arefev@...mel.ru>
>> ---
>>   kernel/rcu/srcutree.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
>> index 20d7a238d675..e14b74fb1ba0 100644
>> --- a/kernel/rcu/srcutree.c
>> +++ b/kernel/rcu/srcutree.c
>> @@ -223,7 +223,7 @@ static bool init_srcu_struct_nodes(struct 
>> srcu_struct *ssp, gfp_t gfp_flags)
>>                   snp->grplo = cpu;
>>               snp->grphi = cpu;
>>           }
>> -        sdp->grpmask = 1 << (cpu - sdp->mynode->grplo);
>> +        sdp->grpmask = 1UL << (cpu - sdp->mynode->grplo);
> 
> What possible values of cpus supported by the Linux kernel and grplo can 
> cause this to overflow on 64-bit architectures ? I suspect the maximum 
> result of this subtraction is defined by the RCU_FANOUT or other srcu 
> level-spread values assigned by rcu_init_levelspread(), which can indeed 
> cause the signed 32-bit integer literal ("1") to overflow when shifted 
> by any value greater than 31. This analysis should be added to the 
> commit message so the impact of the issue can be understood.

Another effect that might be worth mentioning in the commit message is 
what happens with the sign-carry for "1 << 31" when it is promoted from 
signed 32-bit integer to unsigned long on a 64-bit kernel: it translates 
to 0xffffffff80000000, which is certainly not what is expected here.

Thanks,

Mathieu


> 
> I also notice this in the same file:
> 
> srcu_schedule_cbs_snp():
> 
>          for (cpu = snp->grplo; cpu <= snp->grphi; cpu++) {
>                  if (!(mask & (1 << (cpu - snp->grplo))))
>                          continue;
> 
> Which should be fixed at the same time.
> 
> Thanks,
> 
> Mathieu
> 
>>       }
>>       smp_store_release(&ssp->srcu_sup->srcu_size_state, 
>> SRCU_SIZE_WAIT_BARRIER);
>>       return true;
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com