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: <CAEXW_YQW_0RD5mmtwnzwik9Q9K9XL9rxHUyb98SLqCJ-840xOQ@mail.gmail.com>
Date:   Tue, 5 Sep 2023 16:56:12 -0400
From:   Joel Fernandes <joel@...lfernandes.org>
To:     paulmck@...nel.org
Cc:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        David Laight <David.Laight@...lab.com>,
        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>, ldufour@...ux.ibm.com
Subject: Re: [PATCH v2] The value may overflow

On Tue, Sep 5, 2023 at 4:52 PM Paul E. McKenney <paulmck@...nel.org> wrote:
>
> On Tue, Sep 05, 2023 at 04:40:46PM -0400, Joel Fernandes wrote:
[...]
> > > > > > So this bug appears to have no effect for default RCU setups, even on very
> > > > > > large 64-bit systems, which is consistent with field experience.  Even if
> > > > > > this is the case, it still should be fixed, to avoid confusion if nothing
> > > > > > else.  Or just in case someone decides to set CONFIG_RCU_FANOUT_LEAF=32.
> > > > > > Which actually happened the other day due to someone trusting ChatGPT's
> > > > > > opinion about RCU Kconfig options...
> > > > >
> > > > > And I therefore queued Denis's v3 patch with an edited commit log.
> > > > > Of course, if anyone sees some other way that the bug could manifest
> > > > > other than in a 64-bit kernel built with CONFIG_RCU_FANOUT_LEAF greater
> > > > > than 30 on a system with at least 31 CPUs, please let me know so that
> > > > > I can adjust.
> > > > >
> > > > >                                                     Thanx, Paul
> > > > >
> > > > > ------------------------------------------------------------------------
> > > > >
> > > > > commit ed083b0e22f1396dee3599896249a3f218845298
> > > > > Author: Denis Arefev <arefev@...mel.ru>
> > > > > Date:   Mon Sep 4 15:21:14 2023 +0300
> > > > >
> > > > >      Fix srcu_struct node grpmask overflow on 64-bit systems
> > > > >      The value of an arithmetic expression 1 << (cpu - sdp->mynode->grplo)
> > > >
> > > > AFAIU, the overflow resides in the "bitwise expression" and not
> > > > the arithmetic expression.
> > >
> > > Rather than quibble about exactly what constitutes arithmetic, I
> > > updated the first paragraph and added your Reviewed-by as shown
> > > below.  ;-)
> > >
> > > > Other than this, please add my
> > > >
> > > > Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
> > >
> > > Thank you all!!!
> > >
> > > ------------------------------------------------------------------------
> > >
> > > commit 50477ff756ab99402b1523b7c6be8b5d790d05e7
> > > Author: Denis Arefev <arefev@...mel.ru>
> > > Date:   Mon Sep 4 15:21:14 2023 +0300
> > >
> > >     Fix srcu_struct node grpmask overflow on 64-bit systems
> > >
> > >     The value of a bitwise expression 1 << (cpu - sdp->mynode->grplo)
> > >     is subject to overflow due to a failure to cast operands to a larger
> > >     data type before performing the bitwise operation.
> > >
> > >     The maximum result of this subtraction is defined by the RCU_FANOUT_LEAF
> > >     Kconfig option, which on 64-bit systems defaults to 16 (resulting in a
> > >     maximum shift of 15), but which can be set up as high as 64 (resulting
> > >     in a maximum shift of 63).  A value of 31 can result in sign extension,
> > >     resulting in 0xffffffff80000000 instead of the desired 0x80000000.
> > >     A value of 31 or greater triggers undefined behavior per the C standard.
> >
> > Do you mean here "A value of 32 or greater"?
> >
> > Only N >= 32 throws warning for:
> > unsigned long foo = (1 << N);
> >
> > N=31 does undesirable sign extension but no warning.
>
> Good catch, thank you, and I will update this on my next rebase.

Thanks, and with that the patch looks good to me:
Reviewed-by: Joel Fernandes (Google) <joel@...lfernandes.org>

 - Joel