Re: [PATCH v2] ksmbd: vfs_cache: avoid integer overflow in inode_hash()

[David Laight <david.laight.linux@gmail.com>]

On Sat, 15 Nov 2025 23:48:36 +0900
Qianchang Zhao <pioooooooooip@gmail.com> wrote:

> inode_hash() currently mixes a hash value with the super_block pointer
> using an unbounded multiplication:
> 
>     tmp = (hashval * (unsigned long)sb) ^
>           (GOLDEN_RATIO_PRIME + hashval) / L1_CACHE_BYTES;
> 
> On 64-bit kernels this multiplication can overflow and wrap in unsigned
> long arithmetic.

The same happens on 32bits.

> While this is not a memory-safety issue, it is an
> unbounded integer operation and weakens the mixing properties of the
> hash.

Are you sure, I'd have thought all the 'carry' operations in the multiply
would make it better.

> Replace the pointer*hash multiply with hash_long() over a mixed value
> (hashval ^ (unsigned long)sb) and keep the existing shift/mask. This
> removes the overflow source and reuses the standard hash helper already
> used in other kernel code.
> 
> This is an integer wraparound / robustness issue (CWE-190/CWE-407),
> not a memory-safety bug.

It isn't really an integer wraparound bug either.
The fact that only the low bits of the product are used shouldn't matter
at all.

OTOH it might be worth sifting 'sb' right some bits - quite a few of
its low bits are zero and they end up as zeros in the product.

	David

> 
> Reported-by: Qianchang Zhao <pioooooooooip@gmail.com>
> Reported-by: Zhitong Liu <liuzhitong1993@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Qianchang Zhao <pioooooooooip@gmail.com>
> ---
>  fs/smb/server/vfs_cache.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/smb/server/vfs_cache.c b/fs/smb/server/vfs_cache.c
> index dfed6fce8..a62ea5aae 100644
> --- a/fs/smb/server/vfs_cache.c
> +++ b/fs/smb/server/vfs_cache.c
> @@ -10,6 +10,7 @@
>  #include <linux/vmalloc.h>
>  #include <linux/kthread.h>
>  #include <linux/freezer.h>
> +#include <linux/hash.h>
>  
>  #include "glob.h"
>  #include "vfs_cache.h"
> @@ -65,12 +66,8 @@ static void fd_limit_close(void)
>  
>  static unsigned long inode_hash(struct super_block *sb, unsigned long hashval)
>  {
> -	unsigned long tmp;
> -
> -	tmp = (hashval * (unsigned long)sb) ^ (GOLDEN_RATIO_PRIME + hashval) /
> -		L1_CACHE_BYTES;
> -	tmp = tmp ^ ((tmp ^ GOLDEN_RATIO_PRIME) >> inode_hash_shift);
> -	return tmp & inode_hash_mask;
> +	unsigned long mixed = hashval ^ (unsigned long)sb;
> +	return hash_long(mixed, inode_hash_shift) & inode_hash_mask;
>  }
>  
>  static struct ksmbd_inode *__ksmbd_inode_lookup(struct dentry *de)