Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 28 Jun 2023 01:23:05 +0900
From:   Masami Hiramatsu (Google) <mhiramat@kernel.org>
To:     Masami Hiramatsu (Google) <mhiramat@kernel.org>
Cc:     Jiri Olsa <jolsa@kernel.org>, Steven Rostedt <rostedt@goodmis.org>,
        Mark Rutland <mark.rutland@arm.com>,
        lkml <linux-kernel@vger.kernel.org>,
        linux-trace-kernel@vger.kernel.org, bpf@vger.kernel.org
Subject: Re: [PATCH] fprobe: Release rethook after the ftrace_ops is
 unregistered
Message-Id: <20230628012305.978e34d44f1a53fe20327fde@kernel.org>
In-Reply-To: <20230627233306.b9b04d75f86944466f6534c2@kernel.org>
References: <20230615115236.3476617-1-jolsa@kernel.org>
        <20230627233306.b9b04d75f86944466f6534c2@kernel.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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>