linux-kernel - Re: [PATCH] linux/bits.h: Squash unsigned comparison warning for GENMASK

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: <Z6KF8UBsQ4NmR9KS@thinkpad>
Date: Tue, 4 Feb 2025 16:26:20 -0500
From: Yury Norov <yury.norov@...il.com>
To: Yazen Ghannam <yazen.ghannam@....com>
Cc: Rasmus Villemoes <linux@...musvillemoes.dk>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] linux/bits.h: Squash unsigned comparison warning for
 GENMASK

On Tue, Feb 04, 2025 at 05:13:16PM +0000, Yazen Ghannam wrote:
> Cast inputs to 'long' to avoid the following 'type-limits' warning:
>   warning: comparison of unsigned expression in ‘< 0’ is always false
> 
> The 'long' type can hold +/- 2G which far exceeds valid inputs for the
> GENMASK helpers (current max use is 128 bits).
> 
> Idea is similar to implementation in __is_nonneg().
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@....com>
> ---
> Note to maintainers:
> I found some previous discussions on this topic in the mailing list
> archives. The upstream code has changed a bit since then, and this
> proposed solution seems fairly simple when based on the latest code.
> 
> I figured I'd look at something outside my normal focus areas. I
> apologize for the noise if this solution is too naive or incomplete.
> 
> Thanks!

Hi Yazen,

Wtype-limits is enabled in W=2 only, see scripts/Makefile.extrawarn.
We normally shouldn't see this type of warnings even when compiling
with W=1, at all. 

We have quite a lot callers in kernel already that do GENMASK(xxx, 0)

  yury:linux$ git grep GENMASK | grep 0\) | wc -l
  13788

And nobody complained so far. 

Still, I tried to compile a small userspace app that calls
__GENMASK(10,0), and found no warnings with Wall, Wextra and
Wtype-limits enabled.

Can you share more about your compiler, compilation command and config?

> ---
>  include/linux/bits.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 61a75d3f294b..318346f2a5a6 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -21,7 +21,7 @@
>  #if !defined(__ASSEMBLY__)
>  #include <linux/build_bug.h>
>  #include <linux/compiler.h>
> -#define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
> +#define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((long)(l) > (long)(h)))

This implies that GENMASK() is OK with signed input, so negative too.
It's not true. For me typecasting to a signed type just to shut the
compiler is a bad idea

Thanks,
Yury

>  #else
>  /*
>   * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> 
> ---
> base-commit: 0de63bb7d91975e73338300a57c54b93d3cc151c
> change-id: 20250204-fix-genmask-warn-489a6480779f