linux-kernel - Re: [PATCH] fprobe: Release rethook after the ftrace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20230628012305.978e34d44f1a53fe20327fde@kernel.org>
Date:   Wed, 28 Jun 2023 01:23:05 +0900
From:   Masami Hiramatsu (Google) <mhiramat@...nel.org>
To:     Masami Hiramatsu (Google) <mhiramat@...nel.org>
Cc:     Jiri Olsa <jolsa@...nel.org>, Steven Rostedt <rostedt@...dmis.org>,
        Mark Rutland <mark.rutland@....com>,
        lkml <linux-kernel@...r.kernel.org>,
        linux-trace-kernel@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH] fprobe: Release rethook after the ftrace_ops is
 unregistered

On Tue, 27 Jun 2023 23:33:06 +0900
Masami Hiramatsu (Google) <mhiramat@...nel.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@...nel.org>


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