All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] tracing: Detect unsafe dereferencing of pointers from trace events
@ 2021-02-26 18:59 Steven Rostedt
  2021-02-26 18:59 ` [PATCH 1/2] tracing: Add check of trace event print fmts for dereferencing pointers Steven Rostedt
                   ` (2 more replies)
  0 siblings, 3 replies; 25+ messages in thread
From: Steven Rostedt @ 2021-02-26 18:59 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ingo Molnar, Andrew Morton, Masami Hiramatsu, Linus Torvalds,
	Jacob Wen


After seeing that an unsafe string dereference in a trace event made
it into the kernel, I decided it's time to add some sanity checks to
catch these cases without needing me to supervise.

The first patch scans the print fmts of the trace events looking for
dereferencing pointers from %p*, and making sure that they refer back
to the trace event itself.

The second patch handles strings "%s", as there are cases that are
fine with dereferencing the string outside the trace event. On reading
of the trace file, the %s is looked for and when found, the logic checks
the pointer that it is about to be dereferenced to see if it is a
valid location. This check would have caught the last unsafe dereference
committed into the kernel.


Steven Rostedt (VMware) (2):
      tracing: Add check of trace event print fmts for dereferencing pointers
      tracing: Add a verifier to check string pointers for trace events

----
 kernel/trace/trace.c        | 148 +++++++++++++++++++++++++++++++++
 kernel/trace/trace.h        |   2 +
 kernel/trace/trace_events.c | 198 ++++++++++++++++++++++++++++++++++++++++++++
 kernel/trace/trace_output.c |   2 +-
 4 files changed, 349 insertions(+), 1 deletion(-)

^ permalink raw reply	[flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] tracing: Add a verifier to check string pointers for trace events
@ 2021-02-26 22:23 kernel test robot
  0 siblings, 0 replies; 25+ messages in thread
From: kernel test robot @ 2021-02-26 22:23 UTC (permalink / raw)
  To: kbuild

[-- Attachment #1: Type: text/plain, Size: 11121 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210226190705.871102407@goodmis.org>
References: <20210226190705.871102407@goodmis.org>
TO: Steven Rostedt <rostedt@goodmis.org>

Hi Steven,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on next-20210226]
[cannot apply to tip/perf/core linux/master hnaz-linux-mm/master v5.11]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Steven-Rostedt/tracing-Detect-unsafe-dereferencing-of-pointers-from-trace-events/20210227-030901
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 8b83369ddcb3fb9cab5c1088987ce477565bb630
:::::: branch date: 3 hours ago
:::::: commit date: 3 hours ago
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


