linux-kernel - Re: [PATCH next 10/14] bits: Fix assmebler expansions of GENMASK_Uxx() and BIT

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: <20260208222724.6e2dd07e@pumpkin>
Date: Sun, 8 Feb 2026 22:27:24 +0000
From: David Laight <david.laight.linux@...il.com>
To: Yury Norov <ynorov@...dia.com>
Cc: Nathan Chancellor <nathan@...nel.org>, Greg Kroah-Hartman
 <gregkh@...uxfoundation.org>, Thomas Gleixner <tglx@...utronix.de>, Peter
 Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>, Mathieu
 Desnoyers <mathieu.desnoyers@...icios.com>, Arnd Bergmann <arnd@...db.de>,
 linux-arch@...r.kernel.org, linux-kernel@...r.kernel.org, Yury Norov
 <yury.norov@...il.com>, Lucas De Marchi <lucas.demarchi@...el.com>, Jani
 Nikula <jani.nikula@...el.com>, Vincent Mailhol
 <mailhol.vincent@...adoo.fr>, Andy Shevchenko
 <andriy.shevchenko@...ux.intel.com>, Kees Cook <keescook@...omium.org>,
 Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH next 10/14] bits: Fix assmebler expansions of
 GENMASK_Uxx() and BIT_Uxx()

On Sun, 8 Feb 2026 16:20:45 -0500
Yury Norov <ynorov@...dia.com> wrote:

> On Sun, Feb 08, 2026 at 11:42:14AM +0000, David Laight wrote:
> > On Sat, 7 Feb 2026 22:31:34 -0500
> > Yury Norov <ynorov@...dia.com> wrote:
> >   
> > > On Wed, Jan 21, 2026 at 02:57:27PM +0000, david.laight.linux@...il.com wrote:  
> > > > From: David Laight <david.laight.linux@...il.com>
> > > > 
> > > > The assembler only supports one type of signed integers, so expressions
> > > > using BITS_PER_LONG (etc) cannot be guaranteed to be correct.
> > > > 
> > > > Use ((2 << (h)) - (1 << (l))) for all assembler GENMASK() expansions and
> > > > add definitions of BIT_Uxx() as (1 << (nr)).
> > > > 
> > > > Note that 64bit results are (probably) only correct for 64bit builds
> > > > and 128bits results will never be valid.    
> > > 
> > > And this important note will sink in git history.  
> > 
> > At least it isn't only in the email archives.
> > I can put it in a comment.
> >   
> > > > Signed-off-by: David Laight <david.laight.linux@...il.com>    
> > > 
> > > This has been discussed in details when those GENMASK_Uxx() were
> > > introduced. Assembler doesn't support C types, and can't provide any
> > > guarantees. It may only confuse readers when they see something like
> > > GENMASK_U8() in the assembler code, and there's nothing on behalf of
> > > that declaration to enforce the limitation.  
> > 
> > It won't be in asm code, the asm code will be expanding a constant
> > from a C header file.  
> 
> It can be included and preprocessed well in any .S file:
> 
> #define GENMASK_TYPE(t, h,l) ((2 << (h)) - (1 << (l)))
> #define GENMASK(h, l) GENMASK_TYPE(unsigned long, (h), (l))
> 
>     .section .rodata
> fmt:
>     .string "GENMASK(63,60) = 0x%016llx\n"
> 
>     .text
>     .globl main
>     .type main, @function
> 
> main:
>     push    %rbp
>     mov     %rsp, %rbp
> 
>     lea     fmt(%rip), %rdi
>     mov     $GENMASK(63,60), %rsi
>     xor     %rax, %rax
>     call    printf@PLT
> 
>     mov     $0, %eax
>     pop     %rbp
>     ret
> 
> In C this doesn't work at all as it throws overflow. It doesn't even
> work in asm volatile section.

Indeed - that I'm not trying to use that expression in C.
Although it will work for non-constants.
For constants you'd have to use ((1 << hi) - 1) * 2) + 1.
While I'm pretty sure the compiler will convert it to the former, both
generate worse code in some corner cases.

The issue at the moment is that you get these definitions from uapi/linux/bits.h

#define __GENMASK(h, l) (((~_UL(0)) << (l)) & (~_UL(0) >> (__BITS_PER_LONG - 1 - (h))))
#define __GENMASK_ULL(h, l) (((~_ULL(0)) << (l)) & (~_ULL(0) >> (__BITS_PER_LONG_LONG - 1 - (h))))

For .S files both _UL() and _ULL() are null, so these are:

#define __GENMASK(h, l) (((~0) << (l)) & (~0 >> (__BITS_PER_LONG - 1 - (h))))
#define __GENMASK_ULL(h, l) (((~0) << (l)) & (~0 >> (__BITS_PER_LONG_LONG - 1 - (h))))

Which makes them identical except for the two constants.
On 32bit builds the two constants are different which means that while __GENMASK(5, 2)
and __GENMASK_ULL(5,2) should have the same value they may not (depending exactly
how the assembler evaluates constant expressions).
Assuming that either __BITS_PER_LONG or __BITS_PER_LONG_LONG has anything to do
with the number of bits in the assembler's expression evaluator doesn't seem
right at all.

	David

>  
> > > That's why we didn't add fake C types support in the assembler. Unless
> > > we find a way to enforce C types capacity in assembler(s), let's keep
> > > those macros C-only.  
> > 
> > But GENMASK_ULL() was already there and would generate invalid values
> > (for small values) on 32bit.  
> 
> You continuously repeat that GENMASK_ULL() generates wrong values, but
> never submitted a fix.
> 
> Anyways, if you think GENMASK_ULL() is not needed in assembler, it's
> even harder to advocate fixed-type flavors.
> 
> > The only reason for defining these for assembler is so that .h files
> > that use the definitions can be used in .S files.
> > As soon as any of the BIT_Unn() get used the asm code is likely to
> > try to expand them.  
> 
> The only reason for fixed-type GENMASK() and BIT() is strict
> parameters checking. This is not possible in assembler.
> 
> Thanks,
> Yury