[patch] perf tool divide by zero error if f_header.attr_size==0

[patch] perf tool divide by zero error if f_header.attr_size==0

[Vince Weaver]

Hello

so I have been having lots of trouble with hand-crafted perf.data files 
causing segfaults and the like, so I have started fuzzing the perf tool.

First issue found:

If f_header.attr_size is 0 in the perf.data file, then perf will crash
with a divide-by-zero error.

Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index c24db7f4909c..26df60ee9460 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -3559,6 +3559,10 @@ int perf_session__read_header(struct perf_session *session)
 			   data->file.path);
 	}
 
+	if (f_header.attr_size == 0) {
+		return -EINVAL;
+	}
+
 	nr_attrs = f_header.attrs.size / f_header.attr_size;
 	lseek(fd, f_header.attrs.offset, SEEK_SET);
 

Re: [patch] perf tool divide by zero error if f_header.attr_size==0

[Arnaldo Carvalho de Melo]

Em Tue, Jul 23, 2019 at 11:06:01AM -0400, Vince Weaver escreveu:
> Hello
> 
> so I have been having lots of trouble with hand-crafted perf.data files 
> causing segfaults and the like, so I have started fuzzing the perf tool.
> 
> First issue found:
> 
> If f_header.attr_size is 0 in the perf.data file, then perf will crash
> with a divide-by-zero error.

Thanks for the patch, will be in my next perf/urgent pull req.

- Arnaldo
 
> Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index c24db7f4909c..26df60ee9460 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -3559,6 +3559,10 @@ int perf_session__read_header(struct perf_session *session)
>  			   data->file.path);
>  	}
>  
> +	if (f_header.attr_size == 0) {
> +		return -EINVAL;
> +	}
> +
>  	nr_attrs = f_header.attrs.size / f_header.attr_size;
>  	lseek(fd, f_header.attrs.offset, SEEK_SET);

[patch] perf tool buffer overflow in perf_header__read_build_ids

[Vince Weaver]

Hello

my perf_tool_fuzzer has found another issue, this one a buffer overflow
in perf_header__read_build_ids.  The build id filename is read in with a 
filename length read from the perf.data file, but this can be longer than
PATH_MAX which will smash the stack.

