[PATCH] gmon: Fix allocated buffer overflow (bug 2944)

[PATCH] gmon: Fix allocated buffer overflow (bug 2944)

[Леонид Юрьев (Leonid Yuriev)]

The `__monstartup()` allocates a buffer used to store all the data
accumulated by the monitor.

The size of this buffer depends on the size of the internal structures
used and the address range for which the monitor is activated, as well
as on the maximum density of call instuctions and/or callable functions
that could be potentially on a segment of executable code.

In particular a hash table of arcs is placed at the end of this buffer.
The size of this hash table is calculated in bytes as
   p->fromssize = p->textsize / HASHFRACTION;

but actually should be
   p->fromssize = ROUNDUP(p->textsize / HASHFRACTION, sizeof(*p->froms));

Another minor error seems a related typo in the calculation of `kcountsize`.

This results in writing beyond the end of the allocated buffer when an
added arc corresponds to a call near from the end of the monitored
address range, since `_mcount()` check the incoming caller address for
monitored range but not the intermediate result hash-like index that
uses to write into the table.

It should be noted that when the results are output to `gmon.out`, the
table is read to the last element calculated from the allocated size in
bytes, so the arcs stored outside the buffer boundary did not fall into
`gprof` for analysis. Thus this "feature" help me to found this bug
during working with https://sourceware.org/bugzilla/show_bug.cgi?id=29438

Just in case, I will explicitly note that the problem breaks the
`make test t=gmon/tst-gmon-dso` added for Bug 29438.
There, the arc of the `f3()` call disappears from the output, since in
the DSO case, the call to `f3` is located close to the end of the
monitored range.

Signed-off-by: Леонид Юрьев (Leonid Yuriev) <leo@yuriev.ru>
---
 gmon/gmon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gmon/gmon.c b/gmon/gmon.c
index dee64803ad..4712d9f66b 100644
--- a/gmon/gmon.c
+++ b/gmon/gmon.c
@@ -132,7 +132,7 @@ __monstartup (u_long lowpc, u_long highpc)
   p->lowpc = ROUNDDOWN(lowpc, HISTFRACTION * sizeof(HISTCOUNTER));
   p->highpc = ROUNDUP(highpc, HISTFRACTION * sizeof(HISTCOUNTER));
   p->textsize = p->highpc - p->lowpc;
-  p->kcountsize = ROUNDUP(p->textsize / HISTFRACTION, sizeof(*p->froms));
+  p->kcountsize = ROUNDUP(p->textsize / HISTFRACTION, sizeof(*p->kcount));
   p->hashfraction = HASHFRACTION;
   p->log_hashfraction = -1;
   /* The following test must be kept in sync with the corresponding
@@ -142,7 +142,7 @@ __monstartup (u_long lowpc, u_long highpc)
 	 instead of integer division.  Precompute shift amount. */
       p->log_hashfraction = ffs(p->hashfraction * sizeof(*p->froms)) - 1;
   }
-  p->fromssize = p->textsize / HASHFRACTION;
+  p->fromssize = ROUNDUP(p->textsize / HASHFRACTION, sizeof(*p->froms));
   p->tolimit = p->textsize * ARCDENSITY / 100;
   if (p->tolimit < MINARCS)
     p->tolimit = MINARCS;
-- 
2.34.1

Re: [PATCH] gmon: Fix allocated buffer overflow (bug 2944)

