linux-kernel - Re: [RFC PATCH 1/2] tracing: fixes of ftrace_enable

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] [day] [month] [year] [list]

Message-Id: <20250613114527.7e27a9a0ecc2b59d98677b0c@kernel.org>
Date: Fri, 13 Jun 2025 11:45:27 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Gabriele Paoloni <gpaoloni@...hat.com>
Cc: rostedt@...dmis.org, mathieu.desnoyers@...icios.com,
 linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
 acarmina@...hat.com, chuck.wolber@...ing.com
Subject: Re: [RFC PATCH 1/2] tracing: fixes of ftrace_enable_fops

On Thu, 12 Jun 2025 12:43:48 +0200
Gabriele Paoloni <gpaoloni@...hat.com> wrote:

> Currently there are different issues associated with ftrace_enable_fops
> - event_enable_write: *ppos is increased while not used at all in the
>   write operation itself (following a write, this could lead a read to
>   fail or report a corrupted event status);

Here, we expected the "enable" file is a pseudo text file. So if
there is a write, the ppos should be incremented.

> - event_enable_read: cnt < strlen(buf) is allowed and this can lead to
>   reading an incomplete event status (i.e. not all status characters
>   are retrieved) and/or reading the status in a non-atomic way (i.e.
>   the status could change between two consecutive reads);

As I said, the "enable" file is a kind of text file. So reader must read
it until EOF. If you need to get the consistent result, user should
use the enough size of buffer.

> - .llseek is set to default_llseek: this is wrong since for this
>   type of files it does not make sense to reposition the ppos offset.
>   Hence this should be set instead to noop_llseek.

As I said, it is a kind of text file, default_llseek is better.

But, if we change (re-design) what is this "enable" file is,
we can accept these changes. So this is not a "Fix" but re-design
of the "enable" file as an interface (as a char device), not a text
file (or a block device).

I want to keep this as is, same as other tracefs files.

Thank you,

> 
> This patch fixes all the issues listed above.
> 
> Signed-off-by: Gabriele Paoloni <gpaoloni@...hat.com>
> Tested-by: Alessandro Carminati <acarmina@...hat.com>
> ---
>  kernel/trace/trace_events.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 120531268abf..5e84ef01d0c8 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -1798,6 +1798,13 @@ event_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
>  
>  	strcat(buf, "\n");
>  
> +	/*
> +	 * A requested cnt less than strlen(buf) could lead to a wrong
> +	 * event status being reported.
> +	 */
> +	if (cnt < strlen(buf))
> +		return -EINVAL;
> +
>  	return simple_read_from_buffer(ubuf, cnt, ppos, buf, strlen(buf));
>  }
>  
> @@ -1833,8 +1840,6 @@ event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
>  		return -EINVAL;
>  	}
>  
> -	*ppos += cnt;
> -
>  	return cnt;
>  }
>  
> @@ -2557,7 +2562,7 @@ static const struct file_operations ftrace_enable_fops = {
>  	.read = event_enable_read,
>  	.write = event_enable_write,
>  	.release = tracing_release_file_tr,
> -	.llseek = default_llseek,
> +	.llseek = noop_llseek,
>  };
>  
>  static const struct file_operations ftrace_event_format_fops = {
> -- 
> 2.48.1
> 


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