linux-kernel - Re: [PATCH v11 09/15] iio: afe: rescale: reduce risk of integer 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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 22 Dec 2021 23:32:24 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Liam Beguin <liambeguin@...il.com>
Cc:     Peter Rosin <peda@...ntia.se>, Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>
Subject: Re: [PATCH v11 09/15] iio: afe: rescale: reduce risk of integer overflow

On Wed, Dec 22, 2021 at 9:59 PM Liam Beguin <liambeguin@...il.com> wrote:
> On Wed, Dec 22, 2021 at 08:56:12PM +0200, Andy Shevchenko wrote:
> > On Wed, Dec 22, 2021 at 8:38 PM Liam Beguin <liambeguin@...il.com> wrote:
> > > On Wed, Dec 22, 2021 at 02:29:04PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <liambeguin@...il.com> wrote:

...

> > > > > -               tmp = 1 << *val2;
> > > >
> > > > At some point this should be BIT()
> >
> > Forgot to add, If it's 64-bit, then BIT_ULL().
> >
> > > I'm not against changing this, but (to me at least) 1 << *val2 seems
> > > more explicit as we're not working with bitfields. No?
> >
> > You may add a comment. You may use int_pow(), but it will be suboptimal.
> >
> > > > Rule of thumb (in accordance with C standard), always use unsigned
> > > > value as left operand of the _left_ shift.
> > >
> > > Right, that makes sense! In practice though, since we'll most likely
> > > never use higher bits of *val2 with IIO_VAL_FRACTIONAL_LOG2, would it be
> > > enough to simply typecast?
> > >
> > >         tmp = 1 << (unsigned int)*val2;
> >
> > No, it's about the _left_ operand.
> > I haven't checked if tmp is 64-bit, then even that would be still wrong.
>
> Okay so your recommendation is to not use a left shift?

No, I recommend not to use int type as a _leftside_ operand.
BIT() / BIT_ULL() does a left shift anyway.

> I can look into that but given how unlikely it is to fall into those bad
> cases, I'd rather keep things as they are. Would that be okay?

> Also, I don't think using BIT() or BIT_ULL() would address this as they
> both do the same shift, with no extra checks.

They do slightly different versions of it. They use an unsigned int type.

Open coded or not, it's up to you. Just convert to unsigned int.

-- 
With Best Regards,
Andy Shevchenko