[PATCH] x86/MCE/AMD: Fix use after free in error handling

[PATCH] x86/MCE/AMD: Fix use after free in error handling

[Dan Carpenter]

If an error occurs in the threshold_create_bank() function then the real
clean up is supposed to happen in mce_threshold_remove_device().  The
problem here is that if allocate_threshold_blocks() fails, then we
kfree(b) before returning.  Then we use "b" again in
mce_threshold_remove_device() when we do the rest of the clean up work.

Fixes: 019f34fccfd5 ("x86, MCE, AMD: Move shared bank to node descriptor")
Reported-by: Saar Amar <Saar.Amar@microsoft.com>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I believe Saar found this through reading the code and there is no test
case.  I have don't have a way to test it.

 arch/x86/kernel/cpu/mce/amd.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index b3a50d962851..ff01b789066e 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -1342,7 +1342,8 @@ static int threshold_create_bank(unsigned int cpu, unsigned int bank)
 	b->kobj = kobject_create_and_add(name, &dev->kobj);
 	if (!b->kobj) {
 		err = -EINVAL;
-		goto out_free;
+		kfree(b);
+		goto out;
 	}
 
 	per_cpu(threshold_banks, cpu)[bank] = b;
@@ -1358,12 +1359,6 @@ static int threshold_create_bank(unsigned int cpu, unsigned int bank)
 	}
 
 	err = allocate_threshold_blocks(cpu, bank, 0, msr_ops.misc(bank));
-	if (!err)
-		goto out;
-
- out_free:
-	kfree(b);
-
  out:
 	return err;
 }
-- 
2.11.0

RE: [EXTERNAL] [PATCH] x86/MCE/AMD: Fix use after free in error handling

[Saar Amar]

That's correct - I found this issue through reading and audit the code statically. Thanks for helping out Dan!

-----Original Message-----
From: Dan Carpenter <dan.carpenter@oracle.com> 
Sent: Tuesday, January 28, 2020 4:10 PM
To: Tony Luck <tony.luck@intel.com>; Borislav Petkov <bp@alien8.de>
Cc: Thomas Gleixner <tglx@linutronix.de>; Ingo Molnar <mingo@redhat.com>; H. Peter Anvin <hpa@zytor.com>; x86@kernel.org; linux-edac@vger.kernel.org; Saar Amar <Saar.Amar@microsoft.com>; security@kernel.org
Subject: [EXTERNAL] [PATCH] x86/MCE/AMD: Fix use after free in error handling

If an error occurs in the threshold_create_bank() function then the real clean up is supposed to happen in mce_threshold_remove_device().  The problem here is that if allocate_threshold_blocks() fails, then we
kfree(b) before returning.  Then we use "b" again in
mce_threshold_remove_device() when we do the rest of the clean up work.

Fixes: 019f34fccfd5 ("x86, MCE, AMD: Move shared bank to node descriptor")
Reported-by: Saar Amar <Saar.Amar@microsoft.com>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I believe Saar found this through reading the code and there is no test case.  I have don't have a way to test it.

 arch/x86/kernel/cpu/mce/amd.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c index b3a50d962851..ff01b789066e 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -1342,7 +1342,8 @@ static int threshold_create_bank(unsigned int cpu, unsigned int bank)
 	b->kobj = kobject_create_and_add(name, &dev->kobj);
 	if (!b->kobj) {
 		err = -EINVAL;
-		goto out_free;
+		kfree(b);
+		goto out;
 	}
 
 	per_cpu(threshold_banks, cpu)[bank] = b; @@ -1358,12 +1359,6 @@ static int threshold_create_bank(unsigned int cpu, unsigned int bank)
 	}
 
 	err = allocate_threshold_blocks(cpu, bank, 0, msr_ops.misc(bank));
-	if (!err)
-		goto out;
-
- out_free:
-	kfree(b);
-
  out:
 	return err;
 }
--
2.11.0

