Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Mime-Version: 1.0
Subject: RE: Re: [PATCH 1/1] mpi: check for shift exponent greater than 31.
Reply-To: v.narang@samsung.com
From:   Vaneet Narang <v.narang@samsung.com>
To:     Ayush Mittal <ayush.m@samsung.com>
CC:     "dhowells@redhat.com" <dhowells@redhat.com>,
        "aryabinin@virtuozzo.com" <aryabinin@virtuozzo.com>,
        "james.l.morris@oracle.com" <james.l.morris@oracle.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        AMIT SAHRAWAT <a.sahrawat@samsung.com>,
        PANKAJ MISHRA <pankaj.m@samsung.com>
In-Reply-To: <9150.1509545421@warthog.procyon.org.uk>
Message-ID: <20171109145541epcms5p639da345b9cfdf020d35b10f962617cd6@epcms5p6>
Date:   Thu, 09 Nov 2017 14:55:41 +0000
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 105P
DLP-Filter: Pass
References: <9150.1509545421@warthog.procyon.org.uk>
        <1508822823-31231-1-git-send-email-ayush.m@samsung.com>
        <CGME20171024052942epcas5p48384a3861fbfa90a37a03236828b9b67@epcms5p6>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

=C2=A0=0D=0AHi,=0D=0A=0D=0A>>=20This=20patch=20check=20for=20shift=20expone=
nt=20greater=20than=2031,=0D=0A=0D=0AYes,=20This=20should=20be=20=22check=
=20for=20shift=20exponent=20greater=20than=20BITS_PER_LONG=22=0D=0A=0D=0A>F=
irstly,=20isn't=20it=2063=20on=2064-bit=20machines?=0D=0A=0D=0ADescription=
=20of=20patch=20is=20specific=20to=2032bit=20machine=20but=20patch=20is=20m=
ade=20considering=2064bit=20in=20mind=20also.=20=0D=0Aand=20this=20is=20the=
=20precisly=20the=20reason=20we=20have=20been=20comparing=20with=20BITS_PER=
_MPI_LIMB=20=0D=0Aas=20BITS_PER_MPI_LIMB=20is=20BITS_PER_LONG.=0D=0A=0D=0A>=
Secondly,=20this=20is=20the=20wrong=20way=20to=20do=20things.=20=20The=20ba=
nner=20comment=20on=0D=0A>mpihelp_lshift(),=20for=20example,=20says=20that=
=20the=20function=20has=20the=20following=0D=0A>argument=20constraints:=0D=
=0A=0D=0A>=090=20<=20CNT=20<=20BITS_PER_MP_LIMB=0D=0A=0D=0A>so=20sh1=20and=
=20sh2=20must=20both=20be=20in=20the=20range=201-31=20or=201-63,=20assuming=
=20cnt=20is=0D=0A>within=20its=20constraints.=0D=0A=0D=0AYou=20are=20right,=
=20there=20is=20already=20a=20comment=20for=20the=20range=20but=20in=20our=
=20case=20caller=20was=20mpi_powm()=20itself=0D=0Aso=20to=20fix=20UBSAN=20w=
arning=20we=20prefferd=20handling=20in=20mpihelp_lshift/mpihelp_rshift.=0D=
=0A=0D=0ACall=20Sequence:=0D=0A=5B0-1.1677=5D=20=5B<c03ba190>=5D=20(ubsan_e=
pilogue)=20from=20=5B<c03ba754>=5D=20(__ubsan_handle_shift_out_of_bounds+0x=
f4/0x13c)=0D=0A=5B0-1.1677=5D=20=5B<c03ba660>=5D=20(__ubsan_handle_shift_ou=
t_of_bounds)=20from=20=5B<c03b5a44>=5D=20(mpihelp_lshift+0xf0/0x160)=0D=0A=
=5B0-1.1677=5D=20=5B<c03b5954>=5D=20(mpihelp_lshift)=20from=20=5B<c03b8920>=
=5D=20(mpi_powm+0x308/0xc7c)=0D=0A=0D=0AScenario:=20MPI=20mod=20passed=20to=
=20mpi_powm=20has=200=20last=20valid=20limb.=20=0D=0A=0D=0Acount_leading_ze=
ros()=20returns=2032/64=20when=20last=20valid=20limb=20of=20mod=20is=200.=
=0D=0A=0D=0A*=20If=20=40x=20is=200=20then=20the=20result=20is=20COUNT_LEADI=
NG_ZEROS_0.=0D=0A*/=0D=0Astatic=20inline=20int=20count_leading_zeros(unsign=
ed=20long=20x)=0D=0A=23define=20COUNT_LEADING_ZEROS_0=20BITS_PER_LONG=0D=0A=
=0D=0Aint=20mpi_powm(MPI=20res,=20MPI=20base,=20MPI=20exp,=20MPI=20mod)=0D=
=0A=7B=0D=0A....=0D=0A=20=20=20=20=20=20=20=20mod_shift_cnt=20=3D=20count_l=
eading_zeros(mod->d=5Bmsize=20-=201=5D);=20=20-->=20count_leading_zeros=20c=
an=20return=2032/64=0D=0A=20=20=20=20=20=20=20=20if=20(mod_shift_cnt)=0D=0A=
=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20mpihelp_lshift(mp,=20mod->d=
,=20msize,=20mod_shift_cnt);=20-->=2032/64=20can=20be=20passed=20to=20mpihe=
lp_lshift=0D=0A....=0D=0A=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20/*=
=20Remove=20any=20leading=20zero=20words=20from=20the=20result.=20=20*/=0D=
=0A=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20if=20(mod_shift_cnt)=0D=
=0A=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=
mpihelp_rshift(rp,=20rp,=20rsize,=20mod_shift_cnt);=20-->=20Similarly=2032/=
64=20can=20be=20passed=20to=20mpihelp_rshift=0D=0A....=09=09=09=09=09=09=0D=
=0A=7D=09=09=09=09=09=09=0D=0A=0D=0A=0D=0A>Therefore=20if=20it=20needs=20a=
=20checking,=20you=20only=20need=20to=20check=20cnt=20on=20entry=20to=20the=
=0D=0A>function,=20rather=20than=20checking=20sh1=20and=20sh2=20inside=20th=
e=20loop.=20=20Further,=20you=0D=0A>should=20use=20pr_err()=20so=20that=20w=
e=20know=20that=20this=20has=20gone=20wrong=20and=20return=20an=0D=0A>error=
=20to=20the=20caller=20(there=20are=20security=20implications).=0D=0A=0D=0A=
Since=20we=20were=20not=20sure=20about=20checking=20cnt=20and=20returning=
=20some=20error=20value.=20we=20thought=20it=20may=20break=20mpi_powm().=0D=
=0AWe=20preferred=20doing=20a=20clean=20handling=20of=20invalid=20input=20w=
ithout=20changing=20any=20behaviour=20of=20mpihelp_lshift/rshift=20.=20=0D=
=0APlease=20suggest=20how=20overflow=20can=20handled=20without=20breaking=
=20mpi_powm().=0D=0A=0D=0A>Further,=20have=20you=20checked=20the=20caller=
=20to=20see=20if=20they=20do=20ever=20violate=20the=0D=0A>constraints?=0D=
=0A=0D=0AFrom=20caller=20side,=20only=20issue=20which=20i=20can=20think=20i=
s=20passing=200=20in=20as=20last=20valid=20limb=20of=20mod.=20Is=20this=20a=
ny=0D=0Aconstraint=20?=0D=0A=0D=0ARegards,=0D=0AVaneet=20Narang

From 1582873159320778398@xxx Wed Nov 01 14:11:18 +0000 2017
X-GM-THRID: 1582115609131952865
X-Gmail-Labels: Inbox,Category Forums