| Message ID | 20230901095341.55857-1-arefev@swemel.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | srcu: The value may overflow | expand |
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. 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;
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; >
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); } smp_store_release(&ssp->srcu_sup->srcu_size_state, SRCU_SIZE_WAIT_BARRIER); return true;
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(-)