RE: [EXTERNAL] [PATCH] x86/MCE/AMD: Fix use after free in error handling

[Saar Amar]

Actually, if we are at it - Dan, given the fact there is an actual use (a dereference) for that pointer after the free, shouldn't we assign a CVE for it?

-----Original Message-----
From: Dan Carpenter <dan.carpenter@oracle.com> 
Sent: Tuesday, January 28, 2020 4:10 PM
To: Tony Luck <tony.luck@intel.com>; Borislav Petkov <bp@alien8.de>
Cc: Thomas Gleixner <tglx@linutronix.de>; Ingo Molnar <mingo@redhat.com>; H. Peter Anvin <hpa@zytor.com>; x86@kernel.org; linux-edac@vger.kernel.org; Saar Amar <Saar.Amar@microsoft.com>; security@kernel.org
Subject: [EXTERNAL] [PATCH] x86/MCE/AMD: Fix use after free in error handling

If an error occurs in the threshold_create_bank() function then the real clean up is supposed to happen in mce_threshold_remove_device().  The problem here is that if allocate_threshold_blocks() fails, then we
kfree(b) before returning.  Then we use "b" again in
mce_threshold_remove_device() when we do the rest of the clean up work.

Fixes: 019f34fccfd5 ("x86, MCE, AMD: Move shared bank to node descriptor")
Reported-by: Saar Amar <Saar.Amar@microsoft.com>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I believe Saar found this through reading the code and there is no test case.  I have don't have a way to test it.

 arch/x86/kernel/cpu/mce/amd.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c index b3a50d962851..ff01b789066e 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -1342,7 +1342,8 @@ static int threshold_create_bank(unsigned int cpu, unsigned int bank)
 	b->kobj = kobject_create_and_add(name, &dev->kobj);
 	if (!b->kobj) {
 		err = -EINVAL;
-		goto out_free;
+		kfree(b);
+		goto out;
 	}
 
 	per_cpu(threshold_banks, cpu)[bank] = b; @@ -1358,12 +1359,6 @@ static int threshold_create_bank(unsigned int cpu, unsigned int bank)
 	}
 
 	err = allocate_threshold_blocks(cpu, bank, 0, msr_ops.misc(bank));
-	if (!err)
-		goto out;
-
- out_free:
-	kfree(b);
-
  out:
 	return err;
 }
--
2.11.0

Re: [EXTERNAL] [PATCH] x86/MCE/AMD: Fix use after free in error handling

[Dan Carpenter]

On Tue, Jan 28, 2020 at 03:59:08PM +0000, Saar Amar wrote:
> Actually, if we are at it - Dan, given the fact there is an actual use
> (a dereference) for that pointer after the free, shouldn't we assign a
> CVE for it?
> 

We don't give CVEs, you'd have to contact someone else.

I don't think this has a security impact because you already have to
be root to trigger it.

regards,
dan carpenter

Re: [PATCH] x86/MCE/AMD: Fix use after free in error handling

[Borislav Petkov]

On Tue, Jan 28, 2020 at 05:09:52PM +0300, Dan Carpenter wrote:
> If an error occurs in the threshold_create_bank() function then the real
> clean up is supposed to happen in mce_threshold_remove_device().  The
> problem here is that if allocate_threshold_blocks() fails, then we
> kfree(b) before returning.  Then we use "b" again in
> mce_threshold_remove_device() when we do the rest of the clean up work.
> 
> Fixes: 019f34fccfd5 ("x86, MCE, AMD: Move shared bank to node descriptor")
> Reported-by: Saar Amar <Saar.Amar@microsoft.com>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I believe Saar found this through reading the code and there is no test
> case.  I have don't have a way to test it.
> 
>  arch/x86/kernel/cpu/mce/amd.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
> index b3a50d962851..ff01b789066e 100644
> --- a/arch/x86/kernel/cpu/mce/amd.c
> +++ b/arch/x86/kernel/cpu/mce/amd.c
> @@ -1342,7 +1342,8 @@ static int threshold_create_bank(unsigned int cpu, unsigned int bank)
>  	b->kobj = kobject_create_and_add(name, &dev->kobj);
>  	if (!b->kobj) {
>  		err = -EINVAL;
> -		goto out_free;
> +		kfree(b);
> +		goto out;
>  	}
>  
>  	per_cpu(threshold_banks, cpu)[bank] = b;
> @@ -1358,12 +1359,6 @@ static int threshold_create_bank(unsigned int cpu, unsigned int bank)
>  	}
>  
>  	err = allocate_threshold_blocks(cpu, bank, 0, msr_ops.misc(bank));
> -	if (!err)
> -		goto out;
> -
> - out_free:
> -	kfree(b);
> -
>   out:
>  	return err;
>  }
> -- 

