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.
Fix this by changing the constraint from "a" (as an input) to "+a" (as
an input and output).
Cc: David Laight <[email protected]>
Cc: Jiri Hladky <[email protected]>
Cc: Alviro Iskandar Setiawan <[email protected]>
Fixes: e01b70ef3eb3 ("x86: fix bug in arch/i386/lib/delay.c file, delay_loop function")
Signed-off-by: Ammar Faizi <[email protected]>
---
v6:
- Remove unnecessary Cc tags.
- Update commit message, emphasize the danger when the
compiler decides to inline the function.
- Fix the Fixes tag sha1.
- Remove stable Cc.
---
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)
+ :
);
}
--
Ammar Faizi
On 3/29/22 03:47, 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.
>
> Fix this by changing the constraint from "a" (as an input) to "+a" (as
> an input and output).
Was this found by inspection or was it causing real-world problems?
On 4/3/22 11:57 PM, Thomas Gleixner wrote:
> On Tue, Mar 29 2022 at 17:47, 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.
>>
>> Fix this by changing the constraint from "a" (as an input) to "+a" (as
>> an input and output).
>
> This analysis is plain wrong. The assembly code operates on a register
> and not on memory:
> asm volatile(
> " test %0,%0 \n"
> " jz 3f \n"
> " jmp 1f \n"
>
> ".align 16 \n"
> "1: jmp 2f \n"
>
> ".align 16 \n"
> "2: dec %0 \n"
> " jnz 2b \n"
> "3: dec %0 \n"
>
> : /* we don't need output */
> ----> :"a" (loops)
>
> This tells the compiler to use [RE]AX and initialize it from the
> variable 'loops'. It's never written back because all '%0' in the above
> assembly are substituted with [RE]AX. This also tells the compiler that
> the inline assembly clobbers [RE]AX and that's all it needs to know.
Hi Thomas,
Thanks for taking a look. I doubt about your sentence "This also tells
the compiler that the inline assembly clobbers [RE]AX".
How come it tells the compiler that the inline ASM clobbers [RE]AX?
That's an input constraint. Doesn't that mean it is read-only for the
ASM statement? That means the compiler is allowed to assume [RE]AX doesn't
change inside the ASM statement.
Those `dec`s do really change the [RE]AX. Please review this again.
Thanks!
--
Ammar Faizi
On 4/2/22 12:42 AM, Dave Hansen wrote:
> Was this found by inspection or was it causing real-world problems?
It was found by inspection, no real-world problems found so far.
--
Ammar Faizi
On Sun, Apr 03 2022 at 18:57, Thomas Gleixner wrote:
> On Tue, Mar 29 2022 at 17:47, 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.
Ignore me, I misread this part of the explanation.
Thanks,
tglx
On Tue, Mar 29 2022 at 17:47, 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.
>
> Fix this by changing the constraint from "a" (as an input) to "+a" (as
> an input and output).
This analysis is plain wrong. The assembly code operates on a register
and not on memory:
asm volatile(
" test %0,%0 \n"
" jz 3f \n"
" jmp 1f \n"
".align 16 \n"
"1: jmp 2f \n"
".align 16 \n"
"2: dec %0 \n"
" jnz 2b \n"
"3: dec %0 \n"
: /* we don't need output */
----> :"a" (loops)
This tells the compiler to use [RE]AX and initialize it from the
variable 'loops'. It's never written back because all '%0' in the above
assembly are substituted with [RE]AX. This also tells the compiler that
the inline assembly clobbers [RE]AX and that's all it needs to know.
Nothing to fix here, whether the code is inlined or not.
Thanks,
tglx
The following commit has been merged into the x86/misc branch of tip:
Commit-ID: b86eb74098a92afd789da02699b4b0dd3f73b889
Gitweb: https://git.kernel.org/tip/b86eb74098a92afd789da02699b4b0dd3f73b889
Author: Ammar Faizi <[email protected]>
AuthorDate: Tue, 29 Mar 2022 17:47:04 +07:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Tue, 05 Apr 2022 21:21:57 +02:00
x86/delay: Fix the wrong asm constraint in delay_loop()
The asm constraint does not reflect the fact that the asm statement can
modify the value of the local variable loops. Which it does.
Specifying 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.
Change the constraint to "+a" to denote that the first argument is an
input and an output argument.
[ bp: Fix typo, massage commit message. ]
Fixes: e01b70ef3eb3 ("x86: fix bug in arch/i386/lib/delay.c file, delay_loop function")
Signed-off-by: Ammar Faizi <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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 65d15df..0e65d00 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)
+ :
);
}