Re: [RFC PATCH 1/1] tracepoints: tree-wide: Replace %p with %px

[Masami Hiramatsu <mhiramat@kernel.org>]

On Wed, 14 Oct 2020 09:38:13 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 14 Oct 2020 17:59:19 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > To help debugging kernel, use %px to show real addresses on
> > tracefs/trace file.
> > 
> > Since ftrace human-readable format uses vsprintf(), all %p are
> > translated to hash values instead of pointer address.
> > 
> > However, when debugging the kernel, raw address value gives a
> > hint when comparing with the memory mapping in the kernel.
> > (Those are sometimes used with crash log, which is not hashed too)
> > 
> > Moreover, this is not improving the security because the tracefs
> > can be used only by root user and the raw address values are readable
> > from tracefs/percpu/cpu*/trace_pipe_raw file.
> > 
> > Note that this has been done by the following script.
> > 
> >  #!/bin/sh
> >  tmp=`mktemp`
> >  for h in include/trace/events/*.h ; do
> >    sed -e 's/\(%p\)\([^a-zA-Z]\)/\1x\2/g' $h > $tmp
> >    cp $tmp $h
> >  done
> >  rm $tmp
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> 
> Hi Masami,
> 
> I think a better approach is to inject on the output side a conversion of
> "%p" to "%px" before printing. That is, in the trace_raw_output_##call()
> function, instead of calling trace_seq_printf(s, print), we call a new
> function trace_event_printf(s, print), that will take the fmt parameter,
> and copies it to something that does your 's/\(%p\)\([^a-zA-Z]\)/\1x\2/g'
> inline before passing it off to trace_seq_printf().

Hmm, would you mean we always run such conversion on printing the trace
buffer for each entry? It could be much overhead because we need allocate
memory (%p->%px increase 1 byte) and format conversion (with copying it).
Maybe we can avoid repeating it using a kind of cache, but it also consumes
memory. And as I pointed, the security reason is meaningless because there
are raw addresses in raw data which user can read...
Could you tell me what is your point? Making the code change as small as
possible?

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>