linux-kernel - [PATCH] 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, 31 Jul 2019 21:03:09 +0200
From:   Rikard Falkeborn <rikard.falkeborn@...il.com>
To:     joe@...ches.com
Cc:     akpm@...ux-foundation.org, johannes@...solutions.net,
        linux-kernel@...r.kernel.org, rikard.falkeborn@...il.com,
        yamada.masahiro@...ionext.com
Subject: [PATCH] linux/bits.h: Add compile time sanity check of GENMASK inputs

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_OR_ZERO() must be
used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead
of __builtin_constant_p().

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.

Signed-off-by: Rikard Falkeborn <rikard.falkeborn@...il.com>
---
Joe Perches sent a series to fix the existing misuses of GENMASK() that
needs to be merged before this to avoid build failures. Currently, 7 of
the patches were not in Linus tree, and 2 were not in linux-next.

Also, there's currently no asm users of bits.h, but since it was made
asm-compatible just two weeks ago it would be a shame to break it right
away...

 include/linux/bits.h | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/linux/bits.h b/include/linux/bits.h
index 669d69441a62..73489579eef9 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -18,12 +18,22 @@
  * position @h. For example
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
+#ifndef __ASSEMBLY__
+#include <linux/build_bug.h>
+#define GENMASK_INPUT_CHECK(h, l)  BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
+		__is_constexpr(h) && __is_constexpr(l), (l) > (h), 0))
+#else
+#define GENMASK_INPUT_CHECK(h, l) 0
+#endif
+
 #define GENMASK(h, l) \
+	(GENMASK_INPUT_CHECK(h, l) + \
 	(((~UL(0)) - (UL(1) << (l)) + 1) & \
-	 (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
+	 (~UL(0) >> (BITS_PER_LONG - 1 - (h)))))

 #define GENMASK_ULL(h, l) \
+	(GENMASK_INPUT_CHECK(h, l) + \
 	(((~ULL(0)) - (ULL(1) << (l)) + 1) & \
-	 (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
+	 (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h)))))

 #endif	/* __LINUX_BITS_H */
-- 
2.22.0