Re: [PATCH 2/2] tracing: Add a verifier to check string pointers for trace events

[kernel test robot <lkp@intel.com>]

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

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
config: arc-randconfig-r022-20210226 (attached as .config)
compiler: arceb-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/43930a0d94b958cf1fdd82d4ae4cc97fc5d1143b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Steven-Rostedt/tracing-Detect-unsafe-dereferencing-of-pointers-from-trace-events/20210227-030901
        git checkout 43930a0d94b958cf1fdd82d4ae4cc97fc5d1143b
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc 

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

All warnings (new ones prefixed by >>):

   kernel/trace/trace.c: In function 'trace_check_vprintf':
>> kernel/trace/trace.c:3662:3: warning: function 'trace_check_vprintf' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
    3662 |   trace_seq_vprintf(&iter->seq, iter->fmt, ap);
         |   ^~~~~~~~~~~~~~~~~
   kernel/trace/trace.c:3699:3: warning: function 'trace_check_vprintf' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
    3699 |   trace_seq_vprintf(&iter->seq, p, ap);
         |   ^~~~~~~~~~~~~~~~~
   At top level:
   kernel/trace/trace.c:1683:37: warning: 'tracing_max_lat_fops' defined but not used [-Wunused-const-variable=]
    1683 | static const struct file_operations tracing_max_lat_fops;
         |                                     ^~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/perf_event.h:25,
                    from include/linux/hw_breakpoint.h:5,
                    from kernel/trace/trace.h:15,
                    from kernel/trace/trace.c:52:
   arch/arc/include/asm/perf_event.h:126:23: warning: 'arc_pmu_cache_map' defined but not used [-Wunused-const-variable=]
     126 | static const unsigned arc_pmu_cache_map[C(MAX)][C(OP_MAX)][C(RESULT_MAX)] = {
         |                       ^~~~~~~~~~~~~~~~~
   arch/arc/include/asm/perf_event.h:91:27: warning: 'arc_pmu_ev_hw_map' defined but not used [-Wunused-const-variable=]
      91 | static const char * const arc_pmu_ev_hw_map[] = {
         |                           ^~~~~~~~~~~~~~~~~


vim +3662 kernel/trace/trace.c

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

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

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 19685 bytes --]