Received-SPF: pass (google.com: domain of linux-kernel+bounces-91317-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 4 Mar 2024 16:42:05 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: LKML <linux-kernel@vger.kernel.org>, Masami Hiramatsu
 <mhiramat@kernel.org>, Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
 Sachin Sant <sachinp@linux.ibm.com>
Subject: Re: [GIT PULL] tracing: Prevent trace_marker being bigger than
 unsigned short
Message-ID: <20240304164205.3245608a@gandalf.local.home>
In-Reply-To: <20240303160024.458d4f91@rorschach.local.home>
References: <20240302111244.3a1674be@gandalf.local.home>
	<CAHk-=wj376WMgZ24wKGEWDs_ojNtod-LDZBedPzDYRRcY60UYA@mail.gmail.com>
	<20240302145958.05aabdd2@rorschach.local.home>
	<CAHk-=wgjhdRj1V847NTF4veMN_tCbrySiEHXO8RO3n05cNeXeA@mail.gmail.com>
	<20240302154713.71e29402@rorschach.local.home>
	<CAHk-=wioeo5vyEWUZcGBKMsf3jnjrnnHc3uJiV=JjSKPdvZOEw@mail.gmail.com>
	<20240303075937.36fc6043@rorschach.local.home>
	<CAHk-=wiLdWetJgKHB72VeDALZsjpggEyyuiZ2KmoY_g+3horwQ@mail.gmail.com>
	<20240303140705.0f655e36@rorschach.local.home>
	<CAHk-=wiTGmAXfHiRB8ku4diLxRpN=Hac_q86=j65oiP3J5uXKg@mail.gmail.com>
	<20240303160024.458d4f91@rorschach.local.home>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Sun, 3 Mar 2024 16:00:24 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> > So I now tell you that you should
> > 
> >  (a) get rid of the stupid and nonsensical precision  
> 
> I can do that.

As I mentioned that the design is based on that the allocated buffer size is
the string length rounded up to the word size, all I need to do is to make
sure that there's a nul terminating byte within the last word of the
allocated buffer. Then "%s" is all I need.

Although, when writing this I found that it isn't rounded from the size of
the string itself, but because I allocate a bit more than what is written
to trace_marker, in case I need to append a '\n' and '\0' just word size
checking isn't enough. Doing two words is more than enough to find the
terminating nul unless there's a bug, in which case this would trigger a
warning.

Would this work for you?

I tested this on both 32 bit and 64 bit machines, with the following command:

 # cd /sys/kernel/tracing
 # for s in 80 480 1000 1450 2000 3000 4050 5500; do
	let c=$s+64;
	for i in `seq $s $c` ; do
		str=`printf -- 'X%.0s' $(seq $i)`;
		echo "write $i";
		echo "$str" > trace_marker;
	done;
   done
 # cat trace

-- Steve

diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index 3e7fa44dc2b2..848a78bab20e 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -1581,6 +1581,25 @@ static struct trace_event trace_bprint_event = {
 	.funcs		= &trace_bprint_funcs,
 };
 
+/*
+ * Strings in the print entry are stored by their length rounded
+ * up to the nearest word size. The write to the buffer also allocates
+ * a couple of extra bytes in case it needs to append a '\n' and '\0'
+ * if the passed in string doesn't contain them. Check up to two words
+ * in length back to make sure there's a terminating nul.
+ */
+static int test_trace_string(int len, const char *str)
+{
+	int test = sizeof(long) * 2;
+
+	for (int i = 0; len > i && i < test; i++) {
+		if (!str[len - (i + 1)])
+			return 0;
+	}
+	WARN_ONCE(1, "Trace print string missing nul terminator %d", len);
+	return -1;
+}
+
 /* TRACE_PRINT */
 static enum print_line_t trace_print_print(struct trace_iterator *iter,
 					   int flags, struct trace_event *event)
@@ -1591,8 +1610,11 @@ static enum print_line_t trace_print_print(struct trace_iterator *iter,
 
 	trace_assign_type(field, iter->ent);
 
+	if (test_trace_string(max, field->buf))
+		return TRACE_TYPE_UNHANDLED;
+
 	seq_print_ip_sym(s, field->ip, flags);
-	trace_seq_printf(s, ": %.*s", max, field->buf);
+	trace_seq_printf(s, ": %s", field->buf);
 
 	return trace_handle_return(s);
 }
@@ -1605,7 +1627,10 @@ static enum print_line_t trace_print_raw(struct trace_iterator *iter, int flags,
 
 	trace_assign_type(field, iter->ent);
 
-	trace_seq_printf(&iter->seq, "# %lx %.*s", field->ip, max, field->buf);
+	if (test_trace_string(max, field->buf))
+		return TRACE_TYPE_UNHANDLED;
+
+	trace_seq_printf(&iter->seq, "# %lx %s", field->ip, field->buf);
 
 	return trace_handle_return(&iter->seq);
 }