linux-kernel - Re: [PATCH] perf: fix pipe mode read code

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CABPqkBRZu3sb3bRfruix7-HS-1qHkh2CqwXNRGjGu-y6O4ywbA@mail.gmail.com>
Date:	Fri, 24 Feb 2012 11:12:25 +0100
From:	Stephane Eranian <eranian@...gle.com>
To:	linux-kernel@...r.kernel.org
Cc:	acme@...hat.com, peterz@...radead.org, mingo@...e.hu,
	dsahern@...il.com
Subject: Re: [PATCH] perf: fix pipe mode read code

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);
> +               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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/