* Re: [PATCH] perf buildid: fix off-by-one in write_buildid()
2016-04-19 8:17 [PATCH] perf buildid: fix off-by-one in write_buildid() Andrey Ryabinin
@ 2016-04-19 13:38 ` Arnaldo Carvalho de Melo
2016-04-19 15:48 ` Andrey Ryabinin
2016-04-19 13:40 ` Arnaldo Carvalho de Melo
2016-04-27 15:28 ` [tip:perf/core] perf buildid: Fix " tip-bot for Andrey Ryabinin
2 siblings, 1 reply; 7+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-04-19 13:38 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Ingo Molnar, Peter Zijlstra, Alexander Shishkin, linux-kernel
Em Tue, Apr 19, 2016 at 11:17:27AM +0300, Andrey Ryabinin escreveu:
> write_buildid() increments 'name_len' with intention to take into account
> trailing zero byte. However, 'name_len' was already incremented in
> machine__write_buildid_table() before.
> So this leads to out-of-bounds read in do_write():
Could we keep the assumptions that for a string 's' the length is
strlen(s) and that when we want to write a string _with_ its trailing
'\0' we should use strlen(s) + 1?
I.e. I propose this patch instead, ok?
diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index 0573c2ec861d..b6ecf87bc3e3 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -261,14 +261,14 @@ static int machine__write_buildid_table(struct machine *machine, int fd)
if (dso__is_vdso(pos)) {
name = pos->short_name;
- name_len = pos->short_name_len + 1;
+ name_len = pos->short_name_len;
} else if (dso__is_kcore(pos)) {
machine__mmap_name(machine, nm, sizeof(nm));
name = nm;
- name_len = strlen(nm) + 1;
+ name_len = strlen(nm);
} else {
name = pos->long_name;
- name_len = pos->long_name_len + 1;
+ name_len = pos->long_name_len;
}
in_kernel = pos->kernel ||
> $ ./perf record sleep 0
> [ perf record: Woken up 1 times to write data ]
> =================================================================
> ==15899==ERROR: AddressSanitizer: global-buffer-overflow on address 0x00000099fc92 at pc 0x7f1aa9c7eab5 bp 0x7fff940f84d0 sp 0x7fff940f7c78
> READ of size 19 at 0x00000099fc92 thread T0
> #0 0x7f1aa9c7eab4 (/usr/lib/gcc/x86_64-pc-linux-gnu/5.3.0/libasan.so.2+0x44ab4)
> #1 0x649c5b in do_write util/header.c:67
> #2 0x649c5b in write_padded util/header.c:82
> #3 0x57e8bc in write_buildid util/build-id.c:239
> #4 0x57e8bc in machine__write_buildid_table util/build-id.c:278
> ...
>
> 0x00000099fc92 is located 0 bytes to the right of global variable '*.LC99' defined in 'util/symbol.c' (0x99fc80) of size 18
> '*.LC99' is ascii string '[kernel.kallsyms]'
> ...
>
> Shadow bytes around the buggy address:
> 0x00008012bf80: f9 f9 f9 f9 00 00 00 00 00 00 03 f9 f9 f9 f9 f9
> =>0x00008012bf90: 00 00[02]f9 f9 f9 f9 f9 00 00 00 00 00 05 f9 f9
> 0x00008012bfa0: f9 f9 f9 f9 00 03 f9 f9 f9 f9 f9 f9 00 00 00 00
>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> ---
> tools/perf/util/build-id.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> index 0573c2e..a1ff68b 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -235,7 +235,7 @@ static int write_buildid(const char *name, size_t name_len, u8 *build_id,
> if (err < 0)
> return err;
>
> - return write_padded(fd, name, name_len + 1, len);
> + return write_padded(fd, name, name_len, len);
> }
>
> static int machine__write_buildid_table(struct machine *machine, int fd)
> --
> 2.7.3
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] perf buildid: fix off-by-one in write_buildid()
2016-04-19 13:38 ` Arnaldo Carvalho de Melo
@ 2016-04-19 15:48 ` Andrey Ryabinin
0 siblings, 0 replies; 7+ messages in thread
From: Andrey Ryabinin @ 2016-04-19 15:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Ingo Molnar, Peter Zijlstra, Alexander Shishkin, linux-kernel
On 04/19/2016 04:38 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Apr 19, 2016 at 11:17:27AM +0300, Andrey Ryabinin escreveu:
>> write_buildid() increments 'name_len' with intention to take into account
>> trailing zero byte. However, 'name_len' was already incremented in
>> machine__write_buildid_table() before.
>> So this leads to out-of-bounds read in do_write():
>
> Could we keep the assumptions that for a string 's' the length is
> strlen(s) and that when we want to write a string _with_ its trailing
> '\0' we should use strlen(s) + 1?
>
> I.e. I propose this patch instead, ok?
>
Yup, looks good. Thanks.
> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> index 0573c2ec861d..b6ecf87bc3e3 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -261,14 +261,14 @@ static int machine__write_buildid_table(struct machine *machine, int fd)
>
> if (dso__is_vdso(pos)) {
> name = pos->short_name;
> - name_len = pos->short_name_len + 1;
> + name_len = pos->short_name_len;
> } else if (dso__is_kcore(pos)) {
> machine__mmap_name(machine, nm, sizeof(nm));
> name = nm;
> - name_len = strlen(nm) + 1;
> + name_len = strlen(nm);
> } else {
> name = pos->long_name;
> - name_len = pos->long_name_len + 1;
> + name_len = pos->long_name_len;
> }
>
> in_kernel = pos->kernel ||
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf buildid: fix off-by-one in write_buildid()
2016-04-19 8:17 [PATCH] perf buildid: fix off-by-one in write_buildid() Andrey Ryabinin
2016-04-19 13:38 ` Arnaldo Carvalho de Melo
@ 2016-04-19 13:40 ` Arnaldo Carvalho de Melo
2016-04-20 12:36 ` Adrian Hunter
2016-04-27 15:28 ` [tip:perf/core] perf buildid: Fix " tip-bot for Andrey Ryabinin
2 siblings, 1 reply; 7+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-04-19 13:40 UTC (permalink / raw)
To: Adrian Hunter
Cc: Andrey Ryabinin, Ingo Molnar, Peter Zijlstra, Alexander Shishkin,
linux-kernel
Em Tue, Apr 19, 2016 at 11:17:27AM +0300, Andrey Ryabinin escreveu:
> write_buildid() increments 'name_len' with intention to take into account
> trailing zero byte. However, 'name_len' was already incremented in
> machine__write_buildid_table() before.
> So this leads to out-of-bounds read in do_write():
Adrian, can you please take a look at the db-export improvements made in
this series? It'd be good to have your ack for those,
thanks!
- Arnaldo
> $ ./perf record sleep 0
> [ perf record: Woken up 1 times to write data ]
> =================================================================
> ==15899==ERROR: AddressSanitizer: global-buffer-overflow on address 0x00000099fc92 at pc 0x7f1aa9c7eab5 bp 0x7fff940f84d0 sp 0x7fff940f7c78
> READ of size 19 at 0x00000099fc92 thread T0
> #0 0x7f1aa9c7eab4 (/usr/lib/gcc/x86_64-pc-linux-gnu/5.3.0/libasan.so.2+0x44ab4)
> #1 0x649c5b in do_write util/header.c:67
> #2 0x649c5b in write_padded util/header.c:82
> #3 0x57e8bc in write_buildid util/build-id.c:239
> #4 0x57e8bc in machine__write_buildid_table util/build-id.c:278
> ...
>
> 0x00000099fc92 is located 0 bytes to the right of global variable '*.LC99' defined in 'util/symbol.c' (0x99fc80) of size 18
> '*.LC99' is ascii string '[kernel.kallsyms]'
> ...
>
> Shadow bytes around the buggy address:
> 0x00008012bf80: f9 f9 f9 f9 00 00 00 00 00 00 03 f9 f9 f9 f9 f9
> =>0x00008012bf90: 00 00[02]f9 f9 f9 f9 f9 00 00 00 00 00 05 f9 f9
> 0x00008012bfa0: f9 f9 f9 f9 00 03 f9 f9 f9 f9 f9 f9 00 00 00 00
>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> ---
> tools/perf/util/build-id.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> index 0573c2e..a1ff68b 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -235,7 +235,7 @@ static int write_buildid(const char *name, size_t name_len, u8 *build_id,
> if (err < 0)
> return err;
>
> - return write_padded(fd, name, name_len + 1, len);
> + return write_padded(fd, name, name_len, len);
> }
>
> static int machine__write_buildid_table(struct machine *machine, int fd)
> --
> 2.7.3
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf buildid: fix off-by-one in write_buildid()
2016-04-19 13:40 ` Arnaldo Carvalho de Melo
@ 2016-04-20 12:36 ` Adrian Hunter
2016-04-20 13:23 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 7+ messages in thread
From: Adrian Hunter @ 2016-04-20 12:36 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Andrey Ryabinin, Ingo Molnar, Peter Zijlstra, Alexander Shishkin,
linux-kernel
On 19/04/16 16:40, Arnaldo Carvalho de Melo wrote:
> Em Tue, Apr 19, 2016 at 11:17:27AM +0300, Andrey Ryabinin escreveu:
>> write_buildid() increments 'name_len' with intention to take into account
>> trailing zero byte. However, 'name_len' was already incremented in
>> machine__write_buildid_table() before.
>> So this leads to out-of-bounds read in do_write():
>
> Adrian, can you please take a look at the db-export improvements made in
> this series? It'd be good to have your ack for those,
You mean the patches from Chris Phlipot - yes I'll look at them.
>
> thanks!
>
> - Arnaldo
>
>> $ ./perf record sleep 0
>> [ perf record: Woken up 1 times to write data ]
>> =================================================================
>> ==15899==ERROR: AddressSanitizer: global-buffer-overflow on address 0x00000099fc92 at pc 0x7f1aa9c7eab5 bp 0x7fff940f84d0 sp 0x7fff940f7c78
>> READ of size 19 at 0x00000099fc92 thread T0
>> #0 0x7f1aa9c7eab4 (/usr/lib/gcc/x86_64-pc-linux-gnu/5.3.0/libasan.so.2+0x44ab4)
>> #1 0x649c5b in do_write util/header.c:67
>> #2 0x649c5b in write_padded util/header.c:82
>> #3 0x57e8bc in write_buildid util/build-id.c:239
>> #4 0x57e8bc in machine__write_buildid_table util/build-id.c:278
>> ...
>>
>> 0x00000099fc92 is located 0 bytes to the right of global variable '*.LC99' defined in 'util/symbol.c' (0x99fc80) of size 18
>> '*.LC99' is ascii string '[kernel.kallsyms]'
>> ...
>>
>> Shadow bytes around the buggy address:
>> 0x00008012bf80: f9 f9 f9 f9 00 00 00 00 00 00 03 f9 f9 f9 f9 f9
>> =>0x00008012bf90: 00 00[02]f9 f9 f9 f9 f9 00 00 00 00 00 05 f9 f9
>> 0x00008012bfa0: f9 f9 f9 f9 00 03 f9 f9 f9 f9 f9 f9 00 00 00 00
>>
>> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> ---
>> tools/perf/util/build-id.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
>> index 0573c2e..a1ff68b 100644
>> --- a/tools/perf/util/build-id.c
>> +++ b/tools/perf/util/build-id.c
>> @@ -235,7 +235,7 @@ static int write_buildid(const char *name, size_t name_len, u8 *build_id,
>> if (err < 0)
>> return err;
>>
>> - return write_padded(fd, name, name_len + 1, len);
>> + return write_padded(fd, name, name_len, len);
>> }
>>
>> static int machine__write_buildid_table(struct machine *machine, int fd)
>> --
>> 2.7.3
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf buildid: fix off-by-one in write_buildid()
2016-04-20 12:36 ` Adrian Hunter
@ 2016-04-20 13:23 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 7+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-04-20 13:23 UTC (permalink / raw)
To: Adrian Hunter
Cc: Andrey Ryabinin, Ingo Molnar, Peter Zijlstra, Alexander Shishkin,
linux-kernel
Em Wed, Apr 20, 2016 at 03:36:22PM +0300, Adrian Hunter escreveu:
> On 19/04/16 16:40, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Apr 19, 2016 at 11:17:27AM +0300, Andrey Ryabinin escreveu:
> >> write_buildid() increments 'name_len' with intention to take into account
> >> trailing zero byte. However, 'name_len' was already incremented in
> >> machine__write_buildid_table() before.
> >> So this leads to out-of-bounds read in do_write():
> >
> > Adrian, can you please take a look at the db-export improvements made in
> > this series? It'd be good to have your ack for those,
>
> You mean the patches from Chris Phlipot - yes I'll look at them.
Yeah, thanks,
- Arnaldo
^ permalink raw reply [flat|nested] 7+ messages in thread
* [tip:perf/core] perf buildid: Fix off-by-one in write_buildid()
2016-04-19 8:17 [PATCH] perf buildid: fix off-by-one in write_buildid() Andrey Ryabinin
2016-04-19 13:38 ` Arnaldo Carvalho de Melo
2016-04-19 13:40 ` Arnaldo Carvalho de Melo
@ 2016-04-27 15:28 ` tip-bot for Andrey Ryabinin
2 siblings, 0 replies; 7+ messages in thread
From: tip-bot for Andrey Ryabinin @ 2016-04-27 15:28 UTC (permalink / raw)
To: linux-tip-commits
Cc: acme, alexander.shishkin, tglx, hpa, aryabinin, linux-kernel,
mingo, peterz
Commit-ID: 70a2cba972e5e4a5d850e4179381f1cd344c6828
Gitweb: http://git.kernel.org/tip/70a2cba972e5e4a5d850e4179381f1cd344c6828
Author: Andrey Ryabinin <aryabinin@virtuozzo.com>
AuthorDate: Tue, 19 Apr 2016 11:17:27 +0300
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 25 Apr 2016 12:49:16 -0300
perf buildid: Fix off-by-one in write_buildid()
write_buildid() increments 'name_len' with intention to take into
account trailing zero byte. However, 'name_len' was already incremented
in machine__write_buildid_table() before. So this leads to
out-of-bounds read in do_write():
$ ./perf record sleep 0
[ perf record: Woken up 1 times to write data ]
=================================================================
==15899==ERROR: AddressSanitizer: global-buffer-overflow on address 0x00000099fc92 at pc 0x7f1aa9c7eab5 bp 0x7fff940f84d0 sp 0x7fff940f7c78
READ of size 19 at 0x00000099fc92 thread T0
#0 0x7f1aa9c7eab4 (/usr/lib/gcc/x86_64-pc-linux-gnu/5.3.0/libasan.so.2+0x44ab4)
#1 0x649c5b in do_write util/header.c:67
#2 0x649c5b in write_padded util/header.c:82
#3 0x57e8bc in write_buildid util/build-id.c:239
#4 0x57e8bc in machine__write_buildid_table util/build-id.c:278
...
0x00000099fc92 is located 0 bytes to the right of global variable '*.LC99' defined in 'util/symbol.c' (0x99fc80) of size 18
'*.LC99' is ascii string '[kernel.kallsyms]'
...
Shadow bytes around the buggy address:
0x00008012bf80: f9 f9 f9 f9 00 00 00 00 00 00 03 f9 f9 f9 f9 f9
=>0x00008012bf90: 00 00[02]f9 f9 f9 f9 f9 00 00 00 00 00 05 f9 f9
0x00008012bfa0: f9 f9 f9 f9 00 03 f9 f9 f9 f9 f9 f9 00 00 00 00
Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1461053847-5633-1-git-send-email-aryabinin@virtuozzo.com
[ Remove the off-by one at the origin, to keep len(s) == strlen(s) assumption ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/build-id.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index 0573c2e..b6ecf87 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -261,14 +261,14 @@ static int machine__write_buildid_table(struct machine *machine, int fd)
if (dso__is_vdso(pos)) {
name = pos->short_name;
- name_len = pos->short_name_len + 1;
+ name_len = pos->short_name_len;
} else if (dso__is_kcore(pos)) {
machine__mmap_name(machine, nm, sizeof(nm));
name = nm;
- name_len = strlen(nm) + 1;
+ name_len = strlen(nm);
} else {
name = pos->long_name;
- name_len = pos->long_name_len + 1;
+ name_len = pos->long_name_len;
}
in_kernel = pos->kernel ||
^ permalink raw reply related [flat|nested] 7+ messages in thread