[Carlos O'Donell]

On 2/4/23 06:41, Леонид Юрьев (Leonid Yuriev) wrote:
> The `__monstartup()` allocates a buffer used to store all the data
> accumulated by the monitor.

I haven't reviewed this yet, but the bug number is "29444" and is incorrectly
written in the Subject line.

Thanks for posting this. We talked briefly about this patch during the Monday
patch review and we're trying to find a developer to review it.
 
> The size of this buffer depends on the size of the internal structures
> used and the address range for which the monitor is activated, as well
> as on the maximum density of call instuctions and/or callable functions
> that could be potentially on a segment of executable code.
> 
> In particular a hash table of arcs is placed at the end of this buffer.
> The size of this hash table is calculated in bytes as
>    p->fromssize = p->textsize / HASHFRACTION;
> 
> but actually should be
>    p->fromssize = ROUNDUP(p->textsize / HASHFRACTION, sizeof(*p->froms));
> 
> Another minor error seems a related typo in the calculation of `kcountsize`.
> 
> This results in writing beyond the end of the allocated buffer when an
> added arc corresponds to a call near from the end of the monitored
> address range, since `_mcount()` check the incoming caller address for
> monitored range but not the intermediate result hash-like index that
> uses to write into the table.
> 
> It should be noted that when the results are output to `gmon.out`, the
> table is read to the last element calculated from the allocated size in
> bytes, so the arcs stored outside the buffer boundary did not fall into
> `gprof` for analysis. Thus this "feature" help me to found this bug
> during working with https://sourceware.org/bugzilla/show_bug.cgi?id=29438
> 
> Just in case, I will explicitly note that the problem breaks the
> `make test t=gmon/tst-gmon-dso` added for Bug 29438.
> There, the arc of the `f3()` call disappears from the output, since in
> the DSO case, the call to `f3` is located close to the end of the
> monitored range.
> 
> Signed-off-by: Леонид Юрьев (Leonid Yuriev) <leo@yuriev.ru>
> ---
>  gmon/gmon.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gmon/gmon.c b/gmon/gmon.c
> index dee64803ad..4712d9f66b 100644
> --- a/gmon/gmon.c
> +++ b/gmon/gmon.c
> @@ -132,7 +132,7 @@ __monstartup (u_long lowpc, u_long highpc)
>    p->lowpc = ROUNDDOWN(lowpc, HISTFRACTION * sizeof(HISTCOUNTER));
>    p->highpc = ROUNDUP(highpc, HISTFRACTION * sizeof(HISTCOUNTER));
>    p->textsize = p->highpc - p->lowpc;
> -  p->kcountsize = ROUNDUP(p->textsize / HISTFRACTION, sizeof(*p->froms));
> +  p->kcountsize = ROUNDUP(p->textsize / HISTFRACTION, sizeof(*p->kcount));
>    p->hashfraction = HASHFRACTION;
>    p->log_hashfraction = -1;
>    /* The following test must be kept in sync with the corresponding
> @@ -142,7 +142,7 @@ __monstartup (u_long lowpc, u_long highpc)
>  	 instead of integer division.  Precompute shift amount. */
>        p->log_hashfraction = ffs(p->hashfraction * sizeof(*p->froms)) - 1;
>    }
> -  p->fromssize = p->textsize / HASHFRACTION;
> +  p->fromssize = ROUNDUP(p->textsize / HASHFRACTION, sizeof(*p->froms));
>    p->tolimit = p->textsize * ARCDENSITY / 100;
>    if (p->tolimit < MINARCS)
>      p->tolimit = MINARCS;

-- 
Cheers,
Carlos.

Re: [PATCH] gmon: Fix allocated buffer overflow (bug 2944)

[Siddhesh Poyarekar]

On 2023-02-04 06:41, Леонид Юрьев (Leonid Yuriev) wrote:
> The `__monstartup()` allocates a buffer used to store all the data
> accumulated by the monitor.
> 
> The size of this buffer depends on the size of the internal structures
> used and the address range for which the monitor is activated, as well
> as on the maximum density of call instuctions and/or callable functions
> that could be potentially on a segment of executable code.
> 
> In particular a hash table of arcs is placed at the end of this buffer.
> The size of this hash table is calculated in bytes as
>     p->fromssize = p->textsize / HASHFRACTION;
> 
> but actually should be
>     p->fromssize = ROUNDUP(p->textsize / HASHFRACTION, sizeof(*p->froms));
> 
> Another minor error seems a related typo in the calculation of `kcountsize`.
> 
> This results in writing beyond the end of the allocated buffer when an
> added arc corresponds to a call near from the end of the monitored
> address range, since `_mcount()` check the incoming caller address for
> monitored range but not the intermediate result hash-like index that
> uses to write into the table.
> 
> It should be noted that when the results are output to `gmon.out`, the
> table is read to the last element calculated from the allocated size in
> bytes, so the arcs stored outside the buffer boundary did not fall into
> `gprof` for analysis. Thus this "feature" help me to found this bug
> during working with https://sourceware.org/bugzilla/show_bug.cgi?id=29438
> 
> Just in case, I will explicitly note that the problem breaks the
> `make test t=gmon/tst-gmon-dso` added for Bug 29438.
> There, the arc of the `f3()` call disappears from the output, since in
> the DSO case, the call to `f3` is located close to the end of the
> monitored range.
> 
> Signed-off-by: Леонид Юрьев (Leonid Yuriev) <leo@yuriev.ru>

Adding a quick note here since this got raised as a security issue: this 
is a bug, but I don't see any security impact here since the inputs that 
cause this are trusted, coming from addresses of a profiled application. 
We'll be rejecting this CVE.

I'll leave the actual patch review and fix incorporation to DJ, who has 
volunteered to look at it.

Thanks,
Sid

Re: [PATCH] gmon: Fix allocated buffer overflow (bug 2944)

[DJ Delorie]

