Re: [PATCH 2/2] tracing: Add a verifier to check string pointers for trace events

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 7 Jun 2021 16:02:27 +0000
Sean Christopherson <seanjc@google.com> wrote:

> I assumed that was the gist of the unsafe string detection, but the module core
> data exemption confused me.  I take it that the tracepoint itself goes away if
> the module is unloaded?  I.e. it's safe for a module to pass a constant string to
> its own tracepoints, but not to tracepoints defined elsewhere?  The comment
> above tracepoint_string() seems to confirm this.


If a trace event of a module was enabled, a flag is set, so that when
that module is unloaded, the ring buffer that it was enabled in is
cleared. That's because there's more data that is referenced about the
trace event then just strings. And referencing that data is dangerous
if it no longer exists.

> 
> > There's two fixes you can do with this. One is to covert that to use
> > __string, the other is to do what RCU does, and use the
> > tracepoint_string() functionality.

I realize now that tracepoint_string() is not a work around. It only is
or exporting the string to user space. The __string option is really
the only fix here.



> This doesn't appear to be correct.  Were you thinking of something else?
> 
> Unless I'm misreading the code and section output, tracepoint_string() just saves
> a pointer to the string into a dedicated section, it doesn't magically hoist the
> string itself into the kernel proper.  And I can't see how that would work, e.g.
> if the module is compiled and linked independent from the core kernel.

That is correct. I was mistaken about it. It has some code to track
unloading, and I was thinking it saved the string itself, but that is
not correct.

> 
> And the comment above the macro strongly suggests that persistence needs to be
> guaranteed by the entity using tracepoint_string().  Testing bears this out, e.g.
> the WARN and UNSAFE_MEMORY errors still happen when using tracepoint_string().

That is correct. I misspoke when I suggested using it. :-/


> 
>  * The @str used must be a constant string and persistent as it would not
>  * make sense to show a string that no longer exists. But it is still fine
>  * to be used with modules, because when modules are unloaded, if they
>  * had tracepoints, the ring buffers are cleared too. As long as the string
>  * does not change during the life of the module, it is fine to use
>  * tracepoint_string() within a module.
>  */
> #define tracepoint_string(str)						\
> 	({								\
> 		static const char *___tp_str __tracepoint_string = str; \
> 		___tp_str;						\
> 	})
> 
> 
> Thanks for the help!

No problem.

-- Steve