Re: [PATCH v2 01/25] NFSD: Fix TP_printk() format specifier in trace_nfsd_dirent()

[Chuck Lever III <chuck.lever@oracle.com>]

> On May 13, 2021, at 3:17 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Thu, 13 May 2021 19:08:13 +0000
> Chuck Lever III <chuck.lever@oracle.com> wrote:
> 
>> The tracepoints that currently use '%.*s' no longer work when
>> using "trace-cmd start/stop/show". They were working before
>> 9a6944fee68e, so I consider this a regression. I plan to
>> submit patches to address this for 5.13-rc. I guess they will
>> have to go without the use of the new _len macros for now,
>> and you can push the macros in v5.14.
> 
> That's a separate bug. I'm currently running this patch through my tests,
> and will push to Linus when it completes. Feel free to test this one too.

Confirmed that applying the below patch addresses the regression.

Tested-by: Chuck Lever <chuck.lever@oracle.com>

Thanks for your quick response!


> -- Steve
> 
> From eb01f5353bdaa59600b29d864819056a0e3de24d Mon Sep 17 00:00:00 2001
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> Date: Thu, 13 May 2021 12:23:24 -0400
> Subject: [PATCH] tracing: Handle %.*s in trace_check_vprintf()
> 
> If a trace event uses the %*.s notation, the trace_check_vprintf() will
> fail and will warn about a bad processing of strings, because it does not
> take into account the length field when processing the star (*) part.
> Have it handle this case as well.
> 
> Link: https://lore.kernel.org/linux-nfs/238C0E2D-C2A4-4578-ADD2-C565B3B99842@oracle.com/
> 
> Reported-by: Chuck Lever III <chuck.lever@oracle.com>
> Fixes: 9a6944fee68e2 ("tracing: Add a verifier to check string pointers for trace events")
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> kernel/trace/trace.c | 31 +++++++++++++++++++++++++++----
> 1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 560e4c8d3825..a21ef9cd2aae 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3704,6 +3704,9 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt,
> 		goto print;
> 
> 	while (*p) {
> +		bool star = false;
> +		int len = 0;
> +
> 		j = 0;
> 
> 		/* We only care about %s and variants */
> @@ -3725,13 +3728,17 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt,
> 				/* Need to test cases like %08.*s */
> 				for (j = 1; p[i+j]; j++) {
> 					if (isdigit(p[i+j]) ||
> -					    p[i+j] == '*' ||
> 					    p[i+j] == '.')
> 						continue;
> +					if (p[i+j] == '*') {
> +						star = true;
> +						continue;
> +					}
> 					break;
> 				}
> 				if (p[i+j] == 's')
> 					break;
> +				star = false;
> 			}
> 			j = 0;
> 		}
> @@ -3744,6 +3751,9 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt,
> 		iter->fmt[i] = '\0';
> 		trace_seq_vprintf(&iter->seq, iter->fmt, ap);
> 
> +		if (star)
> +			len = va_arg(ap, int);
> +
> 		/* The ap now points to the string data of the %s */
> 		str = va_arg(ap, const char *);
> 
> @@ -3762,8 +3772,18 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt,
> 			int ret;
> 
> 			/* Try to safely read the string */
> -			ret = strncpy_from_kernel_nofault(iter->fmt, str,
> -							  iter->fmt_size);
> +			if (star) {
> +				if (len + 1 > iter->fmt_size)
> +					len = iter->fmt_size - 1;
> +				if (len < 0)
> +					len = 0;
> +				ret = copy_from_kernel_nofault(iter->fmt, str, len);
> +				iter->fmt[len] = 0;
> +				star = false;
> +			} else {
> +				ret = strncpy_from_kernel_nofault(iter->fmt, str,
> +								  iter->fmt_size);
> +			}
> 			if (ret < 0)
> 				trace_seq_printf(&iter->seq, "(0x%px)", str);
> 			else
> @@ -3775,7 +3795,10 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt,
> 			strncpy(iter->fmt, p + i, j + 1);
> 			iter->fmt[j+1] = '\0';
> 		}
> -		trace_seq_printf(&iter->seq, iter->fmt, str);
> +		if (star)
> +			trace_seq_printf(&iter->seq, iter->fmt, len, str);
> +		else
> +			trace_seq_printf(&iter->seq, iter->fmt, str);
> 
> 		p += i + j + 1;
> 	}
> -- 
> 2.29.2
> 

--
Chuck Lever