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

[Stephane Eranian]

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);
> + Â Â Â Â Â Â Â if (!buf) {
> + Â Â Â Â Â Â Â Â Â Â Â 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/