cppcheck possible warnings: (new ones prefixed by >>, may not real problems)

   kernel/trace/trace.c:4910:8: warning: snprintf format string requires 2 parameters but only 1 is given. [wrongPrintfScanfArgNum]
    len = snprintf(NULL, 0, "%*pbn",
          ^
   kernel/trace/trace.c:4916:8: warning: snprintf format string requires 2 parameters but only 1 is given. [wrongPrintfScanfArgNum]
    len = snprintf(mask_str, len, "%*pbn",
          ^
>> kernel/trace/trace.c:3623:10: warning: Either the condition '!fmt' is redundant or there is possible null pointer dereference: p. [nullPointerRedundantCheck]
    while (*p) {
            ^
   kernel/trace/trace.c:3616:19: note: Assuming that condition '!fmt' is not redundant
    if (WARN_ON_ONCE(!fmt))
                     ^
   kernel/trace/trace.c:3612:18: note: Assignment 'p=fmt', assigned value is 0
    const char *p = fmt;
                    ^
   kernel/trace/trace.c:3623:10: note: Null pointer dereference
    while (*p) {
            ^

vim +3623 kernel/trace/trace.c

43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3596) 
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3597) /**
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3598)  * trace_check_vprintf - Check dereferenced strings while writing to the seq buffer
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3599)  * @iter: The iterator that holds the seq buffer and the event being printed
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3600)  * @fmt: The format used to print the event
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3601)  * @ap: The va_list holding the data to print from @fmt.
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3602)  *
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3603)  * This writes the data into the @iter->seq buffer using the data from
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3604)  * @fmt and @ap. If the format has a %s, then the source of the string
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3605)  * is examined to make sure it is safe to print, otherwise it will
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3606)  * warn and print "[UNSAFE MEMORY]" in place of the dereferenced string
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3607)  * pointer.
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3608)  */
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3609) void trace_check_vprintf(struct trace_iterator *iter, const char *fmt,
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3610) 			 va_list ap)
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3611) {
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3612) 	const char *p = fmt;
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3613) 	const char *str;
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3614) 	int i, j;
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3615) 
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3616) 	if (WARN_ON_ONCE(!fmt))
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3617) 		return;
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3618) 
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3619) 	/* Don't bother checking when doing a ftrace_dump() */
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3620) 	if (iter->fmt == static_fmt_buf)
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3621) 		goto print;
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3622) 
43930a0d94b958 Steven Rostedt (VMware  2021-02-26 @3623) 	while (*p) {
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3624) 		j = 0;
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3625) 
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3626) 		/* We only care about %s and variants */
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3627) 		for (i = 0; p[i]; i++) {
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3628) 			if (i + 1 >= iter->fmt_size) {
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3629) 				/*
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3630) 				 * If we can't expand the copy buffer,
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3631) 				 * just print it.
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3632) 				 */
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3633) 				if (!trace_iter_expand_format(iter))
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3634) 					goto print;
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3635) 			}
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3636) 
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3637) 			if (p[i] == '\\' && p[i+1]) {
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3638) 				i++;
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3639) 				continue;
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3640) 			}
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3641) 			if (p[i] == '%') {
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3642) 				/* Need to test cases like %08.*s */
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3643) 				for (j = 1; p[i+j]; j++) {
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3644) 					if (isdigit(p[i+j]) ||
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3645) 					    p[i+j] == '*' ||
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3646) 					    p[i+j] == '.')
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3647) 						continue;
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3648) 					break;
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3649) 				}
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3650) 				if (p[i+j] == 's')
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3651) 					break;
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3652) 			}
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3653) 			j = 0;
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3654) 		}
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3655) 		/* If no %s found then just print normally */
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3656) 		if (!p[i])
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3657) 			break;
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3658) 
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3659) 		/* Copy up to the %s, and print that */
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3660) 		strncpy(iter->fmt, p, i);
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3661) 		iter->fmt[i] = '\0';
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3662) 		trace_seq_vprintf(&iter->seq, iter->fmt, ap);
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3663) 
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3664) 		/* The ap now points to the string data of the %s */
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3665) 		str = va_arg(ap, const char *);
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3666) 
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3667) 		/*
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3668) 		 * If you hit this warning, it is likely that the
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3669) 		 * trace event in question used %s on a string that
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3670) 		 * was saved at the time of the event, but may not be
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3671) 		 * around when the trace is read. Use __string(),
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3672) 		 * __assign_str() and __get_str() helpers in the TRACE_EVENT()
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3673) 		 * instead. See samples/trace_events/trace-events-sample.h
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3674) 		 * for reference.
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3675) 		 */
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3676) 		if (WARN_ON_ONCE(!trace_safe_str(iter, str))) {
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3677) 			int ret;
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3678) 
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3679) 			/* Try to safely read the string */
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3680) 			ret = strncpy_from_kernel_nofault(iter->fmt, str,
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3681) 							  iter->fmt_size);
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3682) 			if (ret < 0)
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3683) 				trace_seq_printf(&iter->seq, "(0x%px)", str);
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3684) 			else
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3685) 				trace_seq_printf(&iter->seq, "(0x%px:%s)",
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3686) 						 str, iter->fmt);
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3687) 			str = "[UNSAFE-MEMORY]";
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3688) 			strcpy(iter->fmt, "%s");
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3689) 		} else {
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3690) 			strncpy(iter->fmt, p + i, j + 1);
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3691) 			iter->fmt[j+1] = '\0';
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3692) 		}
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3693) 		trace_seq_printf(&iter->seq, iter->fmt, str);
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3694) 
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3695) 		p += i + j + 1;
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3696) 	}
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3697)  print:
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3698) 	if (*p)
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3699) 		trace_seq_vprintf(&iter->seq, p, ap);
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3700) }
43930a0d94b958 Steven Rostedt (VMware  2021-02-26  3701) 

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2021-06-07 17:24 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-26 18:59 [PATCH 0/2] tracing: Detect unsafe dereferencing of pointers from trace events Steven Rostedt
2021-02-26 18:59 ` [PATCH 1/2] tracing: Add check of trace event print fmts for dereferencing pointers Steven Rostedt
2021-02-27 14:16   ` [tracing] 5c71984c21: WARNING:at_kernel/trace/trace_events.c:#test_event_printk kernel test robot
2021-02-27 14:16     ` kernel test robot
2021-02-26 18:59 ` [PATCH 2/2] tracing: Add a verifier to check string pointers for trace events Steven Rostedt
2021-02-26 21:22   ` kernel test robot
2021-02-26 23:35     ` Steven Rostedt
2021-06-05  1:20   ` Sean Christopherson
2021-06-05  2:28     ` Steven Rostedt
2021-06-05  2:45       ` Steven Rostedt
2021-06-07 16:02         ` Sean Christopherson
2021-06-07 17:24           ` Steven Rostedt
2021-02-26 22:21 ` [PATCH 0/2] tracing: Detect unsafe dereferencing of pointers from " Linus Torvalds
2021-02-26 23:33   ` Steven Rostedt
2021-02-27  4:04     ` Joe Perches
2021-02-27 19:18   ` Steven Rostedt
2021-02-28  0:08     ` Steven Rostedt
2021-03-01  5:27       ` Pawel Laszczak
2021-03-02  8:23         ` Peter Chen
2021-03-02 14:56           ` Steven Rostedt
2021-03-03  1:21             ` Peter Chen
2021-03-03  1:54               ` Steven Rostedt
2021-03-07  4:01             ` Peter Chen
2021-03-07 21:14               ` Steven Rostedt
  -- strict thread matches above, loose matches on Subject: below --
2021-02-26 22:23 [PATCH 2/2] tracing: Add a verifier to check string pointers for " kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.