[RFC] perf tool: Fix memory corruption because of zero length symbols

[RFC] perf tool: Fix memory corruption because of zero length symbols

[Ravi Bangoria]

Perf top is often crashing at very random locations on powerpc.
After investigating, I found the crash only happens when sample
is of zero length symbol. Powerpc kernel has many such symbols
which does not contain length details in vmlinux binary and thus
start and end addresses of such symbols are same.

Structure

  struct sym_hist {
        u64                   nr_samples;
        u64                   period;
        struct sym_hist_entry addr[0];
  };

has last member 'addr[]' of size zero. 'addr[]' is an array of
addresses that belongs to one symbol (function). If function
consist of 100 instructions, 'addr' points to an array of 100
'struct sym_hist_entry' elements. For zero length symbol, it
points to the *empty* array, i.e. no members in the array and
thus offset 0 is also invalid for such array.

  static int __symbol__inc_addr_samples(...)
  {
        ...
        offset = addr - sym->start;
        h = annotation__histogram(notes, evidx);
        h->nr_samples++;
        h->addr[offset].nr_samples++;
        h->period += sample->period;
        h->addr[offset].period += sample->period;
        ...
  }

Here, when 'addr' is same as 'sym->start', 'offset' becomes 0,
which is valid for normal symbols but *invalid* for zero length
symbols and thus updating h->addr[offset] causes memory corruption.

Fix this by adding one dummy element for zero length symbols.

