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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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()
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
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?
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
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: [email protected]
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
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
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: [email protected]
On Sun, 10 May 2015 21:42:15 +0200 Richard Weinberger <[email protected]> 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.
On Sun, 10 May 2015 13:16:22 -0700 Joe Perches <[email protected]> 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...
Am 12.05.2015 um 01:20 schrieb Andrew Morton:
> On Sun, 10 May 2015 21:42:15 +0200 Richard Weinberger <[email protected]> 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
Am 12.05.2015 um 01:23 schrieb Andrew Morton:
> On Sun, 10 May 2015 13:16:22 -0700 Joe Perches <[email protected]> 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