linux-kernel - RE: [PATCH] ceph: Fix potential undefined behavior in crush

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: <2eddc77bea32f3baa47cfb1bafb4e20edfe00417.camel@ibm.com>
Date: Mon, 22 Sep 2025 17:19:37 +0000
From: Viacheslav Dubeyko <Slava.Dubeyko@....com>
To: "lyican53@...il.com" <lyican53@...il.com>
CC: "ceph-devel@...r.kernel.org" <ceph-devel@...r.kernel.org>,
        "idryomov@...il.com" <idryomov@...il.com>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        Xiubo Li <xiubli@...hat.com>
Subject: RE: [PATCH] ceph: Fix potential undefined behavior in crush_ln() with
 GCC 11.1.0

On Sat, 2025-09-20 at 20:06 +0800, 陈华昭（Lyican） wrote:
> > 2025年9月20日 02:51，Viacheslav Dubeyko <Slava.Dubeyko@....com> 写道：
> > 
> > On Fri, 2025-09-19 at 10:34 +0800, 陈华昭（Lyican） wrote:
> > > > 2025年9月19日 02:07，Viacheslav Dubeyko <Slava.Dubeyko@....com> 写道：
> > > > 
> > 
> > <skipped>
> > I still have the same issue with the new patch. Your patch is trying to modify
> > the line 262. However, we have comments on this line [1]:
> > 
> > 260 /*
> > 261 * figure out number of bits we need to shift and
> > 262 * do it in one step instead of iteratively
> > 263 */
> > 264 if (!(x & 0x18000)) {
> > 265 int bits = __builtin_clz(x & 0x1FFFF) - 16;
> > 266 x <<= bits;
> > 267 iexpon = 15 - bits;
> > 268 }
> > 
> > Thanks,
> > Slava.
> > 
> > [1]
> > https://elixir.bootlin.com/linux/v6.17-rc6/source/net/ceph/crush/mapper.c#L262  
> Hi Slava,
> 
> Thank you for your patience with this patch. I want to clarify the confusion about the line numbering.
> 
> The patch header "@@ -262,7 +262,7 @@" was automatically generated by git format-patch - I did not manually specify line 262. This is how git diff format works: it shows context lines starting from line 262, but the actual code modification is on line 265 where the `__builtin_clz()` call is located (exactly as you referenced in [1]).
> 
> To be absolutely clear:
> - I am NOT trying to modify line 262 (which contains comments)
> - I AM modifying line 265: `int bits = __builtin_clz(x & 0x1FFFF) - 16;`
> - The "@@ -262,7 +262,7 @@" header is git's standard way of providing context
> - Git automatically chooses how many context lines to show and where to start them
> 
> The patch content clearly shows the actual change:
> ```diff
> - int bits = __builtin_clz(x & 0x1FFFF) - 16;
> + int bits = (x & 0x1FFFF) ? __builtin_clz(x & 0x1FFFF) - 16 : 16;
> ```
> 
> This line-by-line diff shows exactly what gets modified - line 265 in the official kernel source.
> 
> Here is the git-generated patch:
> 
> ---
> 
> From ac3a55a6a18761d613971ef6f78fa39e6d7d2172 Mon Sep 17 00:00:00 2001
> From: Huazhao Chen <lyican53@...il.com>
> Date: Sat, 20 Sep 2025 19:42:54 +0800
> Subject: [PATCH] ceph: Fix potential undefined behavior in crush_ln() with GCC
>  11.1.0
> 
> When x & 0x1FFFF equals zero, __builtin_clz() is called with a zero
> argument, which results in undefined behavior. This can happen during
> ceph's consistent hashing calculations and may lead to incorrect
> placement group mappings.
> 
> Fix by checking if the masked value is non-zero before calling
> __builtin_clz(). If the masked value is zero, use the expected result
> of 16 directly.
> 
> Signed-off-by: Huazhao Chen <lyican53@...il.com>
> ---
>  net/ceph/crush/mapper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ceph/crush/mapper.c b/net/ceph/crush/mapper.c
> index 3a5bd1cd1..000f7a633 100644
> --- a/net/ceph/crush/mapper.c
> +++ b/net/ceph/crush/mapper.c
> @@ -262,7 +262,7 @@ static __u64 crush_ln(unsigned int xin)
>   * do it in one step instead of iteratively
>   */
>   if (!(x & 0x18000)) {
> - int bits = __builtin_clz(x & 0x1FFFF) - 16;
> + int bits = (x & 0x1FFFF) ? __builtin_clz(x & 0x1FFFF) - 16 : 16;
>   x <<= bits;
>   iexpon = 15 - bits;
>   }

I completely lost myself in the multiple versions of the patch. Could you please
send one formal patch only that I can successfully apply?

Thanks,
Slava.