linux-kernel - Re: [PATCH v5 7/9] fprobe: Add exit

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]

Date:   Tue, 25 Jan 2022 12:08:04 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     Jiri Olsa <jolsa@...hat.com>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>, netdev@...r.kernel.org,
        bpf@...r.kernel.org, lkml <linux-kernel@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        "Naveen N . Rao" <naveen.n.rao@...ux.ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
        "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH v5 7/9] fprobe: Add exit_handler support

On Tue, 25 Jan 2022 21:12:56 +0900
Masami Hiramatsu <mhiramat@...nel.org> wrote:

> Add exit_handler to fprobe. fprobe + rethook allows us
> to hook the kernel function return without fgraph tracer.
> Eventually, the fgraph tracer will be generic array based
> return hooking and fprobe may use it if user requests.
> Since both array-based approach and list-based approach
> have Pros and Cons, (e.g. memory consumption v.s. less
> missing events) it is better to keep both but fprobe
> will provide the same exit-handler interface.

Again the 55 character width ;-)

> 
> Signed-off-by: Masami Hiramatsu <mhiramat@...nel.org>
> ---
>  Changes in v5:
>   - Add dependency for HAVE_RETHOOK.
>  Changes in v4:
>   - Check fprobe is disabled in the exit handler.
>  Changes in v3:
>   - Make sure to clear rethook->data before free.
>   - Handler checks the data is not NULL.
>   - Free rethook only if the rethook is using.
> ---

> @@ -82,6 +117,7 @@ static int convert_func_addresses(struct fprobe *fp)
>   */
>  int register_fprobe(struct fprobe *fp)
>  {
> +	unsigned int i, size;
>  	int ret;
>  
>  	if (!fp || !fp->nentry || (!fp->syms && !fp->addrs) ||
> @@ -96,10 +132,29 @@ int register_fprobe(struct fprobe *fp)
>  	fp->ops.func = fprobe_handler;
>  	fp->ops.flags = FTRACE_OPS_FL_SAVE_REGS;
>  
> +	/* Initialize rethook if needed */
> +	if (fp->exit_handler) {
> +		size = fp->nentry * num_possible_cpus() * 2;
> +		fp->rethook = rethook_alloc((void *)fp, fprobe_exit_handler);

Shouldn't we check if fp->rethook succeeded to be allocated?

> +		for (i = 0; i < size; i++) {
> +			struct rethook_node *node;
> +
> +			node = kzalloc(sizeof(struct fprobe_rethook_node), GFP_KERNEL);
> +			if (!node) {
> +				rethook_free(fp->rethook);
> +				ret = -ENOMEM;
> +				goto out;
> +			}
> +			rethook_add_node(fp->rethook, node);
> +		}
> +	} else
> +		fp->rethook = NULL;
> +
>  	ret = ftrace_set_filter_ips(&fp->ops, fp->addrs, fp->nentry, 0, 0);
>  	if (!ret)
>  		ret = register_ftrace_function(&fp->ops);
>  
> +out:
>  	if (ret < 0 && fp->syms) {
>  		kfree(fp->addrs);
>  		fp->addrs = NULL;
> @@ -125,8 +180,16 @@ int unregister_fprobe(struct fprobe *fp)
>  		return -EINVAL;
>  
>  	ret = unregister_ftrace_function(&fp->ops);
> +	if (ret < 0)
> +		return ret;

If we fail to unregister the fp->ops, we do not free the allocated nodes
above?

-- Steve

>  
> -	if (!ret && fp->syms) {
> +	if (fp->rethook) {
> +		/* Make sure to clear rethook->data before freeing. */
> +		WRITE_ONCE(fp->rethook->data, NULL);
> +		barrier();
> +		rethook_free(fp->rethook);
> +	}
> +	if (fp->syms) {
>  		kfree(fp->addrs);
>  		fp->addrs = NULL;
>  	}