Re: [PATCH] fprobe: Release rethook after the ftrace_ops is unregistered

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

On Tue, 27 Jun 2023 23:33:06 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> > index 18d36842faf5..0121e8c0d54e 100644
> > --- a/kernel/trace/fprobe.c
> > +++ b/kernel/trace/fprobe.c
> > @@ -364,19 +364,13 @@ int unregister_fprobe(struct fprobe *fp)
> >  		    fp->ops.saved_func != fprobe_kprobe_handler))
> >  		return -EINVAL;
> >  
> > -	/*
> > -	 * rethook_free() starts disabling the rethook, but the rethook handlers
> > -	 * may be running on other processors at this point. To make sure that all
> > -	 * current running handlers are finished, call unregister_ftrace_function()
> > -	 * after this.
> > -	 */

Oh, wait, here is an important comment. If a rethook handler is still running
(because it hooks target function exit), returning from unregister_fprobe()
right after rethook_free() may cause another issue.

rethook_free() clears 'rh->handler', so after calling rethook_free(), we
can ensure no NEW rethook handler (means fprobe_exit_handler()) is called.
However, it doesn't mean there is no current running fprobe_exit_handler().
Thus if unregister_fprobe() caller releases the 'fp' right after returning
from unregister_fprobe(), current running fprobe_exit_handler() can access
'fp' (use-after-free).

Thus we need to add below code with this patch;
	/*
	 * The rethook handlers may be running on other processors at this point.
	 * To make sure that all current running handlers are finished, disable
	 * rethook by clearing handler and call unregister_ftrace_function()
	 * to ensure all running rethook handlers exit. And call rethook_free().
	 */
	if (fp->rethook)
		WRITE_ONCE(fp->rethook->handler, NULL);

> > -	if (fp->rethook)
> > -		rethook_free(fp->rethook);
> > -
> >  	ret = unregister_ftrace_function(&fp->ops);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +	if (fp->rethook)
> > +		rethook_free(fp->rethook);
> > +
> >  	ftrace_free_filter(&fp->ops);
> >  
> >  	return ret;

Thank you,

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


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