linux-kernel - RE: [PATCH 1/3] EDAC/igen6: Fix slab-use-after-free in igen6_unregister

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CY8PR11MB713434F22846F9002DB6238C89CEA@CY8PR11MB7134.namprd11.prod.outlook.com>
Date:   Mon, 9 Oct 2023 02:39:25 +0000
From:   "Zhuo, Qiuxu" <qiuxu.zhuo@...el.com>
To:     Borislav Petkov <bp@...en8.de>
CC:     "Luck, Tony" <tony.luck@...el.com>, "Li, Lili" <lili.li@...el.com>,
        "James Morse" <james.morse@....com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Robert Richter <rric@...nel.org>,
        "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/3] EDAC/igen6: Fix slab-use-after-free in
 igen6_unregister_mci()

Hi Boris,

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