Re: [PATCH] perf: fix pipe mode read code

[Stephane Eranian]

On Fri, Feb 24, 2012 at 3:06 PM, David Ahern <dsahern@xxxxxxxxx> 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@xxxxxxxxxx>
>> Â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@xxxxxxxxxx>
>>> ---
>>>
>>> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/