linux-kernel - Re: [PATCH] x86/math64: handle #DE in mul_u64_u64_div

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] [day] [month] [year] [list]

Message-ID: <20250721192053.58843751@pumpkin>
Date: Mon, 21 Jul 2025 19:20:53 +0100
From: David Laight <david.laight.linux@...il.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Ingo Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>,
 Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>, Dave
 Hansen <dave.hansen@...ux.intel.com>, "H. Peter Anvin" <hpa@...or.com>,
 "Li,Rongqing" <lirongqing@...du.com>, Steven Rostedt <rostedt@...dmis.org>,
 linux-kernel@...r.kernel.org, x86@...nel.org
Subject: Re: [PATCH] x86/math64: handle #DE in mul_u64_u64_div_u64()

On Mon, 21 Jul 2025 15:04:22 +0200
Oleg Nesterov <oleg@...hat.com> wrote:

> Change mul_u64_u64_div_u64() to return ULONG_MAX if the result doesn't
> fit u64, this matches the generic implementation in lib/math/div64.c.

Not quite, the generic version is likely to trap on divide by zero.
I think it would be better to always trap (eg BUG_ON(!div)).

Alternatively have a function that returns (ok, overflow, infinity, NaN) to
its caller - passing the buck.
Although, perhaps, in a way that the the callers can ignore.
The trouble there is that (an ignored) ~(u64)0 is likely to cause another
arithmetic overflow with even more consequences.

So I'm not at all sure what it should look like or whether 0 is a better
error return (esp for div == 0).

Even for the scheduler that recently trapped here, the 64bit sum was
about the wrap and the very small value returned then almost certainly
wouldn't have been handled correctly.
(So that code needs fixing anyway to avoid the overflow.) 

> 
> Reported-by: "Li,Rongqing" <lirongqing@...du.com>
> Link: https://lore.kernel.org/all/78a0d7bb20504c0884d474868eccd858@baidu.com/
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>
> ---
>  arch/x86/include/asm/div64.h | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/div64.h b/arch/x86/include/asm/div64.h
> index 9931e4c7d73f..dfd25cfd1c33 100644
> --- a/arch/x86/include/asm/div64.h
> +++ b/arch/x86/include/asm/div64.h
> @@ -79,20 +79,27 @@ static inline u64 mul_u32_u32(u32 a, u32 b)
>  
>  #else
>  # include <asm-generic/div64.h>
> +# include <asm/asm.h>
> +# include <asm/bug.h>
>  
>  /*
> - * Will generate an #DE when the result doesn't fit u64, could fix with an
> - * __ex_table[] entry when it becomes an issue.
> + * Returns ULONG_MAX when the result doesn't fit u64.
>   */
>  static inline u64 mul_u64_u64_div_u64(u64 a, u64 mul, u64 div)
>  {
> +	int ok = 0;
>  	u64 q;
>  
> -	asm ("mulq %2; divq %3" : "=a" (q)
> -				: "a" (a), "rm" (mul), "rm" (div)
> -				: "rdx");
> +	asm ("mulq %3; 1: divq %4; movl $1,%1; 2:\n"

The "movl $1,%1" is a 5 byte instruction.
Better to use either 'incl' or get the constraints right for 'movb'
(both 2 bytes).

> +		_ASM_EXTABLE(1b, 2b)
> +		: "=a" (q), "+r" (ok)
> +		: "a" (a), "rm" (mul), "rm" (div)
> +		: "rdx");
>  
> -	return q;
> +	if (ok)
> +		return q;
> +	WARN_ON_ONCE(!div);

I think you need to WARN for overflow as well as divide by zero.

	David

> +	return ~(u64)0;
>  }
>  #define mul_u64_u64_div_u64 mul_u64_u64_div_u64
>