Messages in this thread | | | From | "Zhuo, Qiuxu" <> | Subject | RE: [PATCH 1/3] EDAC/igen6: Fix slab-use-after-free in igen6_unregister_mci() | Date | Mon, 9 Oct 2023 02:39:25 +0000 |
| |
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
| |