| Message ID | 20200128140846.phctkvx5btiexvbx@kili.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/MCE/AMD: Fix use after free in error handling | expand |
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
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
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);
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; }
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(-)