linux-kernel - Re: [PATCH v4 2/2] x86/mce/amd: Fix memory leak when `threshold_create

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yh+oyD/5M3TW5ZMM@yaz-ubuntu>
Date:   Wed, 2 Mar 2022 17:26:32 +0000
From:   Yazen Ghannam <yazen.ghannam@....com>
To:     Ammar Faizi <ammarfaizi2@...weeb.org>
Cc:     Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Tony Luck <tony.luck@...el.com>, linux-edac@...r.kernel.org,
        linux-kernel@...r.kernel.org, gwml@...r.gnuweeb.org,
        x86@...nel.org, stable@...r.kernel.org,
        Alviro Iskandar Setiawan <alviro.iskandar@...weeb.org>,
        Jiri Hladky <hladky.jiri@...glemail.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v4 2/2] x86/mce/amd: Fix memory leak when
 `threshold_create_bank()` fails

On Tue, Mar 01, 2022 at 04:46:08PM +0700, Ammar Faizi wrote:
> From: Ammar Faizi <ammarfaizi2@...weeb.org>
> 

Hi Ammar,

...

> diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
> index 9f4b508886dd..a5ef161facd9 100644
> --- a/arch/x86/kernel/cpu/mce/amd.c
> +++ b/arch/x86/kernel/cpu/mce/amd.c
> @@ -1346,19 +1346,23 @@ int mce_threshold_create_device(unsigned int cpu)
>  	if (!bp)
>  		return -ENOMEM;
>  
> +	/*
> +	 * If we fail, mce_threshold_remove_device() will free the @bp
> +	 * via @threshold_banks.
> +	 */
> +	this_cpu_write(threshold_banks, bp);
> +
>  	for (bank = 0; bank < numbanks; ++bank) {
>  		if (!(this_cpu_read(bank_map) & (1 << bank)))
>  			continue;
>  		err = threshold_create_bank(bp, cpu, bank);
> -		if (err)
> -			goto out_err;
> +		if (err) {
> +			mce_threshold_remove_device(cpu);
> +			return err;
> +		}
>  	}
> -	this_cpu_write(threshold_banks, bp);
>

The threshold interrupt handler uses this pointer. I think the goal here is to
set this pointer when the list is fully formed and clear this pointer before
making any changes to the list. Otherwise, the interrupt handler will operate
on incomplete data if an interrupt comes in the middle of these updates.

The changes below should deal with memory leak issue while avoiding a race
with the threshold interrupt. What do you think?

Thanks,
Yazen

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 1940d305db1c..8f3b7859331d 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -1294,10 +1294,22 @@ static void threshold_remove_bank(struct threshold_bank *bank)
 	kfree(bank);
 }
 
+void _mce_threshold_remove_device(struct threshold_bank **bp)
+{
+	unsigned int bank, numbanks = this_cpu_read(mce_num_banks);
+
+	for (bank = 0; bank < numbanks; bank++) {
+		if (bp[bank]) {
+			threshold_remove_bank(bp[bank]);
+			bp[bank] = NULL;
+		}
+	}
+	kfree(bp);
+}
+
 int mce_threshold_remove_device(unsigned int cpu)
 {
 	struct threshold_bank **bp = this_cpu_read(threshold_banks);
-	unsigned int bank, numbanks = this_cpu_read(mce_num_banks);
 
 	if (!bp)
 		return 0;
@@ -1308,13 +1320,7 @@ int mce_threshold_remove_device(unsigned int cpu)
 	 */
 	this_cpu_write(threshold_banks, NULL);
 
-	for (bank = 0; bank < numbanks; bank++) {
-		if (bp[bank]) {
-			threshold_remove_bank(bp[bank]);
-			bp[bank] = NULL;
-		}
-	}
-	kfree(bp);
+	_mce_threshold_remove_device(bp);
 	return 0;
 }
 
@@ -1360,6 +1366,6 @@ int mce_threshold_create_device(unsigned int cpu)
 		mce_threshold_vector = amd_threshold_interrupt;
 	return 0;
 out_err:
-	mce_threshold_remove_device(cpu);
+	_mce_threshold_remove_device(bp);
 	return err;
 }