Re: [PATCH v2 1/7] tracing: fprobe events: Fix possible UAF on modules

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

Oops, I have already sent this as;

https://lore.kernel.org/all/174342990779.781946.9138388479067729366.stgit@devnote2/

please ignore this.

On Tue,  1 Apr 2025 00:35:36 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Commit ac91052f0ae5 ("tracing: tprobe-events: Fix leakage of module
> refcount") moved try_module_get() from __find_tracepoint_module_cb()
> to find_tracepoint() caller, but that introduced a possible UAF
> because the module can be unloaded before try_module_get(). In this
> case, the module object should be freed too. Thus, try_module_get()
> does not only fail but may access to the freed object.
> 
> To avoid that, try_module_get() in __find_tracepoint_module_cb()
> again.
> 
> Fixes: ac91052f0ae5 ("tracing: tprobe-events: Fix leakage of module refcount")
> Cc: stable@vger.kernel.org
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_fprobe.c |   26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
> index 985ff98272da..2cd9ff1049f1 100644
> --- a/kernel/trace/trace_fprobe.c
> +++ b/kernel/trace/trace_fprobe.c
> @@ -919,9 +919,15 @@ static void __find_tracepoint_module_cb(struct tracepoint *tp, struct module *mo
>  	struct __find_tracepoint_cb_data *data = priv;
>  
>  	if (!data->tpoint && !strcmp(data->tp_name, tp->name)) {
> -		data->tpoint = tp;
> -		if (!data->mod)
> +		/* If module is not specified, try getting module refcount. */
> +		if (!data->mod && mod) {
> +			/* If failed to get refcount, ignore this tracepoint. */
> +			if (!try_module_get(mod))
> +				return;
> +
>  			data->mod = mod;
> +		}
> +		data->tpoint = tp;
>  	}
>  }
>  
> @@ -933,7 +939,11 @@ static void __find_tracepoint_cb(struct tracepoint *tp, void *priv)
>  		data->tpoint = tp;
>  }
>  
> -/* Find a tracepoint from kernel and module. */
> +/*
> + * Find a tracepoint from kernel and module. If the tracepoint is on the module,
> + * the module's refcount is incremented and returned as *@tp_mod. Thus, if it is
> + * not NULL, caller must call module_put(*tp_mod) after used the tracepoint.
> + */
>  static struct tracepoint *find_tracepoint(const char *tp_name,
>  					  struct module **tp_mod)
>  {
> @@ -962,7 +972,10 @@ static void reenable_trace_fprobe(struct trace_fprobe *tf)
>  	}
>  }
>  
> -/* Find a tracepoint from specified module. */
> +/*
> + * Find a tracepoint from specified module. In this case, this does not get the
> + * module's refcount. The caller must ensure the module is not freed.
> + */
>  static struct tracepoint *find_tracepoint_in_module(struct module *mod,
>  						    const char *tp_name)
>  {
> @@ -1169,11 +1182,6 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
>  	if (is_tracepoint) {
>  		ctx->flags |= TPARG_FL_TPOINT;
>  		tpoint = find_tracepoint(symbol, &tp_mod);
> -		/* lock module until register this tprobe. */
> -		if (tp_mod && !try_module_get(tp_mod)) {
> -			tpoint = NULL;
> -			tp_mod = NULL;
> -		}
>  		if (tpoint) {
>  			ctx->funcname = kallsyms_lookup(
>  				(unsigned long)tpoint->probestub,
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>