Thanks for the report.

However, I think a better fix would be to publish the "b" pointer only
after allocate_threshold_blocks() succeeds so that any callers would be
sure to access only a valid pointer.

The diffstat is a bit bigger but it makes allocate_threshold_blocks() a
lot more readable, in addition, and that is very important particularly
with that rather convoluted, hard to parse and recursive function.

Thx.

---
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index b3a50d962851..e7313e5c497c 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -1198,8 +1198,9 @@ static const char *get_name(unsigned int bank, struct threshold_block *b)
 	return buf_mcatype;
 }
 
-static int allocate_threshold_blocks(unsigned int cpu, unsigned int bank,
-				     unsigned int block, u32 address)
+static int allocate_threshold_blocks(unsigned int cpu, struct threshold_bank *tb,
+				     unsigned int bank, unsigned int block,
+				     u32 address)
 {
 	struct threshold_block *b = NULL;
 	u32 low, high;
@@ -1243,16 +1244,12 @@ static int allocate_threshold_blocks(unsigned int cpu, unsigned int bank,
 
 	INIT_LIST_HEAD(&b->miscj);
 
-	if (per_cpu(threshold_banks, cpu)[bank]->blocks) {
-		list_add(&b->miscj,
-			 &per_cpu(threshold_banks, cpu)[bank]->blocks->miscj);
-	} else {
-		per_cpu(threshold_banks, cpu)[bank]->blocks = b;
-	}
+	if (tb->blocks)
+		list_add(&b->miscj, &tb->blocks->miscj);
+	else
+		tb->blocks = b;
 
-	err = kobject_init_and_add(&b->kobj, &threshold_ktype,
-				   per_cpu(threshold_banks, cpu)[bank]->kobj,
-				   get_name(bank, b));
+	err = kobject_init_and_add(&b->kobj, &threshold_ktype, tb->kobj, get_name(bank, b));
 	if (err)
 		goto out_free;
 recurse:
@@ -1260,7 +1257,7 @@ static int allocate_threshold_blocks(unsigned int cpu, unsigned int bank,
 	if (!address)
 		return 0;
 
-	err = allocate_threshold_blocks(cpu, bank, block, address);
+	err = allocate_threshold_blocks(cpu, tb, bank, block, address);
 	if (err)
 		goto out_free;
 
@@ -1345,8 +1342,6 @@ static int threshold_create_bank(unsigned int cpu, unsigned int bank)
 		goto out_free;
 	}
 
-	per_cpu(threshold_banks, cpu)[bank] = b;
-
 	if (is_shared_bank(bank)) {
 		refcount_set(&b->cpus, 1);
 
@@ -1357,9 +1352,13 @@ static int threshold_create_bank(unsigned int cpu, unsigned int bank)
 		}
 	}
 
-	err = allocate_threshold_blocks(cpu, bank, 0, msr_ops.misc(bank));
-	if (!err)
-		goto out;
+	err = allocate_threshold_blocks(cpu, b, bank, 0, msr_ops.misc(bank));
+	if (err)
+		goto out_free;
+
+	per_cpu(threshold_banks, cpu)[bank] = b;
+
+	return 0;
 
  out_free:
 	kfree(b);

-- 
Regards/Gruss,
    Boris.

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

[PATCH] x86/mce/amd: Publish the bank pointer only after setup has succeeded

[Borislav Petkov]

Now as a proper, tested patch:

---
From: Borislav Petkov <bp@suse.de>

threshold_create_bank() creates a bank descriptor per MCA error
thresholding counter which can be controlled over sysfs. It publishes
the pointer to that bank in a per-CPU variable and then goes on to
create additional thresholding blocks if the bank has such.

However, that creation of additional blocks in
allocate_threshold_blocks() can fail, leading to a use-after-free
through the per-CPU pointer.

Therefore, publish that pointer only after all blocks have been setup
successfully.

Fixes: 019f34fccfd5 ("x86, MCE, AMD: Move shared bank to node descriptor")
Reported-by: Saar Amar <Saar.Amar@microsoft.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: <stable@vger.kernel.org>
Link: http://lkml.kernel.org/r/20200128140846.phctkvx5btiexvbx@kili.mountain
---
 arch/x86/kernel/cpu/mce/amd.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index b3a50d962851..e7313e5c497c 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -1198,8 +1198,9 @@ static const char *get_name(unsigned int bank, struct threshold_block *b)
 	return buf_mcatype;
 }
 
