[RFC][PATCH 0/3] vsprintf: Do not dereference pointers in bstr_printf()

[RFC][PATCH 0/3] vsprintf: Do not dereference pointers in bstr_printf()

[Steven Rostedt]

vbin_printf() and bstr_printf() are used by trace_printk(), when it is
possible to optimize to copying the binary arguments into the ring buffer
and doing the final conversions at the time of reading the ring buffer.
This is advantageous as it removes computer processing from the executing
of what's being traced, to the reading of the trace at a less critical
time.

The issue is that the way the bprintk() works, as it records pointers
at the time of execution, and then later dereferences those pointers
when the buffer is being read. Things can change between these two events
and bad pointers may be dereferenced.

In stead of just trying to avoid using vbin_printf() when using these
pointers, as there is no use case for using it when dereferencing is
can be an issue, have vbin_printf() and bstr_printf() either save
the dereferenced information in vbin_printf() and print that out without
the need to dereference at bstr_printf(), or simply output an error
message saying that the dereference typo is unsupported.

This will prevent surprises while debugging with trace_printk().

The first patch adds the infrastructure to not support any of the
dereferenced pointers. Then I added two patches to add support of
%pf and %pb. The rest of the dereferenced pointers can be added at
a later time, but I wanted to get people's feel for this change before
going further on it.

Thoughts?

-- Steve



Steven Rostedt (Red Hat) (3):
      vsprintf: Prevent vbin_printf() from using dereferenced pointers
      vsprintf: Add support for %pf and %pF to vbin_printf()
      vsprintf: Add support for %pb and friends to vbin_printf()

----
 lib/vsprintf.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 118 insertions(+), 4 deletions(-)

[RFC][PATCH 1/3] vsprintf: Prevent vbin_printf() from using dereferenced pointers

[Steven Rostedt]

[-- Attachment #1: 0001-vsprintf-Prevent-vbin_printf-from-using-dereferenced.patch --]
[-- Type: text/plain, Size: 4570 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

vbin_printf() is used in conjunction with bstr_printf() to record some
printf() statment and to show it later. It is currently used exclusively
with trace_printk() which records the binary format into the trace ring
buffer and formats the output at a later time when the ring buffer is read.
This helps speed up the calculations of trace_printk() when it is called to
lower the impact the tracing has on the running system. The more execution
that can be delayed to the time of reading the trace, the less intrusive the
trace is on the running executiong being traced.

The problem arises with dereferenced pointers. The printk() format allows
several types of format fields outside of normal printf() that helps in
outputing various kernel structures. This includes dereferenced function
pointers (from powerpc), cpumasks, MAC addresses, ip addresses to name a
few. Currently the pointer to these structures are saved in the buffer by
vbin_printf() and then dereferenced with bstr_printf().

When vbin_printf() is called, it is in the context of where the pointer is
in use, with necessary locks and such. It can also be assumed that because
the pointer is being passed to the vbin_printf() from the user, that that
pointer currently exists. As bstr_printf() can be displayed at a later time,
this is not the case. Locks wont be held and the pointer may not even exist
anymore. This causes bstr_printf() to dereference a bad pointer and can
cause a kernel oops.

When should be done is that vbin_printf() should save the contents of what
the pointer points to and bstr_printf() should only output what it has in
the buffer, without the need to dereference. But in the mean time, a check
is now added to see if a pointer type is supported by vbin_printf() and if
it is not, then "Unsupported type:%pX" is output instead. This prevents the
bstr_printf() from doing a kernel oops. Luckily, trace_printk() is the only
user and that is only added for debugging the kernel.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 lib/vsprintf.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 77 insertions(+), 3 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 0967771d8f7f..b2a331d948a2 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2257,6 +2257,62 @@ EXPORT_SYMBOL(sprintf);
  * bstr_printf() - Binary data to text string
  */
 
+/*
+ * As vbin_printf() is not the same as a normal printf(), as it is broken
+ * into two parts:
+ *   The recording of the pointer information.
+ *   The printing of the information.
+ * As the pointer may change, or even worse no longer exist, then dereferenced
+ * pointers must not be used with this utility unless it is supported.
+ */
+
+static bool supported_bin_ptr(const char *fmt)
+{
+	bool supported = false;
+
+	switch (fmt[0]) {
+	case 'F':
+	case 'f':
+		/* Some archs dereference function pointers */
+		if (vbin_printf == 
+		    dereference_function_descriptor(vbin_printf)) {
+			supported = true;
+			break;
+		}
+		/* Fall through */
+	case 'R':
+	case 'r':
+	case 'b':
+	case 'M':
+	case 'm':
+	case 'I':
+	case 'i':
+	case 'E':
+	case 'U':
+	case 'V':
+		break;
+	case 'N':
+		if (fmt[1] != 'F') {
+			supported = true;
+			break;
+		}
+		/* fall through */
+	case 'a':
+	case 'd':
+	case 'C':
+	case 'D':
+	case 'g':
+	case 'G':
+		break;
+	default:
+		supported = true;
+	}
+
+	return supported;
+}
+
+const static char unsupported_str[] = "Unsupported type:%p";
+
 /**
  * vbin_printf - Parse a format string and place args' binary value in a buffer
  * @bin_buf: The buffer to place args' binary value
@@ -2490,12 +2546,30 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 			break;
 		}
 
-		case FORMAT_TYPE_PTR:
-			str = pointer(fmt, str, end, get_arg(void *), spec);
+		case FORMAT_TYPE_PTR: {
+			const char *_fmt = fmt;
+
+			if (!supported_bin_ptr(fmt)) {
+				int len = sizeof(unsupported_str) + 1;
+
+				if (str + len <= end) {
+					strcpy(str, unsupported_str);
+					str += sizeof(unsupported_str) - 1;
+					str[0] = fmt[0];
+					str[1] = ':';
+					str += 2;
+				} else
+					str += len;
+				/* Just show the pointer itself */
+				_fmt = "";
+				spec.field_width = -1;
+			}
+			str = pointer(_fmt, str, end, get_arg(void *), spec);
+
 			while (isalnum(*fmt))
 				fmt++;
 			break;
-
+		}
 		case FORMAT_TYPE_PERCENT_CHAR:
 			if (str < end)
 				*str = '%';
