| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 24 Feb 2012 16:17:27 +0100
From: Stephane Eranian <eranian@...gle.com>
To: David Ahern <dsahern@...il.com>
Cc: linux-kernel@...r.kernel.org, acme@...hat.com,
peterz@...radead.org, mingo@...e.hu
Subject: Re: [PATCH] perf: fix pipe mode read code
On Fri, Feb 24, 2012 at 3:06 PM, David Ahern <dsahern@...il.com> wrote:
> On 2/24/12 3:12 AM, Stephane Eranian wrote:
>>
>> Any comment on this patch?
>>
>> On Thu, Jan 19, 2012 at 6:49 PM, Stephane Eranian<eranian@...gle.com>
>> wrote:
>>>
>>>
>>> In __perf_session__process_pipe_events(), there is a risk
>>> we could read more than what a union perf_event struct can
>>> hold. This could happen when perf is reading a file which
>>> contains new and unknown record types which are larger than
>>> anything the tool already knows about (i.e. part of union
>>> perf_event).
>>>
>>> In general, perf is supposed to skip records it does not
>>> understand, but in pipe mode, those have to be read and
>>> ignored. They cannot just be skipped. In the current code,
>>> the backing for the read is provided by union perf_event.
>>> There is no check for the size limit thus there is a risk
>>> of buffer overrun:
>>>
>>> union perf_event event;
>>> void *p;
>>>
>>> size = event->header.size;
>>>
>>> p =&event;
>>> p += sizeof(struct perf_event_header);
>>> if (size - sizeof(struct perf_event_header)) {
>>> err = readn(self->fd, p, size - sizeof(struct perf_event_header));
>>>
>>> It should be noted that the same problem may occur with known
>>> record types if they have a variable size body (not captured in
>>> union perf_event).
>>>
>>> We fix this by allocating a buffer based on the size reported in
>>> the header. We reuse the buffer as much as we can. We realloc in
>>> case it becomes too small. In the common case, the performance
>>> impact is negligible.
>>>
>>> Signed-off-by: Stephane Eranian<eranian@...gle.com>
>>> ---
>>>
>>> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
>>> index b5ca255..7f078a6 100644
>>> --- a/tools/perf/util/session.c
>>> +++ b/tools/perf/util/session.c
>>> @@ -972,8 +972,9 @@ volatile int session_done;
>>> static int __perf_session__process_pipe_events(struct perf_session
>>> *self,
>>> struct perf_tool *tool)
>>> {
>>> - union perf_event event;
>>> - uint32_t size;
>>> + union perf_event *event;
>>> + uint32_t size, cur_size = 0;
>>> + void *buf = NULL;
>>> int skip = 0;
>>> u64 head;
>>> int err;
>>> @@ -982,8 +983,14 @@ static int
>>> __perf_session__process_pipe_events(struct perf_session *self,
>>> perf_tool__fill_defaults(tool);
>>>
>>> head = 0;
>>> + cur_size = sizeof(union perf_event);
>>> +
>>> + buf = malloc(cur_size);
>>> + if (!buf)
>>> + return -errno;
>>> more:
>>> - err = readn(self->fd,&event, sizeof(struct perf_event_header));
>>>
>>> + event = buf;
>>> + err = readn(self->fd, event, sizeof(struct perf_event_header));
>>> if (err<= 0) {
>>> if (err == 0)
>>> goto done;
>>> @@ -993,13 +1000,22 @@ static int
>>> __perf_session__process_pipe_events(struct perf_session *self,
>>> }
>>>
>>> if (self->header.needs_swap)
>>> - perf_event_header__bswap(&event.header);
>>> + perf_event_header__bswap(&event->header);
>>>
>>> - size = event.header.size;
>>> + size = event->header.size;
>>> if (size == 0)
>>> size = 8;
>>>
>>> - p =&event;
>>> + if (size> cur_size) {
>>> + buf = realloc(buf, size);
>
>
> Arnaldo pointed out recently this leaks memory if realloc failed. Need to
> save buf before the call ...
>
Ok, will respin the patch to fix this.
>>> + if (!buf) {
>
>
> ... and free on this leg.
>
> David
>
>>> + pr_err("failed to allocate memory to read
>>> event\n");
>>> + goto out_err;
>>> + }
>>> + cur_size = size;
>>> + event = buf;
>>> + }
>>> + p = event;
>>> p += sizeof(struct perf_event_header);
>>>
>>> if (size - sizeof(struct perf_event_header)) {
>>> @@ -1015,9 +1031,9 @@ static int
>>> __perf_session__process_pipe_events(struct perf_session *self,
>>> }
>>> }
>>>
>>> - if ((skip = perf_session__process_event(self,&event, tool,
>>> head))< 0) {
>>>
>>> + if ((skip = perf_session__process_event(self, event, tool,
>>> head))< 0) {
>>> dump_printf("%#" PRIx64 " [%#x]: skipping unknown header
>>> type: %d\n",
>>> - head, event.header.size, event.header.type);
>>> + head, event->header.size,
>>> event->header.type);
>>> /*
>>> * assume we lost track of the stream, check alignment,
>>> and
>>> * increment a single u64 in the hope to catch on again
>>> 'soon'.
>>> @@ -1038,6 +1054,7 @@ static int
>>> __perf_session__process_pipe_events(struct perf_session *self,
>>> done:
>>> err = 0;
>>> out_err:
>>> + free(buf);
>>> perf_session__warn_about_errors(self, tool);
>>> perf_session_free_sample_buffers(self);
>>> return err;
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists