[PATCH] perf session: Correct buffer copying when peek event

[PATCH] perf session: Correct buffer copying when peek event

[Leo Yan]

When peek an event, it has a short path and a long path.  The short path
uses the session pointer "one_mmap_addr" to directly fetch event; and
the long path needs to read out the event header and the followed event
data from file and fill into the buffer pointer passed through the
argument "buf".

The issue is in the long path that it copies the event header and event
data into the same destination address which pointer "buf", this means
the event header is overwritten.  We are just lucky to run into the
short path in most cases, so we don't hit the issue in the long path.

This patch adds the offset "hdr_sz" to the pointer "buf" when copying
the event data, so that it can reserve the event header which can be
used properly by its caller.

Fixes: 5a52f33adf02 ("perf session: Add perf_session__peek_event()")
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/util/session.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 106b3d60881a..e59242c361ce 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1723,6 +1723,7 @@ int perf_session__peek_event(struct perf_session *session, off_t file_offset,
 	if (event->header.size < hdr_sz || event->header.size > buf_sz)
 		return -1;
 
+	buf += hdr_sz;
 	rest = event->header.size - hdr_sz;
 
 	if (readn(fd, buf, rest) != (ssize_t)rest)
-- 
2.25.1

Re: [PATCH] perf session: Correct buffer copying when peek event

[Adrian Hunter]

On 5/06/21 8:29 am, Leo Yan wrote:
> When peek an event, it has a short path and a long path.  The short path
> uses the session pointer "one_mmap_addr" to directly fetch event; and
> the long path needs to read out the event header and the followed event
> data from file and fill into the buffer pointer passed through the
> argument "buf".
> 
> The issue is in the long path that it copies the event header and event
> data into the same destination address which pointer "buf", this means
> the event header is overwritten.  We are just lucky to run into the
> short path in most cases, so we don't hit the issue in the long path.
> 
> This patch adds the offset "hdr_sz" to the pointer "buf" when copying
> the event data, so that it can reserve the event header which can be
> used properly by its caller.
> 
> Fixes: 5a52f33adf02 ("perf session: Add perf_session__peek_event()")
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  tools/perf/util/session.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 106b3d60881a..e59242c361ce 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1723,6 +1723,7 @@ int perf_session__peek_event(struct perf_session *session, off_t file_offset,
>  	if (event->header.size < hdr_sz || event->header.size > buf_sz)
>  		return -1;
>  
> +	buf += hdr_sz;
>  	rest = event->header.size - hdr_sz;
>  
>  	if (readn(fd, buf, rest) != (ssize_t)rest)
> 

Re: [PATCH] perf session: Correct buffer copying when peek event

[Jiri Olsa]

On Sat, Jun 05, 2021 at 01:29:57PM +0800, Leo Yan wrote:
> When peek an event, it has a short path and a long path.  The short path
> uses the session pointer "one_mmap_addr" to directly fetch event; and
> the long path needs to read out the event header and the followed event
> data from file and fill into the buffer pointer passed through the
> argument "buf".
> 
> The issue is in the long path that it copies the event header and event
> data into the same destination address which pointer "buf", this means
> the event header is overwritten.  We are just lucky to run into the
> short path in most cases, so we don't hit the issue in the long path.
> 
> This patch adds the offset "hdr_sz" to the pointer "buf" when copying
> the event data, so that it can reserve the event header which can be
> used properly by its caller.
> 
> Fixes: 5a52f33adf02 ("perf session: Add perf_session__peek_event()")
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  tools/perf/util/session.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 106b3d60881a..e59242c361ce 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1723,6 +1723,7 @@ int perf_session__peek_event(struct perf_session *session, off_t file_offset,
>  	if (event->header.size < hdr_sz || event->header.size > buf_sz)
>  		return -1;
>  
> +	buf += hdr_sz;

nice ;-)

Acked-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka

>  	rest = event->header.size - hdr_sz;
>  
>  	if (readn(fd, buf, rest) != (ssize_t)rest)
> -- 
> 2.25.1
> 

Re: [PATCH] perf session: Correct buffer copying when peek event

[Arnaldo Carvalho de Melo]

Em Tue, Jun 08, 2021 at 01:07:22PM +0200, Jiri Olsa escreveu:
> On Sat, Jun 05, 2021 at 01:29:57PM +0800, Leo Yan wrote:
> > When peek an event, it has a short path and a long path.  The short path
> > uses the session pointer "one_mmap_addr" to directly fetch event; and
> > the long path needs to read out the event header and the followed event
> > data from file and fill into the buffer pointer passed through the
> > argument "buf".
> > 
> > The issue is in the long path that it copies the event header and event
> > data into the same destination address which pointer "buf", this means
> > the event header is overwritten.  We are just lucky to run into the
> > short path in most cases, so we don't hit the issue in the long path.
> > 
> > This patch adds the offset "hdr_sz" to the pointer "buf" when copying
> > the event data, so that it can reserve the event header which can be
> > used properly by its caller.
> > 
> > Fixes: 5a52f33adf02 ("perf session: Add perf_session__peek_event()")
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  tools/perf/util/session.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> > index 106b3d60881a..e59242c361ce 100644
> > --- a/tools/perf/util/session.c
> > +++ b/tools/perf/util/session.c
> > @@ -1723,6 +1723,7 @@ int perf_session__peek_event(struct perf_session *session, off_t file_offset,
> >  	if (event->header.size < hdr_sz || event->header.size > buf_sz)
> >  		return -1;
> >  
> > +	buf += hdr_sz;
> 
> nice ;-)
> 
> Acked-by: Jiri Olsa <jolsa@redhat.com>

Thanks, applied.

- Arnaldo