[2/2] perf trace: Handle NULL pointer dereference in trace__syscall_info()
diff mbox series

Message ID 20190617091140.24372-2-leo.yan@linaro.org
State Not Applicable
Headers show
Series
  • [1/2] perf trace: Use pr_debug() instead of fprintf() for logging
Related show

Commit Message

Leo Yan June 17, 2019, 9:11 a.m. UTC
trace__init_bpf_map_syscall_args() invokes trace__syscall_info() to
retrieve system calls information, it always passes NULL for 'evsel'
argument; when id is an invalid value then the logging will try to
output event name, this triggers NULL pointer dereference.

This patch directly uses string "unknown" for event name when 'evsel'
is NULL pointer.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/builtin-trace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Arnaldo Carvalho de Melo June 17, 2019, 5:32 p.m. UTC | #1
Em Mon, Jun 17, 2019 at 05:11:40PM +0800, Leo Yan escreveu:
> trace__init_bpf_map_syscall_args() invokes trace__syscall_info() to
> retrieve system calls information, it always passes NULL for 'evsel'
> argument; when id is an invalid value then the logging will try to
> output event name, this triggers NULL pointer dereference.
> 
> This patch directly uses string "unknown" for event name when 'evsel'
> is NULL pointer.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  tools/perf/builtin-trace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 5cd74651db4c..49dfb2fd393b 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -1764,7 +1764,7 @@ static struct syscall *trace__syscall_info(struct trace *trace,
>  		static u64 n;
>  
>  		pr_debug("Invalid syscall %d id, skipping (%s, %" PRIu64 ")\n",
> -			 id, perf_evsel__name(evsel), ++n);
> +			 id, evsel ? perf_evsel__name(evsel) : "unknown", ++n);
>  		return NULL;

What do you think of this instead?

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 68beef8f47ff..1d6af95b9207 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -590,6 +590,9 @@ const char *perf_evsel__name(struct perf_evsel *evsel)
 {
 	char bf[128];
 
+	if (!evsel)
+		goto out_unknown;
+
 	if (evsel->name)
 		return evsel->name;
 
@@ -629,7 +632,10 @@ const char *perf_evsel__name(struct perf_evsel *evsel)
 
 	evsel->name = strdup(bf);
 
-	return evsel->name ?: "unknown";
+	if (evsel->name)
+		return evsel->name;
+out_unknown:
+	return "unknown";
 }
 
 const char *perf_evsel__group_name(struct perf_evsel *evsel)
Leo Yan June 18, 2019, 6:39 a.m. UTC | #2
On Mon, Jun 17, 2019 at 02:32:03PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jun 17, 2019 at 05:11:40PM +0800, Leo Yan escreveu:
> > trace__init_bpf_map_syscall_args() invokes trace__syscall_info() to
> > retrieve system calls information, it always passes NULL for 'evsel'
> > argument; when id is an invalid value then the logging will try to
> > output event name, this triggers NULL pointer dereference.
> > 
> > This patch directly uses string "unknown" for event name when 'evsel'
> > is NULL pointer.
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  tools/perf/builtin-trace.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > index 5cd74651db4c..49dfb2fd393b 100644
> > --- a/tools/perf/builtin-trace.c
> > +++ b/tools/perf/builtin-trace.c
> > @@ -1764,7 +1764,7 @@ static struct syscall *trace__syscall_info(struct trace *trace,
> >  		static u64 n;
> >  
> >  		pr_debug("Invalid syscall %d id, skipping (%s, %" PRIu64 ")\n",
> > -			 id, perf_evsel__name(evsel), ++n);
> > +			 id, evsel ? perf_evsel__name(evsel) : "unknown", ++n);
> >  		return NULL;
> 
> What do you think of this instead?

Yes, I agree the below change is right thing to do.  FWIW:

Reviewed-by: Leo Yan <leo.yan@linaro.org>

BTW, my patch followed the code in [1], after apply below your change,
could consider to simplify code in [1] for without checking 'evsel' is
NULL pointer anymore.

Thanks,
Leo

[1] https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/builtin-report.c?h=perf/core#n301

> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 68beef8f47ff..1d6af95b9207 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -590,6 +590,9 @@ const char *perf_evsel__name(struct perf_evsel *evsel)
>  {
>  	char bf[128];
>  
> +	if (!evsel)
> +		goto out_unknown;
> +
>  	if (evsel->name)
>  		return evsel->name;
>  
> @@ -629,7 +632,10 @@ const char *perf_evsel__name(struct perf_evsel *evsel)
>  
>  	evsel->name = strdup(bf);
>  
> -	return evsel->name ?: "unknown";
> +	if (evsel->name)
> +		return evsel->name;
> +out_unknown:
> +	return "unknown";
>  }
>  
>  const char *perf_evsel__group_name(struct perf_evsel *evsel)

Patch
diff mbox series

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 5cd74651db4c..49dfb2fd393b 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1764,7 +1764,7 @@  static struct syscall *trace__syscall_info(struct trace *trace,
 		static u64 n;
 
 		pr_debug("Invalid syscall %d id, skipping (%s, %" PRIu64 ")\n",
-			 id, perf_evsel__name(evsel), ++n);
+			 id, evsel ? perf_evsel__name(evsel) : "unknown", ++n);
 		return NULL;
 	}