linux-kernel - [PATCH] word-at-a-time: avoid undefined behaviour in zero

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-next>] [day] [month] [year] [list]

Date:	Wed, 23 Apr 2014 17:52:52 +0100
From:	Will Deacon <will.deacon@....com>
To:	torvalds@...ux-foundation.org
Cc:	linux-kernel@...r.kernel.org, Will Deacon <will.deacon@....com>,
	<stable@...r.kernel.org>,
	Victor Kamensky <victor.kamensky@...aro.org>
Subject: [PATCH] word-at-a-time: avoid undefined behaviour in zero_bytemask macro

The asm-generic, big-endian version of zero_bytemask creates a mask of
bytes preceding the first zero-byte by left shifting ~0ul based on the
position of the first zero byte.

Unfortunately, if the first (top) byte is zero, the output of
prep_zero_mask has only the top bit set, resulting in undefined C
behaviour as we shift left by an amount equal to the width of the type.
As it happens, GCC doesn't manage to spot this through the call to fls(),
but the issue remains if architectures choose to implement their shift
instructions differently.

An example would be arch/arm/ (AArch32), where LSL Rd, Rn, #32 results
in Rd == 0x0, whilst on arch/arm64 (AArch64) LSL Xd, Xn, #64 results in
Xd == Xn.

Rather than check explicitly for the problematic shift, this patch adds
an extra shift by 1, replacing fls with __fls. Since zero_bytemask is
never called with a zero argument (has_zero() is used to check the data
first), we don't need to worry about calling __fls(0), which is
undefined.

Cc: <stable@...r.kernel.org>
Cc: Victor Kamensky <victor.kamensky@...aro.org>
Signed-off-by: Will Deacon <will.deacon@....com>
---

Hi Linus,

Victor reported this with a big-endian arm64 system. This is the
cleanest, most efficient solution I could come up with, but it would be
helpful if you could please confirm my assumption that zero_bytemask(0)
is nonsensical.

Cheers,

Will

 include/asm-generic/word-at-a-time.h | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/include/asm-generic/word-at-a-time.h b/include/asm-generic/word-at-a-time.h
index d3909effd725..d96deb443f18 100644
--- a/include/asm-generic/word-at-a-time.h
+++ b/include/asm-generic/word-at-a-time.h
@@ -50,11 +50,7 @@ static inline bool has_zero(unsigned long val, unsigned long *data, const struct
 }

 #ifndef zero_bytemask
-#ifdef CONFIG_64BIT
-#define zero_bytemask(mask)	(~0ul << fls64(mask))
-#else
-#define zero_bytemask(mask)	(~0ul << fls(mask))
-#endif /* CONFIG_64BIT */
-#endif /* zero_bytemask */
+#define zero_bytemask(mask) (~0ul << __fls(mask) << 1)
+#endif

 #endif /* _ASM_WORD_AT_A_TIME_H */
-- 
1.9.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/