Re: [PATCH] srcu: The value may overflow

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

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@swemel.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