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

[Carlos O'Donell <carlos@redhat.com>]

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.