diff mbox series

tracing: Fix filter string testing

Message ID 20250417183003.505835fb@gandalf.local.home (mailing list archive)
State Queued
Headers show
Series tracing: Fix filter string testing | expand

Commit Message

Steven Rostedt April 17, 2025, 10:30 p.m. UTC 
From: Steven Rostedt <rostedt@goodmis.org>

The filter string testing uses strncpy_from_kernel/user_nofault() to
retrieve the string to test the filter against. The if() statement was
incorrect as it considered 0 as a fault, when it is only negative that it
faulted.

Link: https://lore.kernel.org/all/CAEf4BzbVPQ=BjWztmEwBPRKHUwNfKBkS3kce-Rzka6zvbQeVpg@mail.gmail.com/

Cc: stable@vger.kernel.org
Fixes: 77360f9bbc7e5 ("tracing: Add test for user space strings when filtering on string pointers")
Reported-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Reported-by: Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace_events_filter.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Andrew Morton April 17, 2025, 11:29 p.m. UTC | #1 
On Thu, 17 Apr 2025 18:30:03 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <rostedt@goodmis.org>
> 
> The filter string testing uses strncpy_from_kernel/user_nofault() to
> retrieve the string to test the filter against. The if() statement was
> incorrect as it considered 0 as a fault, when it is only negative that it
> faulted.

changelog forgot to describe the userspace-visible effects of the bug?

> Cc: stable@vger.kernel.org

Which is more important when proposing this!
Steven Rostedt April 18, 2025, 12:34 a.m. UTC | #2 
On Thu, 17 Apr 2025 16:29:27 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Thu, 17 Apr 2025 18:30:03 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: Steven Rostedt <rostedt@goodmis.org>
> > 
> > The filter string testing uses strncpy_from_kernel/user_nofault() to
> > retrieve the string to test the filter against. The if() statement was
> > incorrect as it considered 0 as a fault, when it is only negative that it
> > faulted.  
> 
> changelog forgot to describe the userspace-visible effects of the bug?
> 
> > Cc: stable@vger.kernel.org  
> 
> Which is more important when proposing this!
> 

I can update the change log to show how it's broken. In fact, I'm
working on a selftest to catch it if it breaks again.

  # cd /sys/kernel/tracing
  # echo "filename.ustring ~ \"/proc*\"" > events/syscalls/sys_enter_openat/filter
  # ls /proc/$$/maps
  # cat trace

If it works you get:

              ls-1192    [007] .....  8169.828333: sys_openat(dfd: ffffffffffffff9c, filename: 7efc18359904, flags: 80000, mode: 0)

If not, you get nothing!

-- Steve
diff mbox series

Patch

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 0993dfc1c5c1..2048560264bb 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -808,7 +808,7 @@  static __always_inline char *test_string(char *str)
 	kstr = ubuf->buffer;
 
 	/* For safety, do not trust the string pointer */
-	if (!strncpy_from_kernel_nofault(kstr, str, USTRING_BUF_SIZE))
+	if (strncpy_from_kernel_nofault(kstr, str, USTRING_BUF_SIZE) < 0)
 		return NULL;
 	return kstr;
 }
@@ -827,7 +827,7 @@  static __always_inline char *test_ustring(char *str)
 
 	/* user space address? */
 	ustr = (char __user *)str;
-	if (!strncpy_from_user_nofault(kstr, ustr, USTRING_BUF_SIZE))
+	if (strncpy_from_user_nofault(kstr, ustr, USTRING_BUF_SIZE) < 0)
 		return NULL;
 
 	return kstr;