›µ¾½¸´
®€Œµ² (Leonid Yuriev) <leo@yuriev.ru>
writes:
>    p->lowpc = ROUNDDOWN(lowpc, HISTFRACTION * sizeof(HISTCOUNTER));
>    p->highpc = ROUNDUP(highpc, HISTFRACTION * sizeof(HISTCOUNTER));
>    p->textsize = p->highpc - p->lowpc;
> -  p->kcountsize = ROUNDUP(p->textsize / HISTFRACTION, sizeof(*p->froms));
> +  p->kcountsize = ROUNDUP(p->textsize / HISTFRACTION, sizeof(*p->kcount));

I think the purpose here isn't to make sure the last entry fits, as that
it handled when lowpc and highpc are rounded.  I think the purpose here
is to make sure that the *next* portion of the buffer (p->froms) is
suitably aligned for its type.

> -  p->fromssize = p->textsize / HASHFRACTION;
> +  p->fromssize = ROUNDUP(p->textsize / HASHFRACTION, sizeof(*p->froms));

This part looks OK to me.

[PATCH v2] gmon: Fix allocated buffer overflow (bug 2944)

[DJ Delorie]

Not getting any feedback from my review-with-questions, I'm posting a
patch that assumes I'm right about the change I thought Leonid's patch
needed.  Fight!  ;-)


From f7fddb025720d4e318fe5425f5a3235e73a13282 Mon Sep 17 00:00:00 2001
From: "Леонид Юрьев (Leonid Yuriev)" <leo@yuriev.ru>
Date: Sat, 4 Feb 2023 14:41:38 +0300
Subject: gmon: Fix allocated buffer overflow (bug 2944)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The `__monstartup()` allocates a buffer used to store all the data
accumulated by the monitor.

The size of this buffer depends on the size of the internal structures
used and the address range for which the monitor is activated, as well
as on the maximum density of call instuctions and/or callable functions
that could be potentially on a segment of executable code.

In particular a hash table of arcs is placed at the end of this buffer.
The size of this hash table is calculated in bytes as
   p->fromssize = p->textsize / HASHFRACTION;

but actually should be
   p->fromssize = ROUNDUP(p->textsize / HASHFRACTION, sizeof(*p->froms));

This results in writing beyond the end of the allocated buffer when an
added arc corresponds to a call near from the end of the monitored
address range, since `_mcount()` check the incoming caller address for
monitored range but not the intermediate result hash-like index that
uses to write into the table.

It should be noted that when the results are output to `gmon.out`, the
table is read to the last element calculated from the allocated size in
bytes, so the arcs stored outside the buffer boundary did not fall into
`gprof` for analysis. Thus this "feature" help me to found this bug
during working with https://sourceware.org/bugzilla/show_bug.cgi?id=29438

Just in case, I will explicitly note that the problem breaks the
`make test t=gmon/tst-gmon-dso` added for Bug 29438.
There, the arc of the `f3()` call disappears from the output, since in
the DSO case, the call to `f3` is located close to the end of the
monitored range.

Signed-off-by: Леонид Юрьев (Leonid Yuriev) <leo@yuriev.ru>

Another minor error seems a related typo in the calculation of
`kcountsize`, but since kcounts are smaller than froms, this is
actually to align the p->froms data.

Co-authored-by: DJ Delorie <dj@redhat.com>

diff --git a/gmon/gmon.c b/gmon/gmon.c
index dee64803ad..bf76358d5b 100644
--- a/gmon/gmon.c
+++ b/gmon/gmon.c
@@ -132,6 +132,8 @@ __monstartup (u_long lowpc, u_long highpc)
   p->lowpc = ROUNDDOWN(lowpc, HISTFRACTION * sizeof(HISTCOUNTER));
   p->highpc = ROUNDUP(highpc, HISTFRACTION * sizeof(HISTCOUNTER));
   p->textsize = p->highpc - p->lowpc;
+  /* This looks like a typo, but it's here to align the p->froms
+     section.  */
   p->kcountsize = ROUNDUP(p->textsize / HISTFRACTION, sizeof(*p->froms));
   p->hashfraction = HASHFRACTION;
   p->log_hashfraction = -1;
@@ -142,7 +144,7 @@ __monstartup (u_long lowpc, u_long highpc)
 	 instead of integer division.  Precompute shift amount. */
       p->log_hashfraction = ffs(p->hashfraction * sizeof(*p->froms)) - 1;
   }
-  p->fromssize = p->textsize / HASHFRACTION;
+  p->fromssize = ROUNDUP(p->textsize / HASHFRACTION, sizeof(*p->froms));
   p->tolimit = p->textsize * ARCDENSITY / 100;
   if (p->tolimit < MINARCS)
     p->tolimit = MINARCS;

Re: [PATCH v2] gmon: Fix allocated buffer overflow (bug 2944)

