Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Tue, 12 Dec 2023 11:35:46 -0500
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Zheng Yejian <zhengyejian1@huawei.com>
Cc:     <mhiramat@kernel.org>, <mathieu.desnoyers@efficios.com>,
        <linux-kernel@vger.kernel.org>,
        <linux-trace-kernel@vger.kernel.org>, <yeweihua4@huawei.com>
Subject: Re: [PATCH] tracing: Fix uaf issue when open the hist or hist_debug
 file
Message-ID: <20231212113546.6a51d359@gandalf.local.home>
In-Reply-To: <20231212113317.4159890-1-zhengyejian1@huawei.com>
References: <20231212113317.4159890-1-zhengyejian1@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 12 Dec 2023 19:33:17 +0800
Zheng Yejian <zhengyejian1@huawei.com> wrote:

> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 1abc07fba1b9..00447ea7dabd 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -5623,10 +5623,12 @@ static int event_hist_open(struct inode *inode, struct file *file)
>  {
>  	int ret;
>  
> -	ret = security_locked_down(LOCKDOWN_TRACEFS);
> +	ret = tracing_open_file_tr(inode, file);
>  	if (ret)
>  		return ret;
>  
> +	/* Clear private_data to avoid warning in single_open */
> +	file->private_data = NULL;
>  	return single_open(file, hist_show, file);
>  }
>  
> @@ -5634,7 +5636,7 @@ const struct file_operations event_hist_fops = {
>  	.open = event_hist_open,
>  	.read = seq_read,
>  	.llseek = seq_lseek,
> -	.release = single_release,
> +	.release = tracing_release_file_tr,

single_release() still needs to be called. This can't simply be replaced
with tracing_release_file_tr().

>  };
>  
>  #ifdef CONFIG_HIST_TRIGGERS_DEBUG
> @@ -5900,10 +5902,12 @@ static int event_hist_debug_open(struct inode *inode, struct file *file)
>  {
>  	int ret;
>  
> -	ret = security_locked_down(LOCKDOWN_TRACEFS);
> +	ret = tracing_open_file_tr(inode, file);
>  	if (ret)
>  		return ret;
>  
> +	/* Clear private_data to avoid warning in single_open */
> +	file->private_data = NULL;
>  	return single_open(file, hist_debug_show, file);
>  }
>  
> @@ -5911,7 +5915,7 @@ const struct file_operations event_hist_debug_fops = {
>  	.open = event_hist_debug_open,
>  	.read = seq_read,
>  	.llseek = seq_lseek,
> -	.release = single_release,
> +	.release = tracing_release_file_tr,

Same here. This just causes a leak of the single resources.

What needs to be done is to add a:

tracing_single_release_file_tr()

That does both:

int tracing_single_release_file_tr(struct inode *inode, struct file *filp)
{
	struct trace_event_file *file = inode->i_private;

	trace_array_put(file->tr);
	event_file_put(file);

	return single_release(inode, filp);
}

-- Steve

>  };
>  #endif
>