-static int allocate_threshold_blocks(unsigned int cpu, unsigned int bank,
-				     unsigned int block, u32 address)
+static int allocate_threshold_blocks(unsigned int cpu, struct threshold_bank *tb,
+				     unsigned int bank, unsigned int block,
+				     u32 address)
 {
 	struct threshold_block *b = NULL;
 	u32 low, high;
@@ -1243,16 +1244,12 @@ static int allocate_threshold_blocks(unsigned int cpu, unsigned int bank,
 
 	INIT_LIST_HEAD(&b->miscj);
 
-	if (per_cpu(threshold_banks, cpu)[bank]->blocks) {
-		list_add(&b->miscj,
-			 &per_cpu(threshold_banks, cpu)[bank]->blocks->miscj);
-	} else {
-		per_cpu(threshold_banks, cpu)[bank]->blocks = b;
-	}
+	if (tb->blocks)
+		list_add(&b->miscj, &tb->blocks->miscj);
+	else
+		tb->blocks = b;
 
-	err = kobject_init_and_add(&b->kobj, &threshold_ktype,
-				   per_cpu(threshold_banks, cpu)[bank]->kobj,
-				   get_name(bank, b));
+	err = kobject_init_and_add(&b->kobj, &threshold_ktype, tb->kobj, get_name(bank, b));
 	if (err)
 		goto out_free;
 recurse:
@@ -1260,7 +1257,7 @@ static int allocate_threshold_blocks(unsigned int cpu, unsigned int bank,
 	if (!address)
 		return 0;
 
-	err = allocate_threshold_blocks(cpu, bank, block, address);
+	err = allocate_threshold_blocks(cpu, tb, bank, block, address);
 	if (err)
 		goto out_free;
 
@@ -1345,8 +1342,6 @@ static int threshold_create_bank(unsigned int cpu, unsigned int bank)
 		goto out_free;
 	}
 
-	per_cpu(threshold_banks, cpu)[bank] = b;
-
 	if (is_shared_bank(bank)) {
 		refcount_set(&b->cpus, 1);
 
@@ -1357,9 +1352,13 @@ static int threshold_create_bank(unsigned int cpu, unsigned int bank)
 		}
 	}
 
-	err = allocate_threshold_blocks(cpu, bank, 0, msr_ops.misc(bank));
-	if (!err)
-		goto out;
+	err = allocate_threshold_blocks(cpu, b, bank, 0, msr_ops.misc(bank));
+	if (err)
+		goto out_free;
+
+	per_cpu(threshold_banks, cpu)[bank] = b;
+
+	return 0;
 
  out_free:
 	kfree(b);
-- 
2.21.0


-- 
Regards/Gruss,
    Boris.

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