On 9/4/23 08:42, Mathieu Desnoyers wrote:
On 9/4/23 08:21, 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.
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.
We could expand on this:
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 on a 64-bit system.
Moreover, when the subtraction value is 31, the 1 << 31 expression results in 0xffffffff80000000 when the signed integer is promoted to unsigned long on 64-bit systems due to type promotion rules, which is certainly not the intended result.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
With the commit message updated with my comment above, please also add:
Fixes: c7e88067c1 ("srcu: Exact tracking of srcu_data structures containing callbacks") Cc: stable@vger.kernel.org # v4.11
Sorry, the line above should read:
Cc: stable@vger.kernel.org # v4.11+
Thanks,
Mathieu
Reviewed-by: Mathieu Desnoyers mathieu.desnoyers@efficios.com
Thanks!
Mathieu
Signed-off-by: Denis Arefev arefev@swemel.ru
v3: Changed the name of the patch, as suggested by Mathieu Desnoyers mathieu.desnoyers@efficios.com v2: Added fixes to the srcu_schedule_cbs_snp function as suggested by Mathieu Desnoyers mathieu.desnoyers@efficios.com kernel/rcu/srcutree.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index 20d7a238d675..6c18e6005ae1 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); } smp_store_release(&ssp->srcu_sup->srcu_size_state, SRCU_SIZE_WAIT_BARRIER); return true; @@ -833,7 +833,7 @@ static void srcu_schedule_cbs_snp(struct srcu_struct *ssp, struct srcu_node *snp int cpu; for (cpu = snp->grplo; cpu <= snp->grphi; cpu++) { - if (!(mask & (1 << (cpu - snp->grplo)))) + if (!(mask & (1UL << (cpu - snp->grplo)))) continue; srcu_schedule_cbs_sdp(per_cpu_ptr(ssp->sda, cpu), delay); }