forked from rpms/glibc
		
	
		
			
				
	
	
		
			192 lines
		
	
	
		
			7.0 KiB
		
	
	
	
		
			Diff
		
	
	
	
	
	
			
		
		
	
	
			192 lines
		
	
	
		
			7.0 KiB
		
	
	
	
		
			Diff
		
	
	
	
	
	
| commit bde121872001d8f3224eeafa5b7effb871c3fbca
 | |
| Author: Simon Kissane <skissane@gmail.com>
 | |
| Date:   Sat Feb 11 08:58:02 2023 +1100
 | |
| 
 | |
|     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>
 | |
| 
 | |
| Conflicts:
 | |
| 	gmon/Makefile
 | |
| 	  (copyright year update)
 | |
| 
 | |
| diff --git a/gmon/Makefile b/gmon/Makefile
 | |
| index 54f05894d4dd8c4a..1bc4ad6e14e292a9 100644
 | |
| --- a/gmon/Makefile
 | |
| +++ b/gmon/Makefile
 | |
| @@ -1,4 +1,5 @@
 | |
| -# Copyright (C) 1995-2018 Free Software Foundation, Inc.
 | |
| +# Copyright (C) 1995-2023 Free Software Foundation, Inc.
 | |
| +# Copyright The GNU Toolchain Authors.
 | |
|  # This file is part of the GNU C Library.
 | |
|  
 | |
|  # The GNU C Library is free software; you can redistribute it and/or
 | |
| @@ -25,7 +26,7 @@ include ../Makeconfig
 | |
|  headers	:= sys/gmon.h sys/gmon_out.h sys/profil.h
 | |
|  routines := gmon mcount profil sprofil prof-freq
 | |
|  
 | |
| -tests	= tst-sprofil tst-gmon tst-mcount-overflow
 | |
| +tests	= tst-sprofil tst-gmon tst-mcount-overflow tst-mcleanup
 | |
|  ifeq ($(build-profile),yes)
 | |
|  tests	+= tst-profile-static
 | |
|  tests-static	+= tst-profile-static
 | |
| @@ -68,6 +69,14 @@ ifeq ($(run-built-tests),yes)
 | |
|  tests-special += $(objpfx)tst-mcount-overflow-check.out
 | |
|  endif
 | |
|  
 | |
| +CFLAGS-tst-mcleanup.c := -fno-omit-frame-pointer -pg
 | |
| +tst-mcleanup-no-pie = yes
 | |
| +CRT-tst-mcleanup := $(csu-objpfx)g$(start-installed-name)
 | |
| +tst-mcleanup-ENV := GMON_OUT_PREFIX=$(objpfx)tst-mcleanup.data
 | |
| +ifeq ($(run-built-tests),yes)
 | |
| +tests-special += $(objpfx)tst-mcleanup.out
 | |
| +endif
 | |
| +
 | |
|  CFLAGS-tst-gmon-static.c := $(PIE-ccflag) -fno-omit-frame-pointer -pg
 | |
|  CRT-tst-gmon-static := $(csu-objpfx)gcrt1.o
 | |
|  tst-gmon-static-no-pie = yes
 | |
| @@ -123,6 +132,10 @@ $(objpfx)tst-mcount-overflow-check.out: tst-mcount-overflow-check.sh $(objpfx)ts
 | |
|  	$(SHELL) $< $(objpfx)tst-mcount-overflow > $@; \
 | |
|  	$(evaluate-test)
 | |
|  
 | |
| +$(objpfx)tst-mcleanup.out: clean-tst-mcleanup-data
 | |
| +clean-tst-mcleanup-data:
 | |
| +	rm -f $(objpfx)tst-mcleanup.data.*
 | |
| +
 | |
|  $(objpfx)tst-gmon-gprof.out: tst-gmon-gprof.sh $(objpfx)tst-gmon.out
 | |
|  	$(SHELL) $< $(GPROF) $(objpfx)tst-gmon $(objpfx)tst-gmon.data.* > $@; \
 | |
|  	$(evaluate-test)
 | |
| diff --git a/gmon/gmon.c b/gmon/gmon.c
 | |
| index 689bf80141e559ca..5e99a7351dc71666 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)
 | |
| @@ -146,6 +145,14 @@ __monstartup (u_long lowpc, u_long highpc)
 | |
|    maxarcs = MAXARCS;
 | |
|  #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;
 | |
|  }
 | |
| diff --git a/gmon/tst-mcleanup.c b/gmon/tst-mcleanup.c
 | |
| new file mode 100644
 | |
| index 0000000000000000..b259653ec833aca4
 | |
| --- /dev/null
 | |
| +++ b/gmon/tst-mcleanup.c
 | |
| @@ -0,0 +1,31 @@
 | |
| +/* Test program for repeated invocation of _mcleanup
 | |
| +   Copyright The GNU Toolchain Authors.
 | |
| +   This file is part of the GNU C Library.
 | |
| +
 | |
| +   The GNU C Library is free software; you can redistribute it and/or
 | |
| +   modify it under the terms of the GNU Lesser General Public
 | |
| +   License as published by the Free Software Foundation; either
 | |
| +   version 2.1 of the License, or (at your option) any later version.
 | |
| +
 | |
| +   The GNU C Library is distributed in the hope that it will be useful,
 | |
| +   but WITHOUT ANY WARRANTY; without even the implied warranty of
 | |
| +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 | |
| +   Lesser General Public License for more details.
 | |
| +
 | |
| +   You should have received a copy of the GNU Lesser General Public
 | |
| +   License along with the GNU C Library; if not, see
 | |
| +   <https://www.gnu.org/licenses/>.  */
 | |
| +
 | |
| +/* Intentionally calls _mcleanup() twice: once manually, it will be
 | |
| +   called again as an atexit handler. This is incorrect use of the API,
 | |
| +   but the point of the test is to make sure we don't crash when the
 | |
| +   API is misused in this way. */
 | |
| +
 | |
| +#include <sys/gmon.h>
 | |
| +
 | |
| +int
 | |
| +main (void)
 | |
| +{
 | |
| +  _mcleanup();
 | |
| +  return 0;
 | |
| +}
 |