linux-kernel - RE: [PATCH] Fix chance of sign extension to nsec after its msb is set during calculation.

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]

Message-ID: <alpine.DEB.2.20.1609041236340.5647@nanos>
Date:   Sun, 4 Sep 2016 12:39:27 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Liav Rehana <liavr@...lanox.com>
cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "john.stultz@...aro.org" <john.stultz@...aro.org>,
        Noam Camus <noamca@...lanox.com>,
        Elad Kanfi <eladkan@...lanox.com>
Subject: RE: [PATCH] Fix chance of sign extension to nsec after its msb is
 set during calculation.

On Sun, 4 Sep 2016, Liav Rehana wrote:
> >> The root of the problem is that in case the multiplication of delta 
> >> and
> >> tkr->mult in the line that I've changed is too big that the MSB of the
> >> result is set, then the shift will cause an unwanted sign extension.
> 
> > I completely understand that, but as I said before:
> 
> > > > This typecast is just a baindaid. What happens if you double the 
> > > > suspend time?  The multiplication will simply overflow. So the 
> > > > proper fix is to sanity check delta and do multiple conversions if 
> > > > delta is big enough.  Preferrably this happens somewhere at the call 
> > > > site and not in this hotpath function.
> 
> > > That sign extension will be avoided completely if the variable nsec 
> > > was unsigned (u64 instead of s64), so I think the correct solution for 
> > > this is to change the type of nsec to u64.
> 
> > That's a different story and its not a solution for the general problem of
> 
> >        delta * mult >= (1 << 31) or delta * mult >= (1 << 32)
> 
> The case that delta * mult >= 1 << 31 is not a problem by itself, but it causes
> an unwanted sign extension since the type of nsec is signed. That sign
> extension is what causes the loop to take too long, and not the overflow.
> I understand that the typecast is not a general solution, so as I've said, I
> think that changing the type of nsec to u64 instead of s64 will be a good and
> general solution, as it will indeed solve the problem of the unwanted sign
> extension.
> 
> To summarize: a sign extension occurs if the nsec variable is signed, and so
> I ask if you think it will be a good solution to change its type to unsigned.

Do you actually read what I write? I asked John before:

> John, why is that stuff signed at all? Shouldn't we use u64 for all of this?

So to summarize: 

   - Yes, we can use u64 if there is nothing which I missed, but John will
     have the last word on this

   - No, making it u64 does not solve the general problem. It just papers
     over the problem you observe. And we don't add 'paper over' fixes,
     period.

Thanks,

	tglx