When unloading the igen6_edac driver, the EDAC core wrongly kfreed
'pvt_info,' which was private data and managed by the igen6_edac
driver. This resulted in a slab-use-after-free bug. Fix it by adding
a flag to indicate whether 'pvt_info' is managed by the EDAC core.
The EDAC core will only kfree 'pvt_info' when the flag is set to true.
BUG: KASAN: slab-use-after-free in igen6_unregister_mcis+0x74/0x1f0 [igen6_edac]
Read of size 8 at addr ffff88810c10a350 by task modprobe/5137
Call Trace:
<TASK>
dump_stack_lvl+0x4c/0x70
print_report+0xcf/0x620
? __virt_addr_valid+0xfc/0x180
? kasan_complete_mode_report_info+0x80/0x210
? igen6_unregister_mcis+0x74/0x1f0 [igen6_edac]
kasan_report+0xbf/0x100
? igen6_unregister_mcis+0x74/0x1f0 [igen6_edac]
__asan_load8+0x82/0xb0
igen6_unregister_mcis+0x74/0x1f0 [igen6_edac]
igen6_remove+0x97/0xc0 [igen6_edac]
...
Allocated by task 578:
kasan_save_stack+0x2a/0x50
kasan_set_track+0x29/0x40
kasan_save_alloc_info+0x1f/0x30
__kasan_kmalloc+0x88/0xa0
kmalloc_trace+0x4e/0xb0
igen6_probe+0xe5/0x1400 [igen6_edac]
local_pci_probe+0x89/0xf0
pci_device_probe+0x18e/0x450
...
Freed by task 5137:
kasan_save_stack+0x2a/0x50
kasan_set_track+0x29/0x40
kasan_save_free_info+0x32/0x50
__kasan_slab_free+0x113/0x1b0
slab_free_freelist_hook+0xb1/0x190
__kmem_cache_free+0x15d/0x280
kfree+0x7d/0x120
mci_release+0x24a/0x280
device_release+0x64/0x110
kobject_put+0xfd/0x260
put_device+0x17/0x30
edac_mc_free+0x43/0x50
igen6_unregister_mcis+0x18f/0x1f0 [igen6_edac]
igen6_remove+0x97/0xc0 [igen6_edac]
pci_device_remove+0x6a/0x100
device_remove+0x6f/0xb0
Fixes: 0bbb265f7089 ("EDAC/mc: Get rid of silly one-shot struct allocation in edac_mc_alloc()")
Co-developed-by: Lili Li <[email protected]>
Signed-off-by: Lili Li <[email protected]>
Tested-by: Lili Li <[email protected]>
Signed-off-by: Qiuxu Zhuo <[email protected]>
---
drivers/edac/edac_mc.c | 19 +++++++++++++++----
include/linux/edac.h | 5 +++++
2 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 6faeb2ab3960..6a68b0225130 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -201,7 +201,14 @@ static void mci_release(struct device *dev)
}
kfree(mci->csrows);
}
- kfree(mci->pvt_info);
+
+ /*
+ * if !pvt_managed_by_edac_core, the resource, e.g. memory or MMIO-mapped
+ * registers pointed by pvt_info is managed by the EDAC driver. The EDAC
+ * core shouldn't kfree it.
+ */
+ if (mci->pvt_managed_by_edac_core)
+ kfree(mci->pvt_info);
kfree(mci->layers);
kfree(mci);
}
@@ -369,9 +376,13 @@ struct mem_ctl_info *edac_mc_alloc(unsigned int mc_num,
if (!mci->layers)
goto error;
- mci->pvt_info = kzalloc(sz_pvt, GFP_KERNEL);
- if (!mci->pvt_info)
- goto error;
+ if (sz_pvt) {
+ mci->pvt_info = kzalloc(sz_pvt, GFP_KERNEL);
+ if (!mci->pvt_info)
+ goto error;
+
+ mci->pvt_managed_by_edac_core = true;
+ }
mci->dev.release = mci_release;
device_initialize(&mci->dev);
diff --git a/include/linux/edac.h b/include/linux/edac.h
index fa4bda2a70f6..6f9f4893ba77 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -567,6 +567,11 @@ struct mem_ctl_info {
const char *ctl_name;
const char *dev_name;
void *pvt_info;
+ /*
+ * Indicate whether the resource pointed by pvt_info is managed by
+ * the EDAC core.
+ */
+ bool pvt_managed_by_edac_core;
unsigned long start_time; /* mci load start time (in jiffies) */
/*
--
2.17.1
From: Lili Li <[email protected]>
If the EDAC driver invokes edac_device_alloc_ctl_info() with pvt_sz=0,
the EDAC driver itself may manage 'pvt_info' and the EDAC core shouldn't
kfree it when unloading the EDAC driver. Otherwise it may lead to a
slab-use-after-free bug. Fix the potential bug by adding a flag to indicate
whether 'pvt_info' is managed by the EDAC core. The EDAC core will only
kfree 'pvt_info' when the flag is set to true.
Fixes: 9fb9ce392aae ("EDAC/device: Get rid of the silly one-shot memory allocation in edac_device_alloc_ctl_info()")
Suggested-by: Qiuxu Zhuo <[email protected]>
Signed-off-by: Lili Li <[email protected]>
---
drivers/edac/edac_device.c | 1 +
drivers/edac/edac_device.h | 8 +++++++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/edac/edac_device.c b/drivers/edac/edac_device.c
index 0689e1510721..b990431df2eb 100644
--- a/drivers/edac/edac_device.c
+++ b/drivers/edac/edac_device.c
@@ -100,6 +100,7 @@ edac_device_alloc_ctl_info(unsigned pvt_sz, char *dev_name, unsigned nr_instance
goto free;
dev_ctl->pvt_info = pvt;
+ dev_ctl->pvt_managed_by_edac_core = true;
}
dev_ctl->dev_idx = device_index;
diff --git a/drivers/edac/edac_device.h b/drivers/edac/edac_device.h
index 3f44e6b9d387..cc6a364a4b83 100644
--- a/drivers/edac/edac_device.h
+++ b/drivers/edac/edac_device.h
@@ -197,6 +197,11 @@ struct edac_device_ctl_info {
const char *dev_name; /* pci/platform/etc... name */
void *pvt_info; /* pointer to 'private driver' info */
+ /*
+ * Indicate whether the resource pointed by pvt_info is managed by
+ * the EDAC core.
+ */
+ bool pvt_managed_by_edac_core;
unsigned long start_time; /* edac_device load start time (jiffies) */
@@ -355,7 +360,8 @@ extern const char *edac_layer_name[];
static inline void __edac_device_free_ctl_info(struct edac_device_ctl_info *ci)
{
if (ci) {
- kfree(ci->pvt_info);
+ if (ci->pvt_managed_by_edac_core)
+ kfree(ci->pvt_info);
kfree(ci->attribs);
kfree(ci->blocks);
kfree(ci->instances);
--
2.17.1
From: Lili Li <[email protected]>
The EDAC PCI core misses kfreeing 'pvt_info' which may result in a memory
leak. Fix it by adding a flag to indicate whether 'pvt_info' is allocated
by the EDAC PCI core and kfreeing 'pvt_info' by the EDAC PCI core when the
flag is set to true.
Fixes: fb8cd45ca39b ("EDAC/pci: Get rid of the silly one-shot memory allocation in edac_pci_alloc_ctl_info()")
Suggested-by: Qiuxu Zhuo <[email protected]>
Signed-off-by: Lili Li <[email protected]>
---
drivers/edac/edac_pci.c | 1 +
drivers/edac/edac_pci.h | 5 +++++
drivers/edac/edac_pci_sysfs.c | 2 ++
3 files changed, 8 insertions(+)
diff --git a/drivers/edac/edac_pci.c b/drivers/edac/edac_pci.c
index 64c142aecca7..7c9d1d9115c4 100644
--- a/drivers/edac/edac_pci.c
+++ b/drivers/edac/edac_pci.c
@@ -40,6 +40,7 @@ struct edac_pci_ctl_info *edac_pci_alloc_ctl_info(unsigned int sz_pvt,
pci->pvt_info = kzalloc(sz_pvt, GFP_KERNEL);
if (!pci->pvt_info)
goto free;
+ pci->pvt_managed_by_edac_core = true;
}
pci->op_state = OP_ALLOC;
diff --git a/drivers/edac/edac_pci.h b/drivers/edac/edac_pci.h
index 5175f5724cfa..27fecf6bfafc 100644
--- a/drivers/edac/edac_pci.h
+++ b/drivers/edac/edac_pci.h
@@ -69,6 +69,11 @@ struct edac_pci_ctl_info {
const char *dev_name; /* pci/platform/etc... name */
void *pvt_info; /* pointer to 'private driver' info */
+ /*
+ * Indicate whether the resource pointed by pvt_info is managed by
+ * EDAC core
+ */
+ bool pvt_managed_by_edac_core;
unsigned long start_time; /* edac_pci load start time (jiffies) */
diff --git a/drivers/edac/edac_pci_sysfs.c b/drivers/edac/edac_pci_sysfs.c
index 287cc51dbc86..520fb2fa9411 100644
--- a/drivers/edac/edac_pci_sysfs.c
+++ b/drivers/edac/edac_pci_sysfs.c
@@ -83,6 +83,8 @@ static void edac_pci_instance_release(struct kobject *kobj)
/* decrement reference count on top main kobj */
kobject_put(edac_pci_top_main_kobj);
+ if (pci->pvt_managed_by_edac_core)
+ kfree(pci->pvt_info);
kfree(pci); /* Free the control struct */
}
--
2.17.1
On Sun, Oct 08, 2023 at 04:02:29PM +0800, Qiuxu Zhuo wrote:
> When unloading the igen6_edac driver, the EDAC core wrongly kfreed
> 'pvt_info,' which was private data and managed by the igen6_edac
> driver. This resulted in a slab-use-after-free bug. Fix it by adding
> a flag to indicate whether 'pvt_info' is managed by the EDAC core.
> The EDAC core will only kfree 'pvt_info' when the flag is set to true.
That's because your silly driver is wrongly allocating stuff:
igen6_probe() allocates the whole pvt struct and then
igen6_register_mci() assigns it piece-meal-wise to each MC's ->pvt_info.
On the unreg path, you then call edac_mc_free(), it frees ->mct_info and
then you do wonder why it complains when you call kfree(igen6_pvt) in
igen6_remove().
You should do the exact opposite of the allocation steps on the unreg
path and it'll all work fine. Definitely not add ugly hacks to the
EDAC core.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Hi Boris,
> From: Borislav Petkov <[email protected]>
> ...
> On Sun, Oct 08, 2023 at 04:02:29PM +0800, Qiuxu Zhuo wrote:
> > When unloading the igen6_edac driver, the EDAC core wrongly kfreed
> > 'pvt_info,' which was private data and managed by the igen6_edac
> > driver. This resulted in a slab-use-after-free bug. Fix it by adding a
> > flag to indicate whether 'pvt_info' is managed by the EDAC core.
> > The EDAC core will only kfree 'pvt_info' when the flag is set to true.
>
> That's because your silly driver is wrongly allocating stuff:
>
> igen6_probe() allocates the whole pvt struct and then
> igen6_register_mci() assigns it piece-meal-wise to each MC's ->pvt_info.
>
> On the unreg path, you then call edac_mc_free(), it frees ->mct_info and then
> you do wonder why it complains when you call kfree(igen6_pvt) in
> igen6_remove().
>
> You should do the exact opposite of the allocation steps on the unreg path
> and it'll all work fine. Definitely not add ugly hacks to the EDAC core.
Thanks for the review.
Rechecked the igen6_edac code, it has already done the exact opposite of the allocation
steps on the unreg patch below:
In the reg path:
igen6_probe()
igen6_pvt = kzalloc() // Step a
igen6_register_mci()
mci = edac_mc_alloc(mc, ARRAY_SIZE(layers), layers, 0) // 0 means igen6 itself manages 'pvt_info'.
mci->pvt_info = &igen6_pvt->imc[mc];
edac_mc_add_mc(mci) // Step b
In the unreg path:
igen6_remove()
igen6_unregister_mcis()
edac_mc_free(mci) // Step B
kfree(igen6_pvt) // Step A
I assume that when calling edac_mc_alloc() with sz_pvt=0, 'pvt_info' is managed
by the EDAC driver, not the EDAC core. Therefore, the EDAC core should not kfree
'pvt_info' unconditionally in this scenario.
So the problem occurred in Step B when the EDAC core mistakenly kfreed 'pvt_info,'
which should have been kfreed in Step A. 'pvt_info' is managed by the igen6_edac
driver, not the EDAC core.
A simple fix is to set mci->pvt_info = NULL just before Step B, but it may seem a bit magical.
Similar issues could also occur in the following drivers where the EDAC core should NOT kfreed the pvt_info as
these 'pvt_info' are managed by the specific EDAC drivers themselves (sz_pvt = 0).
drivers/edac/amd8111_edac.c
dev_info->edac_dev->pvt_info = dev_info;
drivers/edac/cpc925_edac.c
dev_info->edac_dev->pvt_info = dev_info;
drivers/edac/x38_edac.c
mci->pvt_info = window; // This is even an ioremap() memory and requires iounmap() to release it.
-Qiuxu
On Mon, Oct 09, 2023 at 02:39:25AM +0000, Zhuo, Qiuxu wrote:
> Rechecked the igen6_edac code, it has already done the exact opposite
> of the allocation steps on the unreg patch below:
allocation and *assignment*. And it doesn't do them in the exact
opposite way. Look again.
> In the reg path:
> igen6_probe()
> igen6_pvt = kzalloc() // Step a
> igen6_register_mci()
> mci = edac_mc_alloc(mc, ARRAY_SIZE(layers), layers, 0) // 0 means igen6 itself manages 'pvt_info'.
> mci->pvt_info = &igen6_pvt->imc[mc];
> edac_mc_add_mc(mci) // Step b
>
> In the unreg path:
> igen6_remove()
> igen6_unregister_mcis()
> edac_mc_free(mci) // Step B
> kfree(igen6_pvt) // Step A
>
> I assume that when calling edac_mc_alloc() with sz_pvt=0, 'pvt_info' is managed
> by the EDAC driver, not the EDAC core. Therefore, the EDAC core should not kfree
> 'pvt_info' unconditionally in this scenario.
That would mean that the EDAC core would have to remember whether it
allocated a private struct for the driver. Which is not needed - if the
driver has allocated it, the same driver can free it before calling
edac_mc_free().
> So the problem occurred in Step B when the EDAC core mistakenly kfreed
> 'pvt_info,' which should have been kfreed in Step A. 'pvt_info' is
> managed by the igen6_edac driver, not the EDAC core.
Your silly driver is allocating a single struct igen6_pvt and then it is
assigning pointers of the embedded struct igen6_imc elements from the
array in igen6_pvt:
mci->pvt_info = &igen6_pvt->imc[mc];
to the pvt pointer. I.e., only a *part* of that allocated memory.
Then, it is calling
edac_mc_free(mci);
on the mci which frees only that array element - not how it got
allocated and then at the end, in the remove function, you do
kfree(igen6_pvt);
where the array elements have been freed and then KASAN rightfully
complains.
So no, you're doing it wrong.
You either need to alloc the pvt in igen6_register_mci() and free it in
igen6_unregister_mcis(), *before* edac_mc_free() or ...
> A simple fix is to set mci->pvt_info = NULL just before Step B, but it
> may seem a bit magical.
... do that which is a hack.
And looking at your driver, it is doing it wrong - it seems it doesn't
understand what the point of mci->pvt_info is.
Instead of passing around
struct mem_ctl_info *mci
to all the functions that need
struct igen6_imc *imc
so that you can do
struct igen6_imc *imc = mci->pvt_info;
in them, which is exactly what that private pointer is actually for(!),
or to do to_mci() on the containing edac device, you're using that mc
index to index into the global igen6_pvt->imc[] array.
igen6_get_dimm_config() is the only function that does it right.
So this driver needs to be fixed properly to pass around that mci
pointer, like the others do. Not this bolted on hack.
> Similar issues could also occur in the following drivers where the
> EDAC core should NOT kfreed the pvt_info as these 'pvt_info' are
> managed by the specific EDAC drivers themselves (sz_pvt = 0).
>
> drivers/edac/amd8111_edac.c
> dev_info->edac_dev->pvt_info = dev_info;
That one is not even using edac_mc_free().
> drivers/edac/cpc925_edac.c
> dev_info->edac_dev->pvt_info = dev_info;
That's the wrong one - it is:
mci = edac_mc_alloc(edac_mc_idx, ARRAY_SIZE(layers), layers,
sizeof(struct cpc925_mc_pdata));
...
pdata = mci->pvt_info;
and that's handled by the EDAC core only.
> drivers/edac/x38_edac.c
> mci->pvt_info = window; // This is even an ioremap() memory and requires iounmap() to release it.
static void x38_remove_one(struct pci_dev *pdev)
...
iounmap(mci->pvt_info);
How about you slow down, take a piece of paper, draw the allocation and
assignment ordering and try to understand what exactly the idea behind
it is? Look at how the other drivers do it, there are good examples
in there.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette