linux-kernel - Re: [PATCH v2 2/2] linux/bits.h: Add compile time sanity check of GENMASK inputs

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, 7 Aug 2019 23:55:34 +0900
From:   Masahiro Yamada <yamada.masahiro@...ionext.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Rikard Falkeborn <rikard.falkeborn@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Joe Perches <joe@...ches.com>,
        Johannes Berg <johannes@...solutions.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] linux/bits.h: Add compile time sanity check of
 GENMASK inputs

On Wed, Aug 7, 2019 at 11:27 PM Guenter Roeck <linux@...ck-us.net> wrote:
>
> On Fri, Aug 02, 2019 at 01:03:58AM +0200, Rikard Falkeborn wrote:
> > GENMASK() and GENMASK_ULL() are supposed to be called with the high bit
> > as the first argument and the low bit as the second argument. Mixing
> > them will return a mask with zero bits set.
> >
> > Recent commits show getting this wrong is not uncommon, see e.g.
> > commit aa4c0c9091b0 ("net: stmmac: Fix misuses of GENMASK macro") and
> > commit 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK
> > macro").
> >
> > To prevent such mistakes from appearing again, add compile time sanity
> > checking to the arguments of GENMASK() and GENMASK_ULL(). If both the
> > arguments are known at compile time, and the low bit is higher than the
> > high bit, break the build to detect the mistake immediately.
> >
> > Since GENMASK() is used in declarations, BUILD_BUG_ON_ZERO() must be
> > used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead
> > of __builtin_constant_p().
> >
> > If successful, BUILD_BUG_OR_ZERO() returns 0 of type size_t. To avoid
> > problems with implicit conversions, cast the result of BUILD_BUG_OR_ZERO
> > to unsigned long.
> >
> > Since both BUILD_BUG_ON_ZERO() and __is_constexpr() only uses sizeof()
> > on the arguments passed to them, neither of them evaluate the expression
> > unless it is a VLA. Therefore, GENMASK(1, x++) still behaves as
> > expected.
> >
> > Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends
> > available in assembly") made the macros in linux/bits.h available in
> > assembly. Since neither BUILD_BUG_OR_ZERO() or __is_constexpr() are asm
> > compatible, disable the checks if the file is included in an asm file.
> >
>
> Who is going to fix the fallout ? For example, arm64:defconfig no longer
> compiles with this patch applied.
>
> It seems to me that the benefit of catching misuses of GENMASK is much
> less than the fallout from no longer compiling kernels, since those
> kernels won't get any test coverage at all anymore.


We cannot apply this until we fix all errors.

I do not understand why Andrew picked up this so soon.

-- 
Best Regards
Masahiro Yamada