linux-kernel - RE: [PATCH] perf event-parse: Use fixed size string for comms

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: <0a4dba94bdbb41929350fe5504ee24bb@AcuMS.aculab.com>
Date:   Thu, 30 Aug 2018 15:28:18 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     "'cphlipot0@...il.com'" <cphlipot0@...il.com>,
        "namhyung@...nel.org" <namhyung@...nel.org>,
        "acme@...nel.org" <acme@...nel.org>
CC:     "peterz@...radead.org" <peterz@...radead.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] perf event-parse: Use fixed size string for comms

From: cphlipot0@...il.com
> Sent: 30 August 2018 03:20
> 
> Some implementations of libc do not support the 'm' width modifier
> as part of the scanf string format specifier. This can cause the
> parsing to fail.  Since the parser never checks if the scanf
> parsing was successesful, this can result in a crash.
> 
> Change the comm string to be allocated as a fixed size instead of
> dynamically using 'm' scanf width modifier. This can be safely done
> since comm size is limited to 16 bytes by TASK_COMM_LEN within the
> kernel.
> 
> This change prevents perf from crashing when linked against bionic
> as well as reduces the total number of heap allocations and frees
> invoked while accomplishing the same task.
> 
> Signed-off-by: Chris Phlipot <cphlipot0@...il.com>
> ---
>  tools/perf/util/trace-event-parse.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/trace-event-parse.c b/tools/perf/util/trace-event-parse.c
> index 920b1d58a068..e76214f8d596 100644
> --- a/tools/perf/util/trace-event-parse.c
> +++ b/tools/perf/util/trace-event-parse.c
> @@ -164,16 +164,15 @@ void parse_ftrace_printk(struct tep_handle *pevent,
>  void parse_saved_cmdline(struct tep_handle *pevent,
>  			 char *file, unsigned int size __maybe_unused)
>  {
> -	char *comm;
> +	char comm[17]; /* Max comm length in the kernel is 16. */
>  	char *line;
>  	char *next = NULL;
>  	int pid;
> 
>  	line = strtok_r(file, "\n", &next);
>  	while (line) {
> -		sscanf(line, "%d %ms", &pid, &comm);
> -		tep_register_comm(pevent, comm, pid);
> -		free(comm);
> +		if (sscanf(line, "%d %16s", &pid, comm) == 2)
> +			tep_register_comm(pevent, comm, pid);
>  		line = strtok_r(NULL, "\n", &next);

Seems to me that sscanf is the wrong tool for the job (as usual).
Why not just:
		pid = strtoul(line, &comm, 10);
		while (*comm == ' ')
			comm++;
		tep_register_comm(pevent, comm, pid);

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)