* [PATCH] perf tool: Fix use after free in filename__read_build_id
@ 2014-12-16 2:16 Mitchell Krome
2014-12-16 15:35 ` Jiri Olsa
2014-12-18 6:33 ` [tip:perf/urgent] perf symbols: " tip-bot for Mitchell Krome
0 siblings, 2 replies; 4+ messages in thread
From: Mitchell Krome @ 2014-12-16 2:16 UTC (permalink / raw)
To: acme; +Cc: mingo, paulus, a.p.zijlstra, jolsa, linux-kernel
In filename__read_build_id, phdr points to memory in buf, which gets realloced
before a call to fseek that uses phdr->p_offset. This change stores the value
of p_offset before buf is realloced, so the fseek can use the value safely.
Signed-off-by: Mitchell Krome <mitchellkrome@gmail.com>
---
tools/perf/util/symbol-minimal.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
index fa585c6..d7efb03 100644
--- a/tools/perf/util/symbol-minimal.c
+++ b/tools/perf/util/symbol-minimal.c
@@ -129,6 +129,7 @@ int filename__read_build_id(const char *filename, void *bf, size_t size)
for (i = 0, phdr = buf; i < ehdr.e_phnum; i++, phdr++) {
void *tmp;
+ long offset;
if (need_swap) {
phdr->p_type = bswap_32(phdr->p_type);
@@ -140,12 +141,13 @@ int filename__read_build_id(const char *filename, void *bf, size_t size)
continue;
buf_size = phdr->p_filesz;
+ offset = phdr->p_offset;
tmp = realloc(buf, buf_size);
if (tmp == NULL)
goto out_free;
buf = tmp;
- fseek(fp, phdr->p_offset, SEEK_SET);
+ fseek(fp, offset, SEEK_SET);
if (fread(buf, buf_size, 1, fp) != 1)
goto out_free;
@@ -178,6 +180,7 @@ int filename__read_build_id(const char *filename, void *bf, size_t size)
for (i = 0, phdr = buf; i < ehdr.e_phnum; i++, phdr++) {
void *tmp;
+ long offset;
if (need_swap) {
phdr->p_type = bswap_32(phdr->p_type);
@@ -189,12 +192,13 @@ int filename__read_build_id(const char *filename, void *bf, size_t size)
continue;
buf_size = phdr->p_filesz;
+ offset = phdr->p_offset;
tmp = realloc(buf, buf_size);
if (tmp == NULL)
goto out_free;
buf = tmp;
- fseek(fp, phdr->p_offset, SEEK_SET);
+ fseek(fp, offset, SEEK_SET);
if (fread(buf, buf_size, 1, fp) != 1)
goto out_free;
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] perf tool: Fix use after free in filename__read_build_id
2014-12-16 2:16 [PATCH] perf tool: Fix use after free in filename__read_build_id Mitchell Krome
@ 2014-12-16 15:35 ` Jiri Olsa
2014-12-16 16:34 ` Arnaldo Carvalho de Melo
2014-12-18 6:33 ` [tip:perf/urgent] perf symbols: " tip-bot for Mitchell Krome
1 sibling, 1 reply; 4+ messages in thread
From: Jiri Olsa @ 2014-12-16 15:35 UTC (permalink / raw)
To: Mitchell Krome; +Cc: acme, mingo, paulus, a.p.zijlstra, linux-kernel
On Tue, Dec 16, 2014 at 12:16:12PM +1000, Mitchell Krome wrote:
> In filename__read_build_id, phdr points to memory in buf, which gets realloced
> before a call to fseek that uses phdr->p_offset. This change stores the value
> of p_offset before buf is realloced, so the fseek can use the value safely.
>
> Signed-off-by: Mitchell Krome <mitchellkrome@gmail.com>
> ---
> tools/perf/util/symbol-minimal.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
> index fa585c6..d7efb03 100644
> --- a/tools/perf/util/symbol-minimal.c
> +++ b/tools/perf/util/symbol-minimal.c
> @@ -129,6 +129,7 @@ int filename__read_build_id(const char *filename, void *bf, size_t size)
>
> for (i = 0, phdr = buf; i < ehdr.e_phnum; i++, phdr++) {
> void *tmp;
> + long offset;
>
> if (need_swap) {
> phdr->p_type = bswap_32(phdr->p_type);
> @@ -140,12 +141,13 @@ int filename__read_build_id(const char *filename, void *bf, size_t size)
> continue;
>
> buf_size = phdr->p_filesz;
> + offset = phdr->p_offset;
> tmp = realloc(buf, buf_size);
> if (tmp == NULL)
> goto out_free;
>
> buf = tmp;
> - fseek(fp, phdr->p_offset, SEEK_SET);
> + fseek(fp, offset, SEEK_SET);
so the concern is that the realloc buf_size will be smaller
than the 'buf' offset of phdr->p_offset value, right? Anyway:
Acked-by: Jiri Olsa <jolsa@kernel.org>
jirka
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] perf tool: Fix use after free in filename__read_build_id
2014-12-16 15:35 ` Jiri Olsa
@ 2014-12-16 16:34 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 4+ messages in thread
From: Arnaldo Carvalho de Melo @ 2014-12-16 16:34 UTC (permalink / raw)
To: Jiri Olsa; +Cc: Mitchell Krome, mingo, paulus, a.p.zijlstra, linux-kernel
Em Tue, Dec 16, 2014 at 04:35:23PM +0100, Jiri Olsa escreveu:
> On Tue, Dec 16, 2014 at 12:16:12PM +1000, Mitchell Krome wrote:
> > In filename__read_build_id, phdr points to memory in buf, which gets realloced
> > before a call to fseek that uses phdr->p_offset. This change stores the value
> > of p_offset before buf is realloced, so the fseek can use the value safely.
> >
> > Signed-off-by: Mitchell Krome <mitchellkrome@gmail.com>
> > ---
> > tools/perf/util/symbol-minimal.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
> > index fa585c6..d7efb03 100644
> > --- a/tools/perf/util/symbol-minimal.c
> > +++ b/tools/perf/util/symbol-minimal.c
> > @@ -129,6 +129,7 @@ int filename__read_build_id(const char *filename, void *bf, size_t size)
> >
> > for (i = 0, phdr = buf; i < ehdr.e_phnum; i++, phdr++) {
> > void *tmp;
> > + long offset;
> >
> > if (need_swap) {
> > phdr->p_type = bswap_32(phdr->p_type);
> > @@ -140,12 +141,13 @@ int filename__read_build_id(const char *filename, void *bf, size_t size)
> > continue;
> >
> > buf_size = phdr->p_filesz;
> > + offset = phdr->p_offset;
> > tmp = realloc(buf, buf_size);
> > if (tmp == NULL)
> > goto out_free;
> >
> > buf = tmp;
> > - fseek(fp, phdr->p_offset, SEEK_SET);
> > + fseek(fp, offset, SEEK_SET);
>
> so the concern is that the realloc buf_size will be smaller
> than the 'buf' offset of phdr->p_offset value, right? Anyway:
at first I got unsure because of what realloc man page says, i.e. the
common part will have the same contents, i.e. before and after what is
in a a given offset will remain the same if in an area <= new size.
But yeah, if phdr->p_filesz < offsetof(Elf32_Phdr, p_offset) (unlikely,
I guess), then accessing phdr->p_offset after the realloc may be unsafe
(perhaps when using some off-limits memory access tool that paints freed
memory?).
Anyway, the new code is clear and more robust, applying.
> Acked-by: Jiri Olsa <jolsa@kernel.org>
Thanks,
> jirka
^ permalink raw reply [flat|nested] 4+ messages in thread
* [tip:perf/urgent] perf symbols: Fix use after free in filename__read_build_id
2014-12-16 2:16 [PATCH] perf tool: Fix use after free in filename__read_build_id Mitchell Krome
2014-12-16 15:35 ` Jiri Olsa
@ 2014-12-18 6:33 ` tip-bot for Mitchell Krome
1 sibling, 0 replies; 4+ messages in thread
From: tip-bot for Mitchell Krome @ 2014-12-18 6:33 UTC (permalink / raw)
To: linux-tip-commits
Cc: mingo, tglx, mitchellkrome, jolsa, paulus, mingo, hpa, acme,
linux-kernel, jolsa, a.p.zijlstra
Commit-ID: 7ad74b41e56e4f7f42c6b765bc44428cd09310d7
Gitweb: http://git.kernel.org/tip/7ad74b41e56e4f7f42c6b765bc44428cd09310d7
Author: Mitchell Krome <mitchellkrome@gmail.com>
AuthorDate: Tue, 16 Dec 2014 12:16:12 +1000
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 17 Dec 2014 11:58:17 -0300
perf symbols: Fix use after free in filename__read_build_id
In filename__read_build_id, phdr points to memory in buf, which gets realloced
before a call to fseek that uses phdr->p_offset. This change stores the value
of p_offset before buf is realloced, so the fseek can use the value safely.
Signed-off-by: Mitchell Krome <mitchellkrome@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20141216021612.GA7199@mitchell
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/symbol-minimal.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
index fa585c6..d7efb03 100644
--- a/tools/perf/util/symbol-minimal.c
+++ b/tools/perf/util/symbol-minimal.c
@@ -129,6 +129,7 @@ int filename__read_build_id(const char *filename, void *bf, size_t size)
for (i = 0, phdr = buf; i < ehdr.e_phnum; i++, phdr++) {
void *tmp;
+ long offset;
if (need_swap) {
phdr->p_type = bswap_32(phdr->p_type);
@@ -140,12 +141,13 @@ int filename__read_build_id(const char *filename, void *bf, size_t size)
continue;
buf_size = phdr->p_filesz;
+ offset = phdr->p_offset;
tmp = realloc(buf, buf_size);
if (tmp == NULL)
goto out_free;
buf = tmp;
- fseek(fp, phdr->p_offset, SEEK_SET);
+ fseek(fp, offset, SEEK_SET);
if (fread(buf, buf_size, 1, fp) != 1)
goto out_free;
@@ -178,6 +180,7 @@ int filename__read_build_id(const char *filename, void *bf, size_t size)
for (i = 0, phdr = buf; i < ehdr.e_phnum; i++, phdr++) {
void *tmp;
+ long offset;
if (need_swap) {
phdr->p_type = bswap_32(phdr->p_type);
@@ -189,12 +192,13 @@ int filename__read_build_id(const char *filename, void *bf, size_t size)
continue;
buf_size = phdr->p_filesz;
+ offset = phdr->p_offset;
tmp = realloc(buf, buf_size);
if (tmp == NULL)
goto out_free;
buf = tmp;
- fseek(fp, phdr->p_offset, SEEK_SET);
+ fseek(fp, offset, SEEK_SET);
if (fread(buf, buf_size, 1, fp) != 1)
goto out_free;
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-12-18 6:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-16 2:16 [PATCH] perf tool: Fix use after free in filename__read_build_id Mitchell Krome
2014-12-16 15:35 ` Jiri Olsa
2014-12-16 16:34 ` Arnaldo Carvalho de Melo
2014-12-18 6:33 ` [tip:perf/urgent] perf symbols: " tip-bot for Mitchell Krome
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.