[PATCH v4] bitops: Fix shift overflow in GENMASK macros

[PATCH v4] bitops: Fix shift overflow in GENMASK macros

[Maxime COQUELIN]

On some 32 bits architectures, including x86, GENMASK(31, 0) returns 0
instead of the expected ~0UL.

This is the same on some 64 bits architectures with GENMASK_ULL(63, 0).

This is due to an overflow in the shift operand, 1 << 32 for GENMASK,
1 << 64 for GENMASK_ULL.

Fixes: 10ef6b0dffe404bcc54e94cb2ca1a5b18445a66b
Cc: <stable@vger.kernel.org> #v3.13+
Reported-by: Eric Paire <eric.paire@st.com>
Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
---
 include/linux/bitops.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index be5fd38..5d858e0 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -18,8 +18,11 @@
  * position @h. For example
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
-#define GENMASK(h, l)		(((U32_C(1) << ((h) - (l) + 1)) - 1) << (l))
-#define GENMASK_ULL(h, l)	(((U64_C(1) << ((h) - (l) + 1)) - 1) << (l))
+#define GENMASK(h, l) \
+	(((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
+
+#define GENMASK_ULL(h, l) \
+	(((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
 
 extern unsigned int __sw_hweight8(unsigned int w);
 extern unsigned int __sw_hweight16(unsigned int w);
-- 
1.9.1

Re: [PATCH v4] bitops: Fix shift overflow in GENMASK macros

[Andrew Morton]

On Thu,  6 Nov 2014 10:54:19 +0100 Maxime COQUELIN <maxime.coquelin@st.com> wrote:

> On some 32 bits architectures, including x86, GENMASK(31, 0) returns 0
> instead of the expected ~0UL.
> 
> This is the same on some 64 bits architectures with GENMASK_ULL(63, 0).
> 
> This is due to an overflow in the shift operand, 1 << 32 for GENMASK,
> 1 << 64 for GENMASK_ULL.
> 
> Fixes: 10ef6b0dffe404bcc54e94cb2ca1a5b18445a66b
> Cc: <stable@vger.kernel.org> #v3.13+
> Reported-by: Eric Paire <eric.paire@st.com>
> Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>

Why cc:stable?  Does this bug cause some observed kernel misbehaviour? 
If so, please fully describe that in the changelog.  This will help
people to determine whether this patch might fix a bug they're
observing, and will help them to decide whether they should backport
this patch into their kernels.

I'm assuming that Peter will be merging this patch.

Re: [PATCH v4] bitops: Fix shift overflow in GENMASK macros

[Maxime Coquelin]

On 11/13/2014 11:09 PM, Andrew Morton wrote:
> On Thu,  6 Nov 2014 10:54:19 +0100 Maxime COQUELIN <maxime.coquelin@st.com> wrote:
>
>> On some 32 bits architectures, including x86, GENMASK(31, 0) returns 0
>> instead of the expected ~0UL.
>>
>> This is the same on some 64 bits architectures with GENMASK_ULL(63, 0).
>>
>> This is due to an overflow in the shift operand, 1 << 32 for GENMASK,
>> 1 << 64 for GENMASK_ULL.
>>
>> Fixes: 10ef6b0dffe404bcc54e94cb2ca1a5b18445a66b
>> Cc: <stable@vger.kernel.org> #v3.13+
>> Reported-by: Eric Paire <eric.paire@st.com>
>> Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
> Why cc:stable?  Does this bug cause some observed kernel misbehaviour?
> If so, please fully describe that in the changelog.  This will help
> people to determine whether this patch might fix a bug they're
> observing, and will help them to decide whether they should backport
> this patch into their kernels.
We encountered some misbehavior on not upstreamed code.

Looking at all GENMASK and GENMASK_ULL occurences in v3.18-rc4,
I (only) found one possible candidate in drivers/spi/spi_xtensa-xtfpga.c:

static u32 xtfpga_spi_txrx_word(struct spi_device *spi, unsigned nsecs,
                 u32 v, u8 bits)
{
     struct xtfpga_spi *xspi = spi_master_get_devdata(spi->master);

     xspi->data = (xspi->data << bits) | (v & GENMASK(bits - 1, 0));
...
}

Max F., can xtfpga_spi_txrx_word() be called with "bits" = 32?
If yes, then GENMASK(bits - 1, 0) result would be unpredictable on some 
architectures.
I don't know if Xtensa architecture is impacted though.

But even if Xtensa SPI driver is not impacted,
maybe future fixes that will be integrated into stable releases will use 
GENMASK(),
and so could possibly be impacted by the bug.

Andrew, with this information, do you think we should take this patch in 
stable branches?

>
> I'm assuming that Peter will be merging this patch.

Yes, Peter already added this patch in his tree.
>
Kind regards,
Maxime

Re: [PATCH v4] bitops: Fix shift overflow in GENMASK macros

[Max Filippov]

Hi Maxime,

On Fri, Nov 14, 2014 at 5:07 PM, Maxime Coquelin <maxime.coquelin@st.com> wrote:
> Looking at all GENMASK and GENMASK_ULL occurences in v3.18-rc4,
> I (only) found one possible candidate in drivers/spi/spi_xtensa-xtfpga.c:
>
> static u32 xtfpga_spi_txrx_word(struct spi_device *spi, unsigned nsecs,
>                 u32 v, u8 bits)
> {
>     struct xtfpga_spi *xspi = spi_master_get_devdata(spi->master);
>
>     xspi->data = (xspi->data << bits) | (v & GENMASK(bits - 1, 0));
> ...
> }
>
> Max F., can xtfpga_spi_txrx_word() be called with "bits" = 32?

No, bits should never be greater than 16, because spi_master sets its
bits_per_word_mask to SPI_BPW_RANGE_MASK(1, 16).

-- 
Thanks.
-- Max

[tip:core/urgent] bitops: Fix shift overflow in GENMASK macros

[tip-bot for Maxime COQUELIN]

Commit-ID:  00b4d9a14125f1e51874def2b9de6092e007412d
Gitweb:     http://git.kernel.org/tip/00b4d9a14125f1e51874def2b9de6092e007412d
Author:     Maxime COQUELIN <maxime.coquelin@st.com>
AuthorDate: Thu, 6 Nov 2014 10:54:19 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sun, 16 Nov 2014 09:55:39 +0100

bitops: Fix shift overflow in GENMASK macros

On some 32 bits architectures, including x86, GENMASK(31, 0) returns 0
instead of the expected ~0UL.

This is the same on some 64 bits architectures with GENMASK_ULL(63, 0).

This is due to an overflow in the shift operand, 1 << 32 for GENMASK,
1 << 64 for GENMASK_ULL.

Reported-by: Eric Paire <eric.paire@st.com>
Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: <stable@vger.kernel.org> # v3.13+
Cc: linux@rasmusvillemoes.dk
Cc: gong.chen@linux.intel.com
Cc: John Sullivan <jsrhbz@kanargh.force9.co.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Theodore Ts'o <tytso@mit.edu>
Fixes: 10ef6b0dffe4 ("bitops: Introduce a more generic BITMASK macro")
Link: http://lkml.kernel.org/r/1415267659-10563-1-git-send-email-maxime.coquelin@st.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/bitops.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index be5fd38..5d858e0 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -18,8 +18,11 @@
  * position @h. For example
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
-#define GENMASK(h, l)		(((U32_C(1) << ((h) - (l) + 1)) - 1) << (l))
-#define GENMASK_ULL(h, l)	(((U64_C(1) << ((h) - (l) + 1)) - 1) << (l))
+#define GENMASK(h, l) \
+	(((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
+
+#define GENMASK_ULL(h, l) \
+	(((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
 
 extern unsigned int __sw_hweight8(unsigned int w);
 extern unsigned int __sw_hweight16(unsigned int w);