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: Fengguang Wu <fengguang.wu@intel.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 616d90bcb3a4106929523c4a98fe32fb27f6a9a6..9037ffa100d5991abd2323f052590e44dd8f8145 100644 (file)
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -419,14 +419,16 @@ error:
                kfree(mci->dimms);
        }
        if (mci->csrows) {
                kfree(mci->dimms);
        }
        if (mci->csrows) {

-               for (chn = 0; chn < tot_channels; chn++) {
-                       csr = mci->csrows[chn];
+               for (row = 0; row < tot_csrows; row++) {
+                       csr = mci->csrows[row];

                        if (csr) {
                        if (csr) {

-                               for (chn = 0; chn < tot_channels; chn++)
-                                       kfree(csr->channels[chn]);
+                               if (csr->channels) {
+                                       for (chn = 0; chn < tot_channels; chn++)
+                                               kfree(csr->channels[chn]);
+                                       kfree(csr->channels);
+                               }

                                kfree(csr);
                        }
                                kfree(csr);
                        }

-                       kfree(mci->csrows[i]);

                }
                kfree(mci->csrows);
        }
                }
                kfree(mci->csrows);
        }