Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Message-ID: <445ac00d-0f0c-4f6a-b85a-97209635c3f3@efficios.com>
Date:   Tue, 12 Dec 2023 09:33:11 -0500
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] tracing: Allow for max buffer data size trace_marker
 writes
Content-Language: en-US
To:     Steven Rostedt <rostedt@goodmis.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Linux Trace Kernel <linux-trace-kernel@vger.kernel.org>
Cc:     Masami Hiramatsu <mhiramat@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>
References: <20231212090057.41b28efe@gandalf.local.home>
From:   Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
In-Reply-To: <20231212090057.41b28efe@gandalf.local.home>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2023-12-12 09:00, Steven Rostedt wrote:
[...]
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -7272,6 +7272,7 @@ tracing_mark_write(struct file *filp, const char __user *ubuf,
>   	enum event_trigger_type tt = ETT_NONE;
>   	struct trace_buffer *buffer;
>   	struct print_entry *entry;
> +	int meta_size;
>   	ssize_t written;
>   	int size;
>   	int len;
> @@ -7286,12 +7287,9 @@ tracing_mark_write(struct file *filp, const char __user *ubuf,
>   	if (!(tr->trace_flags & TRACE_ITER_MARKERS))
>   		return -EINVAL;
>   
> -	if (cnt > TRACE_BUF_SIZE)
> -		cnt = TRACE_BUF_SIZE;

You're removing an early bound check for a size_t userspace input...

> -
> -	BUILD_BUG_ON(TRACE_BUF_SIZE >= PAGE_SIZE);
> -
> -	size = sizeof(*entry) + cnt + 2; /* add '\0' and possible '\n' */
> +	meta_size = sizeof(*entry) + 2;  /* add '\0' and possible '\n' */
> + again:
> +	size = cnt + meta_size;

... and then implicitly casting it into a "int" size variable, which
can therefore become a negative value.

Just for the sake of not having to rely on ring_buffer_lock_reserve
catching (length > BUF_MAX_DATA_SIZE), I would recommend to add an
early check for negative here.

>   
>   	/* If less than "<faulted>", then make sure we can still add that */
>   	if (cnt < FAULTED_SIZE)
> @@ -7300,9 +7298,25 @@ tracing_mark_write(struct file *filp, const char __user *ubuf,
>   	buffer = tr->array_buffer.buffer;
>   	event = __trace_buffer_lock_reserve(buffer, TRACE_PRINT, size,
>   					    tracing_gen_ctx());
> -	if (unlikely(!event))
> +	if (unlikely(!event)) {
> +		/*
> +		 * If the size was greated than what was allowed, then

greater ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com