linux-kernel - Re: [PATCH v5 1/2] x86/delay: Fix the wrong asm constraint in `delay

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]

Date:   Sun, 27 Mar 2022 23:38:43 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Ammar Faizi <ammarfaizi2@...weeb.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Alviro Iskandar Setiawan <alviro.iskandar@...il.com>,
        Alviro Iskandar Setiawan <alviro.iskandar@...weeb.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
        Tony Luck <tony.luck@...el.com>,
        Yazen Ghannam <yazen.ghannam@....com>,
        linux-edac@...r.kernel.org, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org, gwml@...r.gnuweeb.org, x86@...nel.org,
        David Laight <David.Laight@...lab.com>,
        Jiri Hladky <hladky.jiri@...glemail.com>
Subject: Re: [PATCH v5 1/2] x86/delay: Fix the wrong asm constraint in
 `delay_loop()`

On Thu, Mar 10, 2022 at 08:53:05AM +0700, Ammar Faizi wrote:
> The asm constraint does not reflect that the asm statement can modify
> the value of @loops. But the asm statement in delay_loop() does modify
> the @loops.
> 
> Specifiying the wrong constraint may lead to undefined behavior, it may
> clobber random stuff (e.g. local variable, important temporary value in
> regs, etc.).

This is especially dangerous when the compiler decides to inline the
function and since it doesn't know that the value gets modified, it
might decide to use it from a register directly without reloading it.

Add that to the commit message pls.

> Fix this by changing the constraint from "a" (as an input) to "+a" (as
> an input and output).
> 
> Cc: David Laight <David.Laight@...LAB.COM>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Borislav Petkov <bp@...en8.de>
> Cc: Dave Hansen <dave.hansen@...ux.intel.com>
> Cc: "H. Peter Anvin" <hpa@...or.com>
> Cc: Jiri Hladky <hladky.jiri@...glemail.com>

All those Ccs in the commit message are not really needed -
get_maintainers.pl gives the correct list already.

> Cc: stable@...r.kernel.org # v2.6.27+

I don't see the need for the stable Cc. Or do you have a case where
a corruption really does happen?

> Fixes: e01b70ef3eb ("x86: fix bug in arch/i386/lib/delay.c file, delay_loop function")

Commit sha1 (e01b70ef3eb) needs to be at least 12 chars long:

e01b70ef3eb3 ("x86: fix bug in arch/i386/lib/delay.c file, delay_loop function")

This is best fixed by doing:

[core]
        abbrev = 12

in your .git/config

> Signed-off-by: Ammar Faizi <ammarfaizi2@...weeb.org>
> ---
>  arch/x86/lib/delay.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/lib/delay.c b/arch/x86/lib/delay.c
> index 65d15df6212d..0e65d00e2339 100644
> --- a/arch/x86/lib/delay.c
> +++ b/arch/x86/lib/delay.c
> @@ -54,8 +54,8 @@ static void delay_loop(u64 __loops)
>  		"	jnz 2b		\n"
>  		"3:	dec %0		\n"
>  
> -		: /* we don't need output */
> -		:"a" (loops)
> +		: "+a" (loops)
> +		:
>  	);

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette