[PATCH 0/2] Two x86 fixes

[PATCH 0/2] Two x86 fixes

[Ammar Faizi]

Hi,

Two fixes for x86 arch.

[PATCH 1/2] x86/delay: Fix the wrong asm constraint in `delay_loop()`

The asm constraint does not reflect that the asm statement can modify
the value of @loops. But the asm statement in delay_loop() does change
the @loops.

If by any chance the compiler inlines this function, it may clobber
random stuff (e.g. local variable, important temporary value in reg,
etc.).

Fortunately, delay_loop() is only called indirectly (so it can't
inline), and then the register it clobbers is %rax (which is by the
nature of the calling convention, it's a caller saved register), so it
didn't yield any bug.

^ That shouldn't be an excuse for using the wrong constraint anyway.

This changes "a" (as an input) to "+a" (as an input and output).

[PATCH 2/2] x86/mce/amd: Fix memory leak when `threshold_create_bank()` fails.

@bp is a local variable, calling mce_threshold_remove_device() when
threshold_create_bank() fails will not free the @bp. Note that
mce_threshold_remove_device() frees the @bp only if it's already
stored in the @threshold_banks per-CPU variable.

At that point, the @threshold_banks per-CPU variable is still NULL,
so the mce_threshold_remove_device() will just be a no-op and the
@bp is leaked.

Fix this by calling kfree() and early returning when we fail.

This bug is introduced by commit 6458de97fc15530b544 ("x86/mce/amd:
Straighten CPU hotplug path") [1].

Link: https://lore.kernel.org/all/20200403161943.1458-6-bp@alien8.de [1]

v2:
  - Fix wrong copy/paste.

Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Ammar Faizi (2):
  x86/delay: Fix the wrong asm constraint in `delay_loop()`
  x86/mce/amd: Fix memory leak when `threshold_create_bank()` fails

 arch/x86/kernel/cpu/mce/amd.c | 9 ++++-----
 arch/x86/lib/delay.c          | 4 ++--
 2 files changed, 6 insertions(+), 7 deletions(-)


base-commit: 7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3
-- 
2.32.0

[PATCH v2 1/2] x86/delay: Fix the wrong asm constraint in `delay_loop()`

[Ammar Faizi]

The asm constraint does not reflect that the asm statement can modify
the value of @loops. But the asm statement in delay_loop() does change
the @loops.

If by any chance the compiler inlines this function, it may clobber
random stuff (e.g. local variable, important temporary value in reg,
etc.).

Fortunately, delay_loop() is only called indirectly (so it can't
inline), and then the register it clobbers is %rax (which is by the
nature of the calling convention, it's a caller saved register), so it
didn't yield any bug.

^ That shouldn't be an excuse for using the wrong constraint anyway.

This changes "a" (as an input) to "+a" (as an input and output).

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Fixes: e01b70ef3eb3080fecc35e15f68cd274c0a48163 ("x86: fix bug in arch/i386/lib/delay.c file, delay_loop function")
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.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)
+		:
 	);
 }
 
-- 
2.32.0

[PATCH v2 2/2] x86/mce/amd: Fix memory leak when `threshold_create_bank()` fails

[Ammar Faizi]

@bp is a local variable, calling mce_threshold_remove_device() when
threshold_create_bank() fails will not free the @bp. Note that
mce_threshold_remove_device() frees the @bp only if it's already
stored in the @threshold_banks per-CPU variable.

At that point, the @threshold_banks per-CPU variable is still NULL,
so the mce_threshold_remove_device() will just be a no-op and the
@bp is leaked.

Fix this by calling kfree() and early returning when we fail.

This bug is introduced by commit 6458de97fc15530b544 ("x86/mce/amd:
Straighten CPU hotplug path") [1].

Link: https://lore.kernel.org/all/20200403161943.1458-6-bp@alien8.de [1]

Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Fixes: 6458de97fc15530b54477c4e2b70af653e8ac3d9 ("x86/mce/amd: Straighten CPU hotplug path")
Cc: stable@vger.kernel.org # v5.8+
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 arch/x86/kernel/cpu/mce/amd.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 9f4b508886dd..75d019dfe8d6 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -1350,15 +1350,14 @@ int mce_threshold_create_device(unsigned int cpu)
 		if (!(this_cpu_read(bank_map) & (1 << bank)))
 			continue;
 		err = threshold_create_bank(bp, cpu, bank);
-		if (err)
-			goto out_err;
+		if (err) {
+			kfree(bp);
+			return err;
+		}
 	}
 	this_cpu_write(threshold_banks, bp);
 
 	if (thresholding_irq_en)
 		mce_threshold_vector = amd_threshold_interrupt;
 	return 0;
-out_err:
-	mce_threshold_remove_device(cpu);
-	return err;
 }
-- 
2.32.0

Re: [PATCH v2 1/2] x86/delay: Fix the wrong asm constraint in `delay_loop()`

[Greg KH]

On Tue, Mar 01, 2022 at 02:32:22PM +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 change
> the @loops.
> 
> If by any chance the compiler inlines this function, it may clobber
> random stuff (e.g. local variable, important temporary value in reg,
> etc.).
> 
> Fortunately, delay_loop() is only called indirectly (so it can't
> inline), and then the register it clobbers is %rax (which is by the
> nature of the calling convention, it's a caller saved register), so it
> didn't yield any bug.
> 
> ^ That shouldn't be an excuse for using the wrong constraint anyway.
> 
> This changes "a" (as an input) to "+a" (as an input and output).
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Fixes: e01b70ef3eb3080fecc35e15f68cd274c0a48163 ("x86: fix bug in arch/i386/lib/delay.c file, delay_loop function")

You only need 12 characters here :)

> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> ---

Why is this one not tagged for stable?

Re: [PATCH v2 1/2] x86/delay: Fix the wrong asm constraint in `delay_loop()`

[Ammar Faizi]

Hi Greg,

On 3/1/22 3:26 PM, Greg KH wrote:
> On Tue, Mar 01, 2022 at 02:32:22PM +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 change
>> the @loops.
>>
>> If by any chance the compiler inlines this function, it may clobber
>> random stuff (e.g. local variable, important temporary value in reg,
>> etc.).
>>
>> Fortunately, delay_loop() is only called indirectly (so it can't
>> inline), and then the register it clobbers is %rax (which is by the
>> nature of the calling convention, it's a caller saved register), so it
>> didn't yield any bug.
>>
>> ^ That shouldn't be an excuse for using the wrong constraint anyway.
>>
>> This changes "a" (as an input) to "+a" (as an input and output).
>>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Borislav Petkov <bp@alien8.de>
>> Cc: Dave Hansen <dave.hansen@linux.intel.com>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Fixes: e01b70ef3eb3080fecc35e15f68cd274c0a48163 ("x86: fix bug in arch/i386/lib/delay.c file, delay_loop function")
> 
> You only need 12 characters here :)

Ah well, that's too verbose. Will fix it in the v4.

>> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
>> ---
> 
> Why is this one not tagged for stable?

As far as I can tell, the compiler will never inline that function,
because despite the function is static, it's assigned to a global
variable and it's called indirectly via a function pointer variable,
so it can't be inline. Therefore, it will always be a function call.

Note that %eax is a call clobbered register w.r.t. System V ABI. As
such, *by luck*, this wrong constraint doesn't yield any bug.

The compiler will not assume the %eax value is the same as before the
function call is done. So the compiler isn't aware that constraint
violation. Not sure if it's worth it for backport.

x86 maintainers, any comment on this?

-- 
Ammar Faizi

Re: [PATCH 0/2] Two x86 fixes

[Ammar Faizi]

On 3/1/22 2:23 PM, Ammar Faizi wrote:
> [PATCH 1/2] x86/delay: Fix the wrong asm constraint in `delay_loop()`
> 
> @bp is a local variable, calling mce_threshold_remove_device() when
> threshold_create_bank() fails will not free the @bp. Note that
> mce_threshold_remove_device() frees the @bp only if it's already
> stored in the @threshold_banks per-CPU variable.
> 
> At that point, the @threshold_banks per-CPU variable is still NULL,
> so the mce_threshold_remove_device() will just be a no-op and the
> @bp is leaked.
> 
> Fix this by calling kfree() and early returning when we fail.
> 
> This bug is introduced by commit 6458de97fc15530b544 ("x86/mce/amd:
> Straighten CPU hotplug path") [1].
> 
> Link: https://lore.kernel.org/all/20200403161943.1458-6-bp@alien8.de [1]

Uhh... Wrong cover letter... Sorry... Re-sending now...

-- 
Ammar Faizi

[PATCH 0/2] Two x86 fixes

[Ammar Faizi]

Hi,

Two fixes for x86 arch.

[PATCH 1/2] x86/delay: Fix the wrong asm constraint in `delay_loop()`

@bp is a local variable, calling mce_threshold_remove_device() when
threshold_create_bank() fails will not free the @bp. Note that
mce_threshold_remove_device() frees the @bp only if it's already
stored in the @threshold_banks per-CPU variable.

At that point, the @threshold_banks per-CPU variable is still NULL,
so the mce_threshold_remove_device() will just be a no-op and the
@bp is leaked.

Fix this by calling kfree() and early returning when we fail.

This bug is introduced by commit 6458de97fc15530b544 ("x86/mce/amd:
Straighten CPU hotplug path") [1].

Link: https://lore.kernel.org/all/20200403161943.1458-6-bp@alien8.de [1]

[PATCH 2/2] x86/mce/amd: Fix memory leak when `threshold_create_bank()` fails.

@bp is a local variable, calling mce_threshold_remove_device() when
threshold_create_bank() fails will not free the @bp. Note that
mce_threshold_remove_device() frees the @bp only if it's already
stored in the @threshold_banks per-CPU variable.

At that point, the @threshold_banks per-CPU variable is still NULL,
so the mce_threshold_remove_device() will just be a no-op and the
@bp is leaked.

Fix this by calling kfree() and early returning when we fail.

Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Ammar Faizi (2):
  x86/delay: Fix the wrong asm constraint in `delay_loop()`
  x86/mce/amd: Fix memory leak when `threshold_create_bank()` fails

 arch/x86/kernel/cpu/mce/amd.c | 9 ++++-----
 arch/x86/lib/delay.c          | 4 ++--
 2 files changed, 6 insertions(+), 7 deletions(-)


base-commit: 7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3
-- 
2.32.0