This might not be the right fix, not sure if filename should be NUL
terminated or not.

Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index c24db7f4909c..9a893a26e678 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2001,6 +2001,9 @@ static int perf_header__read_build_ids(struct perf_header *header,
 			perf_event_header__bswap(&bev.header);
 
 		len = bev.header.size - sizeof(bev);
+
+		if (len>PATH_MAX) len=PATH_MAX;
+
 		if (readn(input, filename, len) != len)
 			goto out;
 		/*

Re: [patch] perf tool divide by zero error if f_header.attr_size==0

[Arnaldo Carvalho de Melo]

Em Tue, Jul 23, 2019 at 11:06:01AM -0400, Vince Weaver escreveu:
> Hello
> 
> so I have been having lots of trouble with hand-crafted perf.data files 
> causing segfaults and the like, so I have started fuzzing the perf tool.
> 
> First issue found:
> 
> If f_header.attr_size is 0 in the perf.data file, then perf will crash
> with a divide-by-zero error.
> 
> Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>


I added this on top:

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 25f89d0790fe..47877f0f6667 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -3560,6 +3560,9 @@ int perf_session__read_header(struct perf_session *session)
        }

        if (f_header.attr_size == 0) {
+               pr_err("ERROR: The %s file's attr size field is 0 which is unexpected.\n"
+                      "Was the 'perf record' command properly terminated?\n",
+                      data->file.path);
                return -EINVAL;
        }

[acme@quaco perf]$

Thanks, applied.

- Arnaldo

> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index c24db7f4909c..26df60ee9460 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -3559,6 +3559,10 @@ int perf_session__read_header(struct perf_session *session)
>  			   data->file.path);
>  	}
>  
> +	if (f_header.attr_size == 0) {
> +		return -EINVAL;
> +	}
> +
>  	nr_attrs = f_header.attrs.size / f_header.attr_size;
>  	lseek(fd, f_header.attrs.offset, SEEK_SET);
>  

-- 

- Arnaldo

Re: [patch] perf tool buffer overflow in perf_header__read_build_ids

[Arnaldo Carvalho de Melo]

Em Tue, Jul 23, 2019 at 04:42:30PM -0400, Vince Weaver escreveu:
> Hello
> 
> my perf_tool_fuzzer has found another issue, this one a buffer overflow
> in perf_header__read_build_ids.  The build id filename is read in with a 
> filename length read from the perf.data file, but this can be longer than
> PATH_MAX which will smash the stack.
> 
> This might not be the right fix, not sure if filename should be NUL
> terminated or not.
> 
> Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index c24db7f4909c..9a893a26e678 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -2001,6 +2001,9 @@ static int perf_header__read_build_ids(struct perf_header *header,
>  			perf_event_header__bswap(&bev.header);
>  
>  		len = bev.header.size - sizeof(bev);
> +
> +		if (len>PATH_MAX) len=PATH_MAX;
> +

Humm, I wonder if we shouldn't just declare the whole file invalid like
you did with the previous patch?

- Arnaldo

>  		if (readn(input, filename, len) != len)
>  			goto out;
>  		/*

[tip:perf/urgent] perf header: Fix divide by zero error if f_header.attr_size==0

[tip-bot for Vince Weaver]

Commit-ID:  7622236ceb167aa3857395f9bdaf871442aa467e
Gitweb:     https://git.kernel.org/tip/7622236ceb167aa3857395f9bdaf871442aa467e
Author:     Vince Weaver <vincent.weaver@maine.edu>
AuthorDate: Tue, 23 Jul 2019 11:06:01 -0400
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 29 Jul 2019 09:03:43 -0300

perf header: Fix divide by zero error if f_header.attr_size==0

So I have been having lots of trouble with hand-crafted perf.data files
causing segfaults and the like, so I have started fuzzing the perf tool.

First issue found:

If f_header.attr_size is 0 in the perf.data file, then perf will crash
with a divide-by-zero error.

Committer note:

Added a pr_err() to tell the user why the command failed.

Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/alpine.DEB.2.21.1907231100440.14532@macbook-air
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/header.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 20111f8da5cb..47877f0f6667 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -3559,6 +3559,13 @@ int perf_session__read_header(struct perf_session *session)
 			   data->file.path);
 	}
 
+	if (f_header.attr_size == 0) {
+		pr_err("ERROR: The %s file's attr size field is 0 which is unexpected.\n"
+		       "Was the 'perf record' command properly terminated?\n",
+		       data->file.path);
+		return -EINVAL;
+	}
+
 	nr_attrs = f_header.attrs.size / f_header.attr_size;
 	lseek(fd, f_header.attrs.offset, SEEK_SET);
 

Re: [patch] perf tool buffer overflow in perf_header__read_build_ids

[Vince Weaver]

On Fri, 26 Jul 2019, Arnaldo Carvalho de Melo wrote:

> Em Tue, Jul 23, 2019 at 04:42:30PM -0400, Vince Weaver escreveu:
> > my perf_tool_fuzzer has found another issue, this one a buffer overflow
> > in perf_header__read_build_ids.  The build id filename is read in with a 
> > filename length read from the perf.data file, but this can be longer than
> > PATH_MAX which will smash the stack.
> > 
> > This might not be the right fix, not sure if filename should be NUL
> > terminated or not.
> > 
> > Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>
> > 
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index c24db7f4909c..9a893a26e678 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -2001,6 +2001,9 @@ static int perf_header__read_build_ids(struct perf_header *header,
> >  			perf_event_header__bswap(&bev.header);
> >  
> >  		len = bev.header.size - sizeof(bev);
> > +
> > +		if (len>PATH_MAX) len=PATH_MAX;
> > +
> 
> Humm, I wonder if we shouldn't just declare the whole file invalid like
> you did with the previous patch?
> 
> - Arnaldo
> 
> >  		if (readn(input, filename, len) != len)
> >  			goto out;
> >  		/*
 
did we ever decide how to fix this issue?  Or were you waiting on a 
followup patch from me?

This is actually an exploitable security bug if you can convince someone 
to run "perf" on an untrusted perf.data file.

Vince

Re: [patch] perf tool buffer overflow in perf_header__read_build_ids

[Arnaldo Carvalho de Melo]

Em Fri, Aug 23, 2019 at 04:42:47PM -0400, Vince Weaver escreveu:
> On Fri, 26 Jul 2019, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Jul 23, 2019 at 04:42:30PM -0400, Vince Weaver escreveu:
> > > my perf_tool_fuzzer has found another issue, this one a buffer overflow
> > > in perf_header__read_build_ids.  The build id filename is read in with a 
> > > filename length read from the perf.data file, but this can be longer than
> > > PATH_MAX which will smash the stack.
> > > 
> > > This might not be the right fix, not sure if filename should be NUL
> > > terminated or not.
> > > 
> > > Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>
> > > 
> > > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > > index c24db7f4909c..9a893a26e678 100644
> > > --- a/tools/perf/util/header.c
> > > +++ b/tools/perf/util/header.c
> > > @@ -2001,6 +2001,9 @@ static int perf_header__read_build_ids(struct perf_header *header,
> > >  			perf_event_header__bswap(&bev.header);
> > >  
> > >  		len = bev.header.size - sizeof(bev);
> > > +
> > > +		if (len>PATH_MAX) len=PATH_MAX;
> > > +
> > 
> > Humm, I wonder if we shouldn't just declare the whole file invalid like
> > you did with the previous patch?

> > >  		if (readn(input, filename, len) != len)
> > >  			goto out;
> > >  		/*
>  
> did we ever decide how to fix this issue?  Or were you waiting on a 
> followup patch from me?

Fell thru the cracks, but yeah, I was waiting for a patch, can you send
it?

- Arnaldo
 
> This is actually an exploitable security bug if you can convince someone 
> to run "perf" on an untrusted perf.data file.

Indeed, and in light of the current discussion about unprivileged eBPF I
think we should start dropping privileges in perf report, etc.

- Arnaldo