linux-kernel - Re: [PATCH v2] The value may 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]

Message-ID: <ce51b768-9988-197c-0c35-7574aba77810@efficios.com>
Date:   Tue, 5 Sep 2023 10:34:43 -0400
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     David Laight <David.Laight@...LAB.COM>,
        "'paulmck@...nel.org'" <paulmck@...nel.org>
Cc:     Denis Arefev <arefev@...mel.ru>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Josh Triplett <josh@...htriplett.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        "rcu@...r.kernel.org" <rcu@...r.kernel.org>,
        "lvc-project@...uxtesting.org" <lvc-project@...uxtesting.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "trufanov@...mel.ru" <trufanov@...mel.ru>,
        "vfh@...mel.ru" <vfh@...mel.ru>
Subject: Re: [PATCH v2] The value may overflow

On 9/5/23 10:15, David Laight wrote:
> ...
>> That would instead be more than 512-16=496 CPUs, correct?  496 CPUs would
>> only require a 31-bit shift, which should be OK, but 497 would require
>> a 32-bit shift, which would result in sign extension.  If it turns out
>> that sign extension is OK, then we should get in trouble at 513 CPUs,
>> which would result in a 33-bit shift (and is that even defined in C?).
> 
> Not quite right :-)
> 
> (1 << 31) is int and negative, that gets sign extended before
> being converted to 'unsigned long' - so has the top 33 bits set.
> 
> (1 << 32) is undefined, the current x86 cpu ignore the high
> shift bits so it is (1 << 0).

Yes, I was about to reply the same thing. A shift of 31 is buggy,
because shifting 1 << 31 raises the sign bit, which sets the top 33
bits when cast to unsigned long. A shift of 1 << 32 is undefined,
with for instance x86 choosing to ignore the top bit.

But in order to have a 1 << 31 shift from this expression:

                 sdp->grpmask = 1 << (cpu - sdp->mynode->grplo);

I *think* we need the group to have 32 cpus or more (indexed within
the group from grplo to grplo + 31 (both inclusive)).

So as soon as we have one group with 32 cpus, the problem should show
up. With FANOUT_LEAF=16, we can have 15 groups of 31 cpus and 1
group of 32 cpus, for:

   15*31 + 32 = 497 cpus.

AFAIU, this would be the minimum value for smp_processor_id()+1 which
triggers this issue.

Thanks,

Mathieu

> 
> If the mask is being used to optimise a search the code might
> just happen to work!
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com