-- 
2.8.1

[RFC][PATCH 2/3] vsprintf: Add support for %pf and %pF to vbin_printf()

[Steven Rostedt]

[-- Attachment #1: 0002-vsprintf-Add-support-for-pf-and-pF-to-vbin_printf.patch --]
[-- Type: text/plain, Size: 2127 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Some architectures require a dereference of function pointers (See powerpc).
The dereferenced poiner must be saved into the buffer in vbin_printf() to be
safe to access it later.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 lib/vsprintf.c | 44 ++++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b2a331d948a2..1eb4c7bc3509 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2271,15 +2271,6 @@ static bool supported_bin_ptr(const char *fmt)
 	bool supported = false;
 
 	switch (fmt[0]) {
-	case 'F':
-	case 'f':
-		/* Some archs dereference function pointers */
-		if (vbin_printf == 
-		    dereference_function_descriptor(vbin_printf)) {
-			supported = true;
-			break;
-		}
-		/* Fall through */
 	case 'R':
 	case 'r':
 	case 'b':
@@ -2394,7 +2385,21 @@ do {									\
 		}
 
 		case FORMAT_TYPE_PTR:
-			save_arg(void *);
+			switch (fmt[0]) {
+			case 'F':
+			case 'f': {
+				void *ptr = va_arg(args, void *);
+
+				ptr = dereference_function_descriptor(ptr);
+				str = PTR_ALIGN(str, sizeof(u32));
+				if (str + sizeof(ptr) <= end)
+					*(void **)str = ptr;
+				str += sizeof(ptr);
+				break;
+			}
+			default:
+				save_arg(void *);
+			}
 			/* skip all alphanumeric pointer suffixes */
 			while (isalnum(*fmt))
 				fmt++;
@@ -2548,8 +2553,23 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 
 		case FORMAT_TYPE_PTR: {
 			const char *_fmt = fmt;
-
-			if (!supported_bin_ptr(fmt)) {
+			char tmp_fmt[2];
+
+			if (supported_bin_ptr(fmt)) {
+				switch (fmt[0]) {
+				case 'F':
+				case 'f':
+					/*
+					 * The saved pointer has already
+					 * been derefenced. Convert the
+					 * 'f' to 's' or 'F' to 'S'.
+					 */
+					tmp_fmt[0] = 's' - ('f' - fmt[0]);
+					tmp_fmt[1] = 0;
+					_fmt = tmp_fmt;
+					break;
+				}
+			} else {
 				int len = sizeof(unsupported_str) + 1;
 
 				if (str + len <= end) {
-- 
2.8.1

[RFC][PATCH 3/3] vsprintf: Add support for %pb and friends to vbin_printf()

[Steven Rostedt]

[-- Attachment #1: 0003-vsprintf-Add-support-for-pb-and-friends-to-vbin_prin.patch --]
[-- Type: text/plain, Size: 2432 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

printk() allows a %pb to print out cpumasks. As it uses a pointer to
reference the mask it can be problematic for vbin_printf() to use, as the
referencing is done at a later time (via bstr_printf()). Instead of saving
the pointer to the cpumask, simply save the entire cpumask to the structure
and use that data for bstr_printf() to print it out.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 lib/vsprintf.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 1eb4c7bc3509..474d9ddaca6f 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2273,7 +2273,6 @@ static bool supported_bin_ptr(const char *fmt)
 	switch (fmt[0]) {
 	case 'R':
 	case 'r':
-	case 'b':
 	case 'M':
 	case 'm':
 	case 'I':
@@ -2397,6 +2396,16 @@ do {									\
 				str += sizeof(ptr);
 				break;
 			}
+			case 'b': {
+				unsigned long *mask = va_arg(args, void *);
+				int len = BITS_TO_LONGS(spec.field_width);
+
+				str = PTR_ALIGN(str, sizeof(u32));
+				if (str + len <= end)
+					memcpy(str, mask, len);
+				str += len;
+				break;
+			}
 			default:
 				save_arg(void *);
 			}
@@ -2552,11 +2561,18 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 		}
 
 		case FORMAT_TYPE_PTR: {
+			unsigned long *tmp_ptr;
+			unsigned long *ptr;
 			const char *_fmt = fmt;
 			char tmp_fmt[2];
 
 			if (supported_bin_ptr(fmt)) {
 				switch (fmt[0]) {
+				case 'b': {
+					tmp_ptr = get_arg(void *);
+					ptr = (void *)&tmp_ptr;
+					break;
+				}
 				case 'F':
 				case 'f':
 					/*
@@ -2567,7 +2583,9 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 					tmp_fmt[0] = 's' - ('f' - fmt[0]);
 					tmp_fmt[1] = 0;
 					_fmt = tmp_fmt;
-					break;
+					/* Fall through */
+				default:
+					ptr = get_arg(void *);
 				}
 			} else {
 				int len = sizeof(unsupported_str) + 1;
@@ -2583,8 +2601,10 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 				/* Just show the pointer itself */
 				_fmt = "";
 				spec.field_width = -1;
+				ptr = get_arg(void *);
 			}
-			str = pointer(_fmt, str, end, get_arg(void *), spec);
+
+			str = pointer(_fmt, str, end, ptr, spec);
 
 			while (isalnum(*fmt))
 				fmt++;
-- 
2.8.1

Re: [RFC][PATCH 3/3] vsprintf: Add support for %pb and friends to vbin_printf()

[Jiri Olsa]

On Wed, Jun 29, 2016 at 04:05:25PM -0400, Steven Rostedt wrote:

SNIP

>  			}
> @@ -2552,11 +2561,18 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
>  		}
>  
>  		case FORMAT_TYPE_PTR: {
> +			unsigned long *tmp_ptr;
> +			unsigned long *ptr;
>  			const char *_fmt = fmt;
>  			char tmp_fmt[2];
>  
>  			if (supported_bin_ptr(fmt)) {
>  				switch (fmt[0]) {
> +				case 'b': {
> +					tmp_ptr = get_arg(void *);
> +					ptr = (void *)&tmp_ptr;

this seems wrong.. &tmp_ptr is address from the stack
not from args which is what we want in here IIUC

should we do something like in attached patch? untested..

thanks,
jirka


---
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 474d9ddaca6f..9aeb277ae533 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2561,7 +2561,6 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 		}
 
 		case FORMAT_TYPE_PTR: {
-			unsigned long *tmp_ptr;
 			unsigned long *ptr;
 			const char *_fmt = fmt;
 			char tmp_fmt[2];
@@ -2569,8 +2568,8 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 			if (supported_bin_ptr(fmt)) {
 				switch (fmt[0]) {
 				case 'b': {
-					tmp_ptr = get_arg(void *);
-					ptr = (void *)&tmp_ptr;
+					ptr = (unsigned long *) PTR_ALIGN(args, sizeof(u32));
+					args += BITS_TO_LONGS(spec.field_width);
 					break;
 				}
 				case 'F':

Re: [RFC][PATCH 3/3] vsprintf: Add support for %pb and friends to vbin_printf()

[Steven Rostedt]

On Thu, 30 Jun 2016 16:56:34 +0200
Jiri Olsa <jolsa@redhat.com> wrote:

> On Wed, Jun 29, 2016 at 04:05:25PM -0400, Steven Rostedt wrote:
> 
> SNIP
> 
> >  			}
> > @@ -2552,11 +2561,18 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
> >  		}
> >  
> >  		case FORMAT_TYPE_PTR: {
> > +			unsigned long *tmp_ptr;
> > +			unsigned long *ptr;
> >  			const char *_fmt = fmt;
> >  			char tmp_fmt[2];
> >  
> >  			if (supported_bin_ptr(fmt)) {
> >  				switch (fmt[0]) {
> > +				case 'b': {
> > +					tmp_ptr = get_arg(void *);
> > +					ptr = (void *)&tmp_ptr;  
> 
> this seems wrong.. &tmp_ptr is address from the stack
> not from args which is what we want in here IIUC

Ah, your right. This only worked because the test I did was on a mask
of less than 64 cpus, where the above moved the bitmask into tmp_ptr.

I do want the address of the actual args array.

> 
> should we do something like in attached patch? untested..

I'll test it out.

Thanks,

-- Steve

> 
> thanks,
> jirka
> 
> 
> ---
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 474d9ddaca6f..9aeb277ae533 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2561,7 +2561,6 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
>  		}
>  
>  		case FORMAT_TYPE_PTR: {
> -			unsigned long *tmp_ptr;
>  			unsigned long *ptr;
>  			const char *_fmt = fmt;
>  			char tmp_fmt[2];
> @@ -2569,8 +2568,8 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
>  			if (supported_bin_ptr(fmt)) {
>  				switch (fmt[0]) {
>  				case 'b': {
> -					tmp_ptr = get_arg(void *);
> -					ptr = (void *)&tmp_ptr;
> +					ptr = (unsigned long *) PTR_ALIGN(args, sizeof(u32));
> +					args += BITS_TO_LONGS(spec.field_width);
>  					break;
>  				}
>  				case 'F':