linux-kernel - Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack tracer is ON

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: <1511440352.2719.34.camel@gmail.com>
Date:   Thu, 23 Nov 2017 14:32:32 +0200
From:   Vladislav Valtchev <vladislav.valtchev@...il.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] trace-cmd: Making stat to report when the stack
 tracer is ON

On Wed, 2017-11-22 at 14:50 -0500, Steven Rostedt wrote:
> 
> I applied the first two. Small comments about this one.

Thanks, Steven.

> > 
> >  
> > +/* Stack tracer public functions */
> > +int is_stack_tracer_enabled(void);
> 
> As this is now in the trace-cmd.h header, please rename it to:
> 
>  tracecmd_is_stack_tracer_enabled()
> 
> >  
> > -static char read_proc(void)
> > +int is_stack_tracer_enabled(void)
> >  {
> >  	char buf[1];
> >  	int fd;
> > @@ -62,8 +62,10 @@ static char read_proc(void)
> >  	close(fd);
> >  	if (n != 1)
> >  		die("error reading %s", PROC_FILE);
> > +	if (buf[0] != '0' && buf[0] != '1')
> > +		die("Invalid value '%c' in %s", buf[0], PROC_FILE);
> 
> Why kill it here? We are reading the proc file system. What happens if
> a new kernel does update this. We just broke this tool, and we don't
> break user space with kernel updates. But user space should also be
> robust for updates like this.
> 

I perfectly understand that you might want to accept values > 1, in the future.
I was concerned about using buf != '0' since that means to accept as enabled
any kind of weird values like '?', ' ', 'x', '(' etc. plus non-printable chars
as well: that feels kind-of an "unsafe" to me: if a kernel bug causes
the tracing files to contain garbage, shouldn't we complain somehow?

> Actually, what I suggest is to keep the static read_proc function, and
> simply add:
> 
> bool tracecmd_is_stack_tracer_enabled(void)
> {
> 	char buf;
> 
> 	buf = read_proc();
> 	return buf != '0';
> }
> 
> Much easier change. And handles cases where the proc file is 2 or more.
> 

Agree.
We might also add an if (!isdigit(buf)) die() before return, but I understand
that, on the other side, we might not need to check the kernel's behavior
this way. We might ultimately trust the kernel [every part of it] and save
trace-cmd's code from having a ton of verbose sanity checks like this one.

It's all about trade-offs, clearly.
Therefore, I'm fine with whatever trade-off you believe is better for trace-cmd.

Vlad