* RE: [EXTERNAL] [PATCH] x86/MCE/AMD: Fix use after free in error handling
2020-01-28 14:09 ` [PATCH] x86/MCE/AMD: Fix use after free in error handling Dan Carpenter
@ 2020-01-28 14:12 ` Saar Amar
2020-01-28 15:59 ` Saar Amar
2020-02-04 13:36 ` Borislav Petkov
2 siblings, 0 replies; 6+ messages in thread
From: Saar Amar @ 2020-01-28 14:12 UTC (permalink / raw)
To: Dan Carpenter, Tony Luck, Borislav Petkov
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86@kernel.org,
linux-edac@vger.kernel.org, security@kernel.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [EXTERNAL] [PATCH] x86/MCE/AMD: Fix use after free in error handling
2020-01-28 14:09 ` [PATCH] x86/MCE/AMD: Fix use after free in error handling Dan Carpenter
2020-01-28 14:12 ` [EXTERNAL] " Saar Amar
@ 2020-01-28 15:59 ` Saar Amar
2020-01-28 16:15 ` Dan Carpenter
2020-02-04 13:36 ` Borislav Petkov
2 siblings, 1 reply; 6+ messages in thread
From: Saar Amar @ 2020-01-28 15:59 UTC (permalink / raw)
To: Dan Carpenter, Tony Luck, Borislav Petkov
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86@kernel.org,
linux-edac@vger.kernel.org, security@kernel.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/MCE/AMD: Fix use after free in error handling
2020-01-28 14:09 ` [PATCH] x86/MCE/AMD: Fix use after free in error handling Dan Carpenter
2020-01-28 14:12 ` [EXTERNAL] " Saar Amar
2020-01-28 15:59 ` Saar Amar
@ 2020-02-04 13:36 ` Borislav Petkov
2020-02-14 8:37 ` [PATCH] x86/mce/amd: Publish the bank pointer only after setup has succeeded Borislav Petkov
2 siblings, 1 reply; 6+ messages in thread
From: Borislav Petkov @ 2020-02-04 13:36 UTC (permalink / raw)
To: Dan Carpenter
Cc: Tony Luck, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
linux-edac, Saar Amar, security@kernel.org
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
^ permalink raw reply related [flat|nested] 6+ messages in thread