vsprintf: Add support for userspace strings

vsprintf: Add support for userspace strings

[Richard Weinberger]

While debugging issues I often add (trace_)printks to strategic positions.
Dealing with user provided string is complicated as an extra buffer a
copy_from_user() is needed.
This adds a new format string to allow direct printing of such strings.

My initial plan was to use %pU but 'U' is already taken, therefore
I used the next letter which comes in mind when one thinks of userpace,
'L'.
The %pL format string works exactly like %s.

[PATCH 1/2] Fix printk() on ERR_PTR()
[PATCH 2/2] vsprintf: Add support for userspace strings

[PATCH 1/2] Fix printk() on ERR_PTR()

[Richard Weinberger]

vbin_printf() checks whether the provided pointer is larger
than -PAGE_SIZE such that it does not explode on ERR_PTR() pointers.
printk() does not.

Let's add this check also to the printk() code such that
trace_printk() and printk() are consistent again.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 lib/vsprintf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index da39c60..092d5a7 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -511,7 +511,8 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 {
 	int len, i;
 
-	if ((unsigned long)s < PAGE_SIZE)
+	if ((unsigned long)s > (unsigned long)-PAGE_SIZE ||
+	    (unsigned long)s < PAGE_SIZE)
 		s = "(null)";
 
 	len = strnlen(s, spec.precision);
-- 
1.8.4.5

Re: [PATCH 1/2] Fix printk() on ERR_PTR()

[Andrew Morton]

On Sun, 10 May 2015 21:42:15 +0200 Richard Weinberger <richard@nod.at> wrote:

> vbin_printf() checks whether the provided pointer is larger
> than -PAGE_SIZE such that it does not explode on ERR_PTR() pointers.
> printk() does not.
> 
> Let's add this check also to the printk() code such that
> trace_printk() and printk() are consistent again.
> 
> ..
>
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -511,7 +511,8 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
>  {
>  	int len, i;
>  
> -	if ((unsigned long)s < PAGE_SIZE)
> +	if ((unsigned long)s > (unsigned long)-PAGE_SIZE ||

hm, PAGE_SIZE has type ulong, so is the cast needed?

> +	    (unsigned long)s < PAGE_SIZE)

This would be a place for

	if (!within(PAGE_SIZE, (unsigned long)s, -PAGE_SIZE))
  		s = "(null)";

I'm counting at least five implementations of within(), not all the same.

Maybe.  Both the lower and upper bounds will sometimes want inclusive
semantics, sometimes exclusive.

Re: [PATCH 1/2] Fix printk() on ERR_PTR()

[Richard Weinberger]

Am 12.05.2015 um 01:20 schrieb Andrew Morton:
> On Sun, 10 May 2015 21:42:15 +0200 Richard Weinberger <richard@nod.at> wrote:
> 
>> vbin_printf() checks whether the provided pointer is larger
>> than -PAGE_SIZE such that it does not explode on ERR_PTR() pointers.
>> printk() does not.
>>
>> Let's add this check also to the printk() code such that
>> trace_printk() and printk() are consistent again.
>>
>> ..
>>
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -511,7 +511,8 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
>>  {
>>  	int len, i;
>>  
>> -	if ((unsigned long)s < PAGE_SIZE)
>> +	if ((unsigned long)s > (unsigned long)-PAGE_SIZE ||
> 
> hm, PAGE_SIZE has type ulong, so is the cast needed?

But we add a negative sign to it. AFAIK the cast is needed
to prevent gcc from promoting this to a signed long.

>> +	    (unsigned long)s < PAGE_SIZE)
> 
> This would be a place for
> 
> 	if (!within(PAGE_SIZE, (unsigned long)s, -PAGE_SIZE))
>   		s = "(null)";
> 
> I'm counting at least five implementations of within(), not all the same.

Challenge accepted. :D

Thanks,
//richard

[PATCH 2/2] vsprintf: Add support for userspace strings

[Richard Weinberger]

Add %pL format string to print userspace strings.
It works like %s but does copy_from_user() instead
of a memcpy().

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 Documentation/printk-formats.txt |  6 +++
 lib/vsprintf.c                   | 89 ++++++++++++++++++++++++++++++++++------
 2 files changed, 82 insertions(+), 13 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 2216eb1..74f6038 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -284,6 +284,12 @@ bitmap and its derivatives such as cpumask and nodemask:
 
 	Passed by reference.
 
+userspace string:
+
+	%pL
+
+	Like %s but works on strings located in userspace.
+
 Thank you for your cooperation and attention.
 
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 092d5a7..4138340 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -355,6 +355,7 @@ int num_to_str(char *buf, int size, unsigned long long num)
 #define ZEROPAD	16		/* pad with zero, must be 16 == '0' - ' ' */
 #define SMALL	32		/* use lowercase in hex (must be 32 == 0x20) */
 #define SPECIAL	64		/* prefix hex with "0x", octal with "0" */
+#define USER	128		/* value points to user memory */
 
 enum format_type {
 	FORMAT_TYPE_NONE, /* Just a string part */
@@ -510,12 +511,27 @@ static noinline_for_stack
 char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 {
 	int len, i;
+	int is_user = spec.flags & USER;
+
+	if (is_user) {
+		int max_len = min_t(unsigned long, spec.precision, PAGE_SIZE);
+
+		len = strnlen_user(s, max_len);
+		if (len == 0) {
+			s = "(bad address)";
+			len = strnlen(s, spec.precision);
+			is_user = 0;
+		} else {
+			/* strnlen_user() counts also the NUL byte */
+			len--;
+		}
+	} else {
+		if ((unsigned long)s > (unsigned long)-PAGE_SIZE ||
+		    (unsigned long)s < PAGE_SIZE)
+			s = "(null)";
 
-	if ((unsigned long)s > (unsigned long)-PAGE_SIZE ||
-	    (unsigned long)s < PAGE_SIZE)
-		s = "(null)";
-
-	len = strnlen(s, spec.precision);
+		len = strnlen(s, spec.precision);
+	}
 
 	if (!(spec.flags & LEFT)) {
 		while (len < spec.field_width--) {
@@ -525,8 +541,17 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 		}
 	}
 	for (i = 0; i < len; ++i) {
-		if (buf < end)
+		if (buf >= end)
+			goto next;
+
+		if (is_user) {
+			if (get_user(*buf, s)) {
+				/* get_user() failed, add a blank */
+				*buf = ' ';
+			}
+		} else
 			*buf = *s;
+next:
 		++buf; ++s;
 	}
 	while (len < spec.field_width--) {
@@ -1743,7 +1768,19 @@ qualifier:
 
 	case 'p':
 		spec->type = FORMAT_TYPE_PTR;
-		return ++fmt - start;
+		fmt++;
+		/*
+		 * handle %pL here and turn it into a special FORMAT_TYPE_STR
+		 * type.
+		 * %pL is much like the %s case. This way we keep vbin_printf()
+		 * straight forward.
+		 */
+		if (*fmt == 'L') {
+			spec->type = FORMAT_TYPE_STR;
+			spec->flags |= USER;
+			fmt++;
+		}
+		return fmt - start;
 
 	case '%':
 		spec->type = FORMAT_TYPE_PERCENT_CHAR;
@@ -2204,14 +2241,33 @@ do {									\
 
 		case FORMAT_TYPE_STR: {
 			const char *save_str = va_arg(args, char *);
+			int is_user = spec.flags & USER;
 			size_t len;
 
-			if ((unsigned long)save_str > (unsigned long)-PAGE_SIZE
-					|| (unsigned long)save_str < PAGE_SIZE)
-				save_str = "(null)";
-			len = strlen(save_str) + 1;
-			if (str + len < end)
-				memcpy(str, save_str, len);
+			if (is_user) {
+				len = strnlen_user(save_str, PAGE_SIZE);
+				if (len == 0) {
+					save_str = "(bad address)";
+					is_user = 0;
+				} else {
+					if (copy_from_user(str, save_str, len)) {
+						save_str = "(bad address)";
+						is_user = 0;
+					}
+				}
+			}
+
+			if (!is_user) {
+				if ((unsigned long)save_str > (unsigned long)-PAGE_SIZE
+						|| (unsigned long)save_str < PAGE_SIZE)
+					save_str = "(null)";
+
+				len = strlen(save_str) + 1;
+
+				if (str + len < end)
+					memcpy(str, save_str, len);
+			}
+
 			str += len;
 			break;
 		}
@@ -2364,6 +2420,13 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 		case FORMAT_TYPE_STR: {
 			const char *str_arg = args;
 			args += strlen(str_arg) + 1;
+
+			/*
+			 * vbin_printf() sanitized the user provided string
+			 * for us and copied it into our binary buffer.
+			 */
+			spec.flags &= ~USER;
+
 			str = string(str, end, (char *)str_arg, spec);
 			break;
 		}
-- 
1.8.4.5

Re: [PATCH 2/2] vsprintf: Add support for userspace strings

[Joe Perches]

On Sun, 2015-05-10 at 21:42 +0200, Richard Weinberger wrote:
> Add %pL format string to print userspace strings.
> It works like %s but does copy_from_user() instead
> of a memcpy().

I think this would be much simpler in a new
function rather than complicating string()

Re: [PATCH 2/2] vsprintf: Add support for userspace strings

[Richard Weinberger]

Am 10.05.2015 um 22:09 schrieb Joe Perches:
> On Sun, 2015-05-10 at 21:42 +0200, Richard Weinberger wrote:
>> Add %pL format string to print userspace strings.
>> It works like %s but does copy_from_user() instead
>> of a memcpy().
> 
> I think this would be much simpler in a new
> function rather than complicating string()

-ENOPATCH.

Thanks,
//richard

Re: [PATCH 2/2] vsprintf: Add support for userspace strings

[Joe Perches]

On Sun, 2015-05-10 at 22:11 +0200, Richard Weinberger wrote:
> Am 10.05.2015 um 22:09 schrieb Joe Perches:
> > On Sun, 2015-05-10 at 21:42 +0200, Richard Weinberger wrote:
> >> Add %pL format string to print userspace strings.
> >> It works like %s but does copy_from_user() instead
> >> of a memcpy().
> > 
> > I think this would be much simpler in a new
> > function rather than complicating string()
> 
> -ENOPATCH.

It's your patch, I'm just commenting on it.

I'm not sure there's much value in it.

Can it can add security holes if used with %pV?

Re: [PATCH 2/2] vsprintf: Add support for userspace strings

[Richard Weinberger]

Am 10.05.2015 um 22:16 schrieb Joe Perches:
> On Sun, 2015-05-10 at 22:11 +0200, Richard Weinberger wrote:
>> Am 10.05.2015 um 22:09 schrieb Joe Perches:
>>> On Sun, 2015-05-10 at 21:42 +0200, Richard Weinberger wrote:
>>>> Add %pL format string to print userspace strings.
>>>> It works like %s but does copy_from_user() instead
>>>> of a memcpy().
>>>
>>> I think this would be much simpler in a new
>>> function rather than complicating string()
>>
>> -ENOPATCH.
> 
> It's your patch, I'm just commenting on it.

Yeah, if you read string() you'll notice that it adds only
very few lines. Duplicating string() is not a good idea.
%pL is not a completely new feature it just covers a %s corner
case.

> I'm not sure there's much value in it.

Maybe because you're not in the kernel-debugging-business? ;-)

> Can it can add security holes if used with %pV?

If abused every kernel function can be used to add security holes.
As I wrote the goal of %pL is for debugging.

Thanks,
//richard

Re: [PATCH 2/2] vsprintf: Add support for userspace strings

[Andrew Morton]

On Sun, 10 May 2015 13:16:22 -0700 Joe Perches <joe@perches.com> wrote:

> On Sun, 2015-05-10 at 22:11 +0200, Richard Weinberger wrote:
> > Am 10.05.2015 um 22:09 schrieb Joe Perches:
> > > On Sun, 2015-05-10 at 21:42 +0200, Richard Weinberger wrote:
> > >> Add %pL format string to print userspace strings.
> > >> It works like %s but does copy_from_user() instead
> > >> of a memcpy().
> > > 
> > > I think this would be much simpler in a new
> > > function rather than complicating string()
> > 
> > -ENOPATCH.
> 
> It's your patch, I'm just commenting on it.
> 
> I'm not sure there's much value in it.

I can't say I'm terribly excited either.  Are there any existing
callsites which might use this?  The %p namespace is already pretty
crowded and if %pL is just for non-production developer debug use then
it can reasonably live outside the main kernel tree...

Re: [PATCH 2/2] vsprintf: Add support for userspace strings

[Richard Weinberger]

Am 12.05.2015 um 01:23 schrieb Andrew Morton:
> On Sun, 10 May 2015 13:16:22 -0700 Joe Perches <joe@perches.com> wrote:
> 
>> On Sun, 2015-05-10 at 22:11 +0200, Richard Weinberger wrote:
>>> Am 10.05.2015 um 22:09 schrieb Joe Perches:
>>>> On Sun, 2015-05-10 at 21:42 +0200, Richard Weinberger wrote:
>>>>> Add %pL format string to print userspace strings.
>>>>> It works like %s but does copy_from_user() instead
>>>>> of a memcpy().
>>>>
>>>> I think this would be much simpler in a new
>>>> function rather than complicating string()
>>>
>>> -ENOPATCH.
>>
>> It's your patch, I'm just commenting on it.
>>
>> I'm not sure there's much value in it.
> 
> I can't say I'm terribly excited either.  Are there any existing
> callsites which might use this?  The %p namespace is already pretty
> crowded and if %pL is just for non-production developer debug use then
> it can reasonably live outside the main kernel tree...

My indention was to use it for two things:
a) Debugging (at least I use such code from time to time when debugging strange stuff)
b) Using this new format string in ftrace to print strings provided to syscalls instead of raw addresses.

Masami showed me that I can use "perf probe" for for both a) and b).
So let'S drop this patch.

Thanks,
//richard

Re: vsprintf: Add support for userspace strings

[Masami Hiramatsu]

On 2015/05/11 4:42, Richard Weinberger wrote:
> While debugging issues I often add (trace_)printks to strategic positions.
> Dealing with user provided string is complicated as an extra buffer a
> copy_from_user() is needed.
> This adds a new format string to allow direct printing of such strings.
> 
> My initial plan was to use %pU but 'U' is already taken, therefore
> I used the next letter which comes in mind when one thinks of userpace,
> 'L'.
> The %pL format string works exactly like %s.

BTW, if you need to do this for debug, you can also use ftrace's kprobe-tracer
(and perf probe) which allows you to dump userspace strings :)

Here is an example.
-----
[mhiramat@localhost perf]$ ./perf probe -L do_sys_open:0-3
<do_sys_open@/usr/src/debug/kernel-3.10.0-229.1.2.el7/linux-3.10.0-229.1.2.el7.x
      0  long do_sys_open(int dfd, const char __user *filename, int flags, umode
      1  {
                struct open_flags op;
      3         int lookup = build_open_flags(flags, mode, &op);
[mhiramat@localhost perf]$ ./perf probe -V do_sys_open
Available variables at do_sys_open
        @<do_sys_open+0>
                char*   filename
                int     dfd
                int     flags
                int     lookup
                struct open_flags       op
                umode_t mode
[mhiramat@localhost perf]$ sudo ./perf probe do_sys_open filename:string
Added new event:
  probe:do_sys_open    (on do_sys_open with filename:string)

You can now use it in all perf tools, such as:

        perf record -e probe:do_sys_open -aR sleep 1


[mhiramat@localhost perf]$ sudo ./perf record -e probe:do_sys_open -a ls &> /dev/null
[mhiramat@localhost perf]$ sudo ./perf script | more
              ls  7238 [003] 1629305.250347: probe:do_sys_open: (ffffffff811c5e40) filename_string="/etc/ld.so.cache"
              ls  7238 [003] 1629305.250384: probe:do_sys_open: (ffffffff811c5e40) filename_string="/lib64/libselinux.so.1"
              ls  7238 [003] 1629305.250501: probe:do_sys_open: (ffffffff811c5e40) filename_string="/lib64/libcap.so.2"
              ls  7238 [003] 1629305.250562: probe:do_sys_open: (ffffffff811c5e40) filename_string="/lib64/libacl.so.1"
              ls  7238 [003] 1629305.250631: probe:do_sys_open: (ffffffff811c5e40) filename_string="/lib64/libc.so.6"
              ls  7238 [003] 1629305.250706: probe:do_sys_open: (ffffffff811c5e40) filename_string="/lib64/libpcre.so.1"
              ls  7238 [003] 1629305.250769: probe:do_sys_open: (ffffffff811c5e40) filename_string="/lib64/liblzma.so.5"
              ls  7238 [003] 1629305.250838: probe:do_sys_open: (ffffffff811c5e40) filename_string="/lib64/libdl.so.2"
              ls  7238 [003] 1629305.250898: probe:do_sys_open: (ffffffff811c5e40) filename_string="/lib64/libattr.so.1"
              ls  7238 [003] 1629305.250959: probe:do_sys_open: (ffffffff811c5e40) filename_string="/lib64/libpthread.so.0"
              ls  7238 [003] 1629305.251591: probe:do_sys_open: (ffffffff811c5e40) filename_string=""
              ls  7238 [003] 1629305.251695: probe:do_sys_open: (ffffffff811c5e40) filename_string="."
[mhiramat@localhost perf]$ sudo ./perf probe -d \*
Removed event: probe:do_sys_open
-----

Thank you,

-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com

Re: vsprintf: Add support for userspace strings

[Richard Weinberger]

Am 11.05.2015 um 02:24 schrieb Masami Hiramatsu:
> On 2015/05/11 4:42, Richard Weinberger wrote:
>> While debugging issues I often add (trace_)printks to strategic positions.
>> Dealing with user provided string is complicated as an extra buffer a
>> copy_from_user() is needed.
>> This adds a new format string to allow direct printing of such strings.
>>
>> My initial plan was to use %pU but 'U' is already taken, therefore
>> I used the next letter which comes in mind when one thinks of userpace,
>> 'L'.
>> The %pL format string works exactly like %s.
> 
> BTW, if you need to do this for debug, you can also use ftrace's kprobe-tracer
> (and perf probe) which allows you to dump userspace strings :)

Sounds promising!

But I fail to use it:
$ perf probe -vv -L do_sys_open:0-3 -k /boot/vmlinux-4.1.0-rc2-3.g3541e77-vanilla
Line range is 0 to 3
Use vmlinux: /boot/vmlinux-4.1.0-rc2-3.g3541e77-vanilla
Failed to find path of kernel module.
  Error: Failed to show lines. Reason: No such file or directory (Code: -2)

Any idea what's the issue?

Thanks,
//richard

Re: vsprintf: Add support for userspace strings

[Richard Weinberger]

Am 11.05.2015 um 10:59 schrieb Richard Weinberger:
> Am 11.05.2015 um 02:24 schrieb Masami Hiramatsu:
>> On 2015/05/11 4:42, Richard Weinberger wrote:
>>> While debugging issues I often add (trace_)printks to strategic positions.
>>> Dealing with user provided string is complicated as an extra buffer a
>>> copy_from_user() is needed.
>>> This adds a new format string to allow direct printing of such strings.
>>>
>>> My initial plan was to use %pU but 'U' is already taken, therefore
>>> I used the next letter which comes in mind when one thinks of userpace,
>>> 'L'.
>>> The %pL format string works exactly like %s.
>>
>> BTW, if you need to do this for debug, you can also use ftrace's kprobe-tracer
>> (and perf probe) which allows you to dump userspace strings :)
> 
> Sounds promising!
> 
> But I fail to use it:
> $ perf probe -vv -L do_sys_open:0-3 -k /boot/vmlinux-4.1.0-rc2-3.g3541e77-vanilla
> Line range is 0 to 3
> Use vmlinux: /boot/vmlinux-4.1.0-rc2-3.g3541e77-vanilla
> Failed to find path of kernel module.
>   Error: Failed to show lines. Reason: No such file or directory (Code: -2)
> 
> Any idea what's the issue?

Okay, works now as expected. :-)
Had the wrong debuginfo package installed.

Thanks,
//richard

Re: Re: vsprintf: Add support for userspace strings

[Masami Hiramatsu]

On 2015/05/11 19:57, Richard Weinberger wrote:
> Am 11.05.2015 um 10:59 schrieb Richard Weinberger:
>> Am 11.05.2015 um 02:24 schrieb Masami Hiramatsu:
>>> On 2015/05/11 4:42, Richard Weinberger wrote:
>>>> While debugging issues I often add (trace_)printks to strategic positions.
>>>> Dealing with user provided string is complicated as an extra buffer a
>>>> copy_from_user() is needed.
>>>> This adds a new format string to allow direct printing of such strings.
>>>>
>>>> My initial plan was to use %pU but 'U' is already taken, therefore
>>>> I used the next letter which comes in mind when one thinks of userpace,
>>>> 'L'.
>>>> The %pL format string works exactly like %s.
>>>
>>> BTW, if you need to do this for debug, you can also use ftrace's kprobe-tracer
>>> (and perf probe) which allows you to dump userspace strings :)
>>
>> Sounds promising!
>>
>> But I fail to use it:
>> $ perf probe -vv -L do_sys_open:0-3 -k /boot/vmlinux-4.1.0-rc2-3.g3541e77-vanilla
>> Line range is 0 to 3
>> Use vmlinux: /boot/vmlinux-4.1.0-rc2-3.g3541e77-vanilla
>> Failed to find path of kernel module.
>>   Error: Failed to show lines. Reason: No such file or directory (Code: -2)
>>
>> Any idea what's the issue?
> 
> Okay, works now as expected. :-)
> Had the wrong debuginfo package installed.

Ah, the message is not good... If you give "-vv" (double -v options),
you'll see below reason.

symsrc__init: build id mismatch for /boot/vmlinux-4.1.0-rc2-3.g3541e77-vanilla

I'll fix that.

Thank you for reporting!

-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com