gmon: fix memory corruption issues [BZ# 30101]

V2 of this patch fixes an issue in V1, where the state was changed to ON not
OFF at end of _mcleanup. I hadn't noticed that (counterintuitively) ON=0 and
OFF=3, hence zeroing the buffer turned it back on. So set the state to OFF
after the memset.

1. Prevent double free, and reads from unallocated memory, when
   _mcleanup is (incorrectly) called two or more times in a row,
   without an intervening call to __monstartup; with this patch, the
   second and subsequent calls effectively become no-ops instead.
   While setting tos=NULL is minimal fix, safest action is to zero the
   whole gmonparam buffer.

2. Prevent memory leak when __monstartup is (incorrectly) called two
   or more times in a row, without an intervening call to _mcleanup;
   with this patch, the second and subsequent calls effectively become
   no-ops instead.

3. After _mcleanup, treat __moncontrol(1) as __moncontrol(0) instead.
   With zeroing of gmonparam buffer in _mcleanup, this stops the
   state incorrectly being changed to GMON_PROF_ON despite profiling
   actually being off. If we'd just done the minimal fix to _mcleanup
   of setting tos=NULL, there is risk of far worse memory corruption:
   kcount would point to deallocated memory, and the __profil syscall
   would make the kernel write profiling data into that memory,
   which could have since been reallocated to something unrelated.

4. Ensure __moncontrol(0) still turns off profiling even in error
   state. Otherwise, if mcount overflows and sets state to
   GMON_PROF_ERROR, when _mcleanup calls __moncontrol(0), the __profil
   syscall to disable profiling will not be invoked. _mcleanup will
   free the buffer, but the kernel will still be writing profiling
   data into it, potentially corrupted arbitrary memory.

Also adds a test case for (1). Issues (2)-(4) are not feasible to test.

Signed-off-by: Simon Kissane <skissane@gmail.com>
Reviewed-by: DJ Delorie <dj@redhat.com>

1 files changed, 19 insertions, 7 deletions

diff --git a/gmon/gmon.c b/gmon/gmon.c
index 689bf80141..5e99a7351d 100644
--- a/gmon/gmon.c
+++ b/gmon/gmon.c
@@ -102,11 +102,8 @@ __moncontrol (int mode)
 {
   struct gmonparam *p = &_gmonparam;
 
-  /* Don't change the state if we ran into an error.  */
-  if (p->state == GMON_PROF_ERROR)
-    return;
-
-  if (mode)
+  /* Treat start request as stop if error or gmon not initialized. */
+  if (mode && p->state != GMON_PROF_ERROR && p->tos != NULL)
     {
       /* start */
       __profil((void *) p->kcount, p->kcountsize, p->lowpc, s_scale);
@@ -116,7 +113,9 @@ __moncontrol (int mode)
     {
       /* stop */
       __profil(NULL, 0, 0, 0);
-      p->state = GMON_PROF_OFF;
+      /* Don't change the state if we ran into an error. */
+      if (p->state != GMON_PROF_ERROR)
+        p->state = GMON_PROF_OFF;
     }
 }
 libc_hidden_def (__moncontrol)
@@ -147,6 +146,14 @@ __monstartup (u_long lowpc, u_long highpc)
 #endif
 
   /*
+   * If we are incorrectly called twice in a row (without an
+   * intervening call to _mcleanup), ignore the second call to
+   * prevent leaking memory.
+   */
+  if (p->tos != NULL)
+      return;
+
+  /*
    * round lowpc and highpc to multiples of the density we're using
    * so the rest of the scaling (here and in gprof) stays in ints.
    */
@@ -463,9 +470,14 @@ _mcleanup (void)
 {
   __moncontrol (0);
 
-  if (_gmonparam.state != GMON_PROF_ERROR)
+  if (_gmonparam.state != GMON_PROF_ERROR && _gmonparam.tos != NULL)
     write_gmon ();
 
   /* free the memory. */
   free (_gmonparam.tos);
+
+  /* reset buffer to initial state for safety */
+  memset(&_gmonparam, 0, sizeof _gmonparam);
+  /* somewhat confusingly, ON=0, OFF=3 */
+  _gmonparam.state = GMON_PROF_OFF;
 }