[Carlos O'Donell]

On 2/14/23 00:59, DJ Delorie via Libc-alpha wrote:
> 
> Not getting any feedback from my review-with-questions, I'm posting a
> patch that assumes I'm right about the change I thought Leonid's patch
> needed.  Fight!  ;-)

LGTM. You can keep my Reviewed-by: if you make only the spelling and subject fix.

Please do the following:
- Post a v3 with my Reviewed-by: included.
- Push v3.
- Mark v2 superseded in patchwork.

Thank you!

Reviewed-by: Carlos O'Donell <carlos@redhat.com>


> From f7fddb025720d4e318fe5425f5a3235e73a13282 Mon Sep 17 00:00:00 2001
> From: "Леонид Юрьев (Leonid Yuriev)" <leo@yuriev.ru>
> Date: Sat, 4 Feb 2023 14:41:38 +0300
> Subject: gmon: Fix allocated buffer overflow (bug 2944)

s/bug 2944/bug 29444/g

> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> The `__monstartup()` allocates a buffer used to store all the data
> accumulated by the monitor.
> 
> The size of this buffer depends on the size of the internal structures
> used and the address range for which the monitor is activated, as well
> as on the maximum density of call instuctions and/or callable functions

s/instuctions/instructions/g

> that could be potentially on a segment of executable code.
> 
> In particular a hash table of arcs is placed at the end of this buffer.
> The size of this hash table is calculated in bytes as
>    p->fromssize = p->textsize / HASHFRACTION;
> 
> but actually should be
>    p->fromssize = ROUNDUP(p->textsize / HASHFRACTION, sizeof(*p->froms));

OK. Agreed. The size must be rounded to the size of the arc indices.

> 
> This results in writing beyond the end of the allocated buffer when an
> added arc corresponds to a call near from the end of the monitored
> address range, since `_mcount()` check the incoming caller address for
> monitored range but not the intermediate result hash-like index that
> uses to write into the table.
> 
> It should be noted that when the results are output to `gmon.out`, the
> table is read to the last element calculated from the allocated size in
> bytes, so the arcs stored outside the buffer boundary did not fall into
> `gprof` for analysis. Thus this "feature" help me to found this bug
> during working with https://sourceware.org/bugzilla/show_bug.cgi?id=29438
> 
> Just in case, I will explicitly note that the problem breaks the
> `make test t=gmon/tst-gmon-dso` added for Bug 29438.
> There, the arc of the `f3()` call disappears from the output, since in
> the DSO case, the call to `f3` is located close to the end of the
> monitored range.
> 
> Signed-off-by: Леонид Юрьев (Leonid Yuriev) <leo@yuriev.ru>
> 
> Another minor error seems a related typo in the calculation of
> `kcountsize`, but since kcounts are smaller than froms, this is
> actually to align the p->froms data.

OK.

> 
> Co-authored-by: DJ Delorie <dj@redhat.com>
> 
> diff --git a/gmon/gmon.c b/gmon/gmon.c
> index dee64803ad..bf76358d5b 100644
> --- a/gmon/gmon.c
> +++ b/gmon/gmon.c
> @@ -132,6 +132,8 @@ __monstartup (u_long lowpc, u_long highpc)
>    p->lowpc = ROUNDDOWN(lowpc, HISTFRACTION * sizeof(HISTCOUNTER));
>    p->highpc = ROUNDUP(highpc, HISTFRACTION * sizeof(HISTCOUNTER));
>    p->textsize = p->highpc - p->lowpc;
> +  /* This looks like a typo, but it's here to align the p->froms
> +     section.  */
>    p->kcountsize = ROUNDUP(p->textsize / HISTFRACTION, sizeof(*p->froms));

OK. Agreed.

>    p->hashfraction = HASHFRACTION;
>    p->log_hashfraction = -1;
> @@ -142,7 +144,7 @@ __monstartup (u_long lowpc, u_long highpc)
>  	 instead of integer division.  Precompute shift amount. */
>        p->log_hashfraction = ffs(p->hashfraction * sizeof(*p->froms)) - 1;
>    }
> -  p->fromssize = p->textsize / HASHFRACTION;
> +  p->fromssize = ROUNDUP(p->textsize / HASHFRACTION, sizeof(*p->froms));

OK. Agreed, the size you want is the rounded up size during __monstartup() calculation.
- Eventually we call calloc later in the function to allocate this size.

In other places we compute like this:
268   from_len = _gmonparam.fromssize / sizeof (*_gmonparam.froms);
- So it's expected that the allocations are multiples of sizeof(*p->froms).


>    p->tolimit = p->textsize * ARCDENSITY / 100;
>    if (p->tolimit < MINARCS)
>      p->tolimit = MINARCS;
> 

-- 
Cheers,
Carlos.