[Bug libc/30101] New: Memory corruption when incorrectly calling _mcleanup/__monstartup/moncontrol repeatedly or in wrong order

[Bug libc/30101] New: Memory corruption when incorrectly calling _mcleanup/__monstartup/moncontrol repeatedly or in wrong order

[skissane at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30101

            Bug ID: 30101
           Summary: Memory corruption when incorrectly calling
                    _mcleanup/__monstartup/moncontrol repeatedly or in
                    wrong order
           Product: glibc
           Version: unspecified
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: libc
          Assignee: unassigned at sourceware dot org
          Reporter: skissane at gmail dot com
                CC: drepper.fsp at gmail dot com
  Target Milestone: ---

Reproduced in latest Git master branch. I wrote a patch which I will send to
the mailing list shortly.

It is possible to corrupt memory by calling the _mcleanup, __monstartup, and
moncontrol APIs an incorrect number of times, or in an incorrect order. While
such incorrect use of these APIs is of course unsupported, it still should not
corrupt memory or crash. And, I think it is relatively simple to fix these
bugs, so why not.

##### Issue 1: Calling _mcleanup twice in a row crashes

This one is the easiest to reproduce. The second call will crash due to a
double free. Additionally, prior to the double free, it attempts to read
deallocated memory (although that is only obvious under valgrind), and will
write out a gmon.out file containing (possibly meaningless) data read from that
deallocated memory.

Test case:

1. Create file test_mcleanup.c with following contents:

#include <sys/gmon.h>
int main(int argc, char **argv) { _mcleanup(); return 0; }

2. Build and run it:

gcc -pg -o test_mcleanup test_mcleanup.c  && ./test_mcleanup

3. Observe the crash:

double free or corruption (top)
Aborted (core dumped)

This happens because _mcleanup is called once manually (from main), and then a
second time (as an atexit handler installed by __gmon_start__). _mcleanup calls
free on the tos pointer, but doesn't set it to NULL, so the second call results
in a double free.

To fix this, zero the _gmonparam buffer at end of _mcleanup. While setting
tos=NULL would be a minimal fix, zeroing the buffer is safest and helps prevent
other issues. To ensure we don't read any unallocated memory, we also need to
skip calling write_gmon() when the tos pointer is NULL (since write_gmon is
written to assume tos!=NULL).

##### Issue 2: Calling __monstartup repeatedly leaks memory

This issue is only visible under valgrind. __monstartup allocates a buffer, the
address of which is placed in _gmonparam.tos. If you call __monstartup again
(without calling _mcleanup first), it allocates a new buffer, and overwrites
the address of the old one, which is never freed. I think the simplest solution
is to check that tos == NULL at start of __monstartup, and just return in that
case, turning improper repeated calls to __monstartup into no-ops.

The other option to fix this would be to free any existing buffer at the start
of __monstartup; before doing that, we must also call moncontrol(0), in case
the kernel is writing to it due to the __profil system call. That would make a
second call to __monstartup override the first, instead of being a no-op; while
this is a reasonable solution, I prefer the first option since it is simpler.

##### Issue 3: Calling moncontrol(1) after _mcleanup corrupts memory

This issue is hypothetical, it would be difficult to reproduce, so I have not
attempted to do so.

A buggy program calls _mcleanup(), then some time later incorrectly calls
moncontrol(1), without an intervening call to __monstartup. Prior to the fix
for to issue (1) above, _mcleanup() did not zero out _gmonparam.kcount, which
would now point to freed memory, which may have since been reallocated for some
unrelated purpose. That pointer to freed memory would then be passed to the
__profil system call, which would then cause the kernel to overwrite that
unrelated data structure with profiling data.

With zeroing of _gmonparam at end of _mcleanup to fix (1), this issue can no
longer occur. Instead something less drastic goes wrong – the _gmonparam.state
gets incorrectly changed to GMON_PROF_ON, despite the fact that profiling is
actually off. From my reading of the mcount code, it seems plausible that might
cause memory corruption or a crash, but I haven't been able to reproduce any
such issue. To be safest, ensure the state remains GMON_PROF_OFF if tos==NULL

##### Issue 4: moncontrol(0) doesn't disable profiling in error state

This is also a hypothetical issue I haven't been able to reproduce. It relates
to the if branch at start of moncontrol, which makes the operation a no-op when
in GMON_PROF_ERROR state.

There are two ways we could end up in GMON_PROF_ERROR state: memory allocation
failure (out of memory) in __monstartup, or an overflow error in the mcount
function.

Suppose we call _mcleanup(), which in turn calls moncontrol(0). If
state=GMON_PROF_ERROR due to memory allocation failure, we will have tos=NULL,
and we never will have enabled profiling, so even though moncontrol(0) won't
disable profiling, it shouldn't be on to begin with, so we are fine.

However, suppose we had an overflow error in mcount. Now state=GMON_PROF_ERROR,
but profiling is on, and tos!=NULL. We call _mcleanup(), which in turn calls
moncontrol(0) – but since state=GMON_PROF_ERROR, moncontrol does not make the
__profil system call to turn profiling off. Now _mcleanup() frees the tos
buffer, but the kernel goes on writing the profiling data into the
now-deallocated memory, so this is another variant on issue 3. Unlike issue
(3), zeroing the buffer in _mcleanup (to fix (1)) makes no difference here,
since the address of the freed buffer is saved in kernel memory.

Aside from the difficulty in reproducing issue (3), I'm also not sure how to
trigger an overflow error in mcount. However, the fix is simple: have
mcontrol() still disable profiling even if called in error state. But make sure
we keep the current behaviour that mcontrol(0) doesn't clear any error
condition. One change in behaviour I would propose, is make mcontrol(1) in
error condition turn into a stop instead of a nop; I think doing it that way
makes the code a little simpler, and leaving kernel profiling on in the error
state is pointless (since we won't write the file).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30101] Memory corruption when incorrectly calling _mcleanup/__monstartup/moncontrol repeatedly or in wrong order

[skissane at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30101

--- Comment #1 from Simon Kissane <skissane at gmail dot com> ---
Created attachment 14665
  --> https://sourceware.org/bugzilla/attachment.cgi?id=14665&action=edit
patch sent to mailing list

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30101] Memory corruption when incorrectly calling _mcleanup/__monstartup/moncontrol repeatedly or in wrong order

[skissane at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30101

Simon Kissane <skissane at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #14665|0                           |1
        is obsolete|                            |

--- Comment #2 from Simon Kissane <skissane at gmail dot com> ---
Created attachment 14674
  --> https://sourceware.org/bugzilla/attachment.cgi?id=14674&action=edit
V2 of patch (also sent to mailing list)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30101] Memory corruption when incorrectly calling _mcleanup/__monstartup/moncontrol repeatedly or in wrong order

[skissane at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30101

--- Comment #3 from Simon Kissane <skissane at gmail dot com> ---
Created V2 of patch. I realised "zero the _gmonparam buffer at end of
_mcleanup" is not 100% correct, because (counterintuitively) 0=ON, 3=OFF, so
zeroing the buffer actually makes it (incorrectly) report that profiling is ON.
So, after zeroing with memset, we then need to explicitly set state=OFF.

-- 
You are receiving this mail because:
You are on the CC list for the bug.