RE: [PATCH 1/3] EDAC/igen6: Fix slab-use-after-free in igen6_unregister_mci()

[Zhuo, Qiuxu]

Hi Boris,

> From: Borislav Petkov <bp@xxxxxxxxx>
> ...
> 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