Fixes: edee44be5919 ("perf annotate: Don't throw error for zero length symbols")
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 tools/perf/util/annotate.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 4397a8b..15db525 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -606,9 +606,19 @@ static struct arch *arch__find(const char *name)
 int symbol__alloc_hist(struct symbol *sym)
 {
 	struct annotation *notes = symbol__annotation(sym);
-	const size_t size = symbol__size(sym);
+	size_t size = symbol__size(sym);
 	size_t sizeof_sym_hist;
 
+	/*
+	 * Add buffer of one element for zero length symbol.
+	 * When sample is taken from first instruction of
+	 * zero length symbol, perf still resolves it and
+	 * shows symbol name in perf report and allows to
+	 * annotate it.
+	 */
+	if (size == 0)
+		size = 1;
+
 	/* Check for overflow when calculating sizeof_sym_hist */
 	if (size > (SIZE_MAX - sizeof(struct sym_hist)) / sizeof(struct sym_hist_entry))
 		return -1;
-- 
1.8.3.1

Re: [RFC] perf tool: Fix memory corruption because of zero length symbols

[Namhyung Kim]

On Tue, Oct 24, 2017 at 07:50:06PM +0530, Ravi Bangoria wrote:
> Perf top is often crashing at very random locations on powerpc.
> After investigating, I found the crash only happens when sample
> is of zero length symbol. Powerpc kernel has many such symbols
> which does not contain length details in vmlinux binary and thus
> start and end addresses of such symbols are same.
> 
> Structure
> 
>   struct sym_hist {
>         u64                   nr_samples;
>         u64                   period;
>         struct sym_hist_entry addr[0];
>   };
> 
> has last member 'addr[]' of size zero. 'addr[]' is an array of
> addresses that belongs to one symbol (function). If function
> consist of 100 instructions, 'addr' points to an array of 100
> 'struct sym_hist_entry' elements. For zero length symbol, it
> points to the *empty* array, i.e. no members in the array and
> thus offset 0 is also invalid for such array.
> 
>   static int __symbol__inc_addr_samples(...)
>   {
>         ...
>         offset = addr - sym->start;
>         h = annotation__histogram(notes, evidx);
>         h->nr_samples++;
>         h->addr[offset].nr_samples++;
>         h->period += sample->period;
>         h->addr[offset].period += sample->period;
>         ...
>   }
> 
> Here, when 'addr' is same as 'sym->start', 'offset' becomes 0,
> which is valid for normal symbols but *invalid* for zero length
> symbols and thus updating h->addr[offset] causes memory corruption.
> 
> Fix this by adding one dummy element for zero length symbols.
> 
> Fixes: edee44be5919 ("perf annotate: Don't throw error for zero length symbols")
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung


> ---
>  tools/perf/util/annotate.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 4397a8b..15db525 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -606,9 +606,19 @@ static struct arch *arch__find(const char *name)
>  int symbol__alloc_hist(struct symbol *sym)
>  {
>  	struct annotation *notes = symbol__annotation(sym);
> -	const size_t size = symbol__size(sym);
> +	size_t size = symbol__size(sym);
>  	size_t sizeof_sym_hist;
>  
> +	/*
> +	 * Add buffer of one element for zero length symbol.
> +	 * When sample is taken from first instruction of
> +	 * zero length symbol, perf still resolves it and
> +	 * shows symbol name in perf report and allows to
> +	 * annotate it.
> +	 */
> +	if (size == 0)
> +		size = 1;
> +
>  	/* Check for overflow when calculating sizeof_sym_hist */
>  	if (size > (SIZE_MAX - sizeof(struct sym_hist)) / sizeof(struct sym_hist_entry))
>  		return -1;
> -- 
> 1.8.3.1
> 

Re: [RFC] perf tool: Fix memory corruption because of zero length symbols

[Jiri Olsa]

On Tue, Oct 24, 2017 at 07:50:06PM +0530, Ravi Bangoria wrote:
> Perf top is often crashing at very random locations on powerpc.
> After investigating, I found the crash only happens when sample
> is of zero length symbol. Powerpc kernel has many such symbols
> which does not contain length details in vmlinux binary and thus
> start and end addresses of such symbols are same.
> 
> Structure
> 
>   struct sym_hist {
>         u64                   nr_samples;
>         u64                   period;
>         struct sym_hist_entry addr[0];
>   };
> 
> has last member 'addr[]' of size zero. 'addr[]' is an array of
> addresses that belongs to one symbol (function). If function
> consist of 100 instructions, 'addr' points to an array of 100
> 'struct sym_hist_entry' elements. For zero length symbol, it
> points to the *empty* array, i.e. no members in the array and
> thus offset 0 is also invalid for such array.
> 
>   static int __symbol__inc_addr_samples(...)
>   {
>         ...
>         offset = addr - sym->start;
>         h = annotation__histogram(notes, evidx);
>         h->nr_samples++;
>         h->addr[offset].nr_samples++;
>         h->period += sample->period;
>         h->addr[offset].period += sample->period;
>         ...
>   }
> 
> Here, when 'addr' is same as 'sym->start', 'offset' becomes 0,
> which is valid for normal symbols but *invalid* for zero length
> symbols and thus updating h->addr[offset] causes memory corruption.

I think this will work, however what's the idea behind
zero sized symbols? I mean when we get samples for it,
what are they for.. is it like alias, whats the purpose?

thanks,
jirka

Re: [RFC] perf tool: Fix memory corruption because of zero length symbols

[Naveen N. Rao]

On 2017/10/25 08:28AM, Jiri Olsa wrote:
> On Tue, Oct 24, 2017 at 07:50:06PM +0530, Ravi Bangoria wrote:
> > Perf top is often crashing at very random locations on powerpc.
> > After investigating, I found the crash only happens when sample
> > is of zero length symbol. Powerpc kernel has many such symbols
> > which does not contain length details in vmlinux binary and thus
> > start and end addresses of such symbols are same.
> > 
> > Structure
> > 
> >   struct sym_hist {
> >         u64                   nr_samples;
> >         u64                   period;
> >         struct sym_hist_entry addr[0];
> >   };
> > 
> > has last member 'addr[]' of size zero. 'addr[]' is an array of
> > addresses that belongs to one symbol (function). If function
> > consist of 100 instructions, 'addr' points to an array of 100
> > 'struct sym_hist_entry' elements. For zero length symbol, it
> > points to the *empty* array, i.e. no members in the array and
> > thus offset 0 is also invalid for such array.
> > 
> >   static int __symbol__inc_addr_samples(...)
> >   {
> >         ...
> >         offset = addr - sym->start;
> >         h = annotation__histogram(notes, evidx);
> >         h->nr_samples++;
> >         h->addr[offset].nr_samples++;
> >         h->period += sample->period;
> >         h->addr[offset].period += sample->period;
> >         ...
> >   }
> > 
> > Here, when 'addr' is same as 'sym->start', 'offset' becomes 0,
> > which is valid for normal symbols but *invalid* for zero length
> > symbols and thus updating h->addr[offset] causes memory corruption.
> 
> I think this will work, however what's the idea behind
> zero sized symbols? I mean when we get samples for it,
> what are they for.. is it like alias, whats the purpose?

This was discussed here:
https://lkml.org/lkml/2016/10/10/148

We aren't setting the size for our assembly routines.

- Naveen

Re: [RFC] perf tool: Fix memory corruption because of zero length symbols

[Jiri Olsa]

On Tue, Oct 24, 2017 at 07:50:06PM +0530, Ravi Bangoria wrote:
> Perf top is often crashing at very random locations on powerpc.
> After investigating, I found the crash only happens when sample
> is of zero length symbol. Powerpc kernel has many such symbols
> which does not contain length details in vmlinux binary and thus
> start and end addresses of such symbols are same.
> 
> Structure
> 
>   struct sym_hist {
>         u64                   nr_samples;
>         u64                   period;
>         struct sym_hist_entry addr[0];
>   };
> 
> has last member 'addr[]' of size zero. 'addr[]' is an array of
> addresses that belongs to one symbol (function). If function
> consist of 100 instructions, 'addr' points to an array of 100
> 'struct sym_hist_entry' elements. For zero length symbol, it
> points to the *empty* array, i.e. no members in the array and
> thus offset 0 is also invalid for such array.
> 
>   static int __symbol__inc_addr_samples(...)
>   {
>         ...
>         offset = addr - sym->start;
>         h = annotation__histogram(notes, evidx);
>         h->nr_samples++;
>         h->addr[offset].nr_samples++;
>         h->period += sample->period;
>         h->addr[offset].period += sample->period;
>         ...
>   }
> 
> Here, when 'addr' is same as 'sym->start', 'offset' becomes 0,
> which is valid for normal symbols but *invalid* for zero length
> symbols and thus updating h->addr[offset] causes memory corruption.
> 
> Fix this by adding one dummy element for zero length symbols.
> 
> Fixes: edee44be5919 ("perf annotate: Don't throw error for zero length symbols")
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

Re: [RFC] perf tool: Fix memory corruption because of zero length symbols

[Arnaldo Carvalho de Melo]

Em Wed, Oct 25, 2017 at 02:50:33PM +0530, Naveen N. Rao escreveu:
> On 2017/10/25 08:28AM, Jiri Olsa wrote:
> > On Tue, Oct 24, 2017 at 07:50:06PM +0530, Ravi Bangoria wrote:
> > > Perf top is often crashing at very random locations on powerpc.
> > > After investigating, I found the crash only happens when sample
> > > is of zero length symbol. Powerpc kernel has many such symbols
> > > which does not contain length details in vmlinux binary and thus
> > > start and end addresses of such symbols are same.
> > > 
> > > Structure
> > > 
> > >   struct sym_hist {
> > >         u64                   nr_samples;
> > >         u64                   period;
> > >         struct sym_hist_entry addr[0];
> > >   };
> > > 
> > > has last member 'addr[]' of size zero. 'addr[]' is an array of
> > > addresses that belongs to one symbol (function). If function
> > > consist of 100 instructions, 'addr' points to an array of 100
> > > 'struct sym_hist_entry' elements. For zero length symbol, it
> > > points to the *empty* array, i.e. no members in the array and
> > > thus offset 0 is also invalid for such array.
> > > 
> > >   static int __symbol__inc_addr_samples(...)
> > >   {
> > >         ...
> > >         offset = addr - sym->start;
> > >         h = annotation__histogram(notes, evidx);
> > >         h->nr_samples++;
> > >         h->addr[offset].nr_samples++;
> > >         h->period += sample->period;
> > >         h->addr[offset].period += sample->period;
> > >         ...
> > >   }
> > > 
> > > Here, when 'addr' is same as 'sym->start', 'offset' becomes 0,
> > > which is valid for normal symbols but *invalid* for zero length
> > > symbols and thus updating h->addr[offset] causes memory corruption.
> > 
> > I think this will work, however what's the idea behind
> > zero sized symbols? I mean when we get samples for it,
> > what are they for.. is it like alias, whats the purpose?
> 
> This was discussed here:
> https://lkml.org/lkml/2016/10/10/148
> 
> We aren't setting the size for our assembly routines.

Applying, after adding this as a Link: tag and Namhyung's and Jiri's
acked-by, will try to have it in perf/urgent soon, i.e. for 4.14, if
still possible.

- Arnaldo

[tip:perf/urgent] perf symbols: Fix memory corruption because of zero length symbols

[tip-bot for Ravi Bangoria]

Commit-ID:  331c7cb307971eac38e9470340e10c87855bf4bc
Gitweb:     https://git.kernel.org/tip/331c7cb307971eac38e9470340e10c87855bf4bc
Author:     Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
AuthorDate: Tue, 24 Oct 2017 19:50:06 +0530
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 25 Oct 2017 13:01:09 -0300

perf symbols: Fix memory corruption because of zero length symbols

Perf top is often crashing at very random locations on powerpc.  After
investigating, I found the crash only happens when sample is of zero
length symbol. Powerpc kernel has many such symbols which does not
contain length details in vmlinux binary and thus start and end
addresses of such symbols are same.

Structure

  struct sym_hist {
        u64                   nr_samples;
        u64                   period;
        struct sym_hist_entry addr[0];
  };

has last member 'addr[]' of size zero. 'addr[]' is an array of addresses
that belongs to one symbol (function). If function consist of 100
instructions, 'addr' points to an array of 100 'struct sym_hist_entry'
elements. For zero length symbol, it points to the *empty* array, i.e.
no members in the array and thus offset 0 is also invalid for such
array.

  static int __symbol__inc_addr_samples(...)
  {
        ...
        offset = addr - sym->start;
        h = annotation__histogram(notes, evidx);
        h->nr_samples++;
        h->addr[offset].nr_samples++;
        h->period += sample->period;
        h->addr[offset].period += sample->period;
        ...
  }

Here, when 'addr' is same as 'sym->start', 'offset' becomes 0, which is
valid for normal symbols but *invalid* for zero length symbols and thus
updating h->addr[offset] causes memory corruption.

Fix this by adding one dummy element for zero length symbols.

Link: https://lkml.org/lkml/2016/10/10/148
Fixes: edee44be5919 ("perf annotate: Don't throw error for zero length symbols")
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Kim Phillips <kim.phillips@arm.com>
Cc: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Taeung Song <treeze.taeung@gmail.com>
Link: http://lkml.kernel.org/r/1508854806-10542-1-git-send-email-ravi.bangoria@linux.vnet.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/annotate.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 4397a8b..aa66791 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -606,9 +606,19 @@ static struct arch *arch__find(const char *name)
 int symbol__alloc_hist(struct symbol *sym)
 {
 	struct annotation *notes = symbol__annotation(sym);
-	const size_t size = symbol__size(sym);
+	size_t size = symbol__size(sym);
 	size_t sizeof_sym_hist;
 
+	/*
+	 * Add buffer of one element for zero length symbol.
+	 * When sample is taken from first instruction of
+	 * zero length symbol, perf still resolves it and
+	 * shows symbol name in perf report and allows to
+	 * annotate it.
+	 */
+	if (size == 0)
+		size = 1;
+
 	/* Check for overflow when calculating sizeof_sym_hist */
 	if (size > (SIZE_MAX - sizeof(struct sym_hist)) / sizeof(struct sym_hist_entry))
 		return -1;