| Message ID | 20231008080231.51917-1-qiuxu.zhuo@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] EDAC/igen6: Fix slab-use-after-free in igen6_unregister_mci() | expand |
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.
Hi Boris, > From: Borislav Petkov <bp@alien8.de> > ... > 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.
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) */ /*
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 <lili.li@intel.com> Signed-off-by: Lili Li <lili.li@intel.com> Tested-by: Lili Li <lili.li@intel.com> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com> --- drivers/edac/edac_mc.c | 19 +++++++++++++++---- include/linux/edac.h | 5 +++++ 2 files changed, 20 insertions(+), 4 deletions(-)