Commit ef6e7816 authored by Fengguang Wu's avatar Fengguang Wu Committed by Linus Torvalds

edac_mc: fix messy kfree calls in the error path

coccinelle warns about:

+ drivers/edac/edac_mc.c:429:9-23: ERROR: reference preceded by free on line 429

   421         if (mci->csrows) {
 > 422                 for (chn = 0; chn < tot_channels; chn++) {
   423                         csr = mci->csrows[chn];
   424                         if (csr) {
 > 425                                 for (chn = 0; chn < tot_channels; chn++)
   426                                          kfree(csr->channels[chn]);
   427                                  kfree(csr);
   428                          }
 > 429                          kfree(mci->csrows[i]);
   430                  }
   431                  kfree(mci->csrows);
   432          }

and that code block seem to mess things up in several ways (double free, memory
leak, out-of-bound reads etc.):

L422: The iterator "chn" and bound "tot_channels" are totally wrong. Should be
      "row" and "tot_csrows" respectively. Which means either memory leak, or
      out-of-bound reads (which if does not trigger an immediate page fault
      error, will further lead to kfree() on random addresses).

L425: The inner loop is reusing the same iterator "chn" as the outer loop,
      which could lead to premature end of the outer loop, and hence memory leak.

L429: The array index 'i' in mci->csrows[i] is a temporary value used in
      previous loops, and won't change at all in the current loop. Which
      means either out-of-bound read and possibly kfree(random number), or the
      same mci->csrows[i] get freed once and again, and possibly double free
      for the kfree(csr) in L427.

L426/L427: a kfree(csr->channels) is needed in between to avoid leaking the memory.

The buggy code was introduced by commit de3910eb ("edac: change the mem
allocation scheme to make Documentation/kobject.txt happy") in the 3.6-rc1
merge window. Fix it by freeing up resources in this order:

  free csrows[i]->channels[j]
  free csrows[i]->channels
  free csrows[i]
  free csrows

CC: Mauro Carvalho Chehab <mchehab@redhat.com>
CC: Shaun Ruffell <sruffell@digium.com>
Signed-off-by: default avatarFengguang Wu <fengguang.wu@intel.com>
Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
parent e5e77cf9
...@@ -419,14 +419,16 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num, ...@@ -419,14 +419,16 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
kfree(mci->dimms); kfree(mci->dimms);
} }
if (mci->csrows) { if (mci->csrows) {
for (chn = 0; chn < tot_channels; chn++) { for (row = 0; row < tot_csrows; row++) {
csr = mci->csrows[chn]; csr = mci->csrows[row];
if (csr) { if (csr) {
if (csr->channels) {
for (chn = 0; chn < tot_channels; chn++) for (chn = 0; chn < tot_channels; chn++)
kfree(csr->channels[chn]); kfree(csr->channels[chn]);
kfree(csr->channels);
}
kfree(csr); kfree(csr);
} }
kfree(mci->csrows[i]);
} }
kfree(mci->csrows); kfree(mci->csrows);
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment