| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20190509232555.bd1f6aad78202d8f4a38ced1@kernel.org>
Date: Thu, 9 May 2019 23:25:55 +0900
From: Masami Hiramatsu <mhiramat@...nel.org>
To: Ingo Molnar <mingo@...nel.org>
Cc: Steven Rostedt <rostedt@...dmis.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Shuah Khan <shuah@...nel.org>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
linux-kernel@...r.kernel.org,
Andy Lutomirski <luto@...capital.net>,
Andrew Morton <akpm@...ux-foundation.org>,
Changbin Du <changbin.du@...il.com>,
Jann Horn <jannh@...gle.com>,
Kees Cook <keescook@...omium.org>,
Andy Lutomirski <luto@...nel.org>,
Alexei Starovoitov <alexei.starovoitov@...il.com>,
Nadav Amit <namit@...are.com>,
Joel Fernandes <joel@...lfernandes.org>, yhs@...com
Subject: Re: [PATCH v7 3/6] tracing/probe: Add ustring type for user-space
string
On Thu, 9 May 2019 11:15:07 +0200
Ingo Molnar <mingo@...nel.org> wrote:
>
> * Masami Hiramatsu <mhiramat@...nel.org> wrote:
>
> > Add "ustring" type for fetching user-space string from kprobe event.
> > User can specify ustring type at uprobe event, and it is same as
> > "string" for uprobe.
> >
> > Note that probe-event provides this option but it doesn't choose the
> > correct type automatically since we have not way to decide the address
> > is in user-space or not on some arch (and on some other arch, you can
> > fetch the string by "string" type). So user must carefully check the
> > target code (e.g. if you see __user on the target variable) and
> > use this new type.
> >
> > Signed-off-by: Masami Hiramatsu <mhiramat@...nel.org>
> > Acked-by: Steven Rostedt (VMware) <rostedt@...dmis.org>
> >
> > ---
> > Changes in v5:
> > - Use strnlen_unsafe_user() in fetch_store_strlen_user().
> > Changes in v2:
> > - Use strnlen_user() instead of open code for fetch_store_strlen_user().
> > ---
> > Documentation/trace/kprobetrace.rst | 9 +++++++--
> > kernel/trace/trace.c | 2 +-
> > kernel/trace/trace_kprobe.c | 29 +++++++++++++++++++++++++++++
> > kernel/trace/trace_probe.c | 14 +++++++++++---
> > kernel/trace/trace_probe.h | 1 +
> > kernel/trace/trace_probe_tmpl.h | 14 +++++++++++++-
> > kernel/trace/trace_uprobe.c | 12 ++++++++++++
> > 7 files changed, 74 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/trace/kprobetrace.rst b/Documentation/trace/kprobetrace.rst
> > index 235ce2ab131a..a3ac7c9ac242 100644
> > --- a/Documentation/trace/kprobetrace.rst
> > +++ b/Documentation/trace/kprobetrace.rst
> > @@ -55,7 +55,8 @@ Synopsis of kprobe_events
> > NAME=FETCHARG : Set NAME as the argument name of FETCHARG.
> > FETCHARG:TYPE : Set TYPE as the type of FETCHARG. Currently, basic types
> > (u8/u16/u32/u64/s8/s16/s32/s64), hexadecimal types
> > - (x8/x16/x32/x64), "string" and bitfield are supported.
> > + (x8/x16/x32/x64), "string", "ustring" and bitfield
> > + are supported.
> >
> > (\*1) only for the probe on function entry (offs == 0).
> > (\*2) only for return probe.
> > @@ -77,7 +78,11 @@ apply it to registers/stack-entries etc. (for example, '$stack1:x8[8]' is
> > wrong, but '+8($stack):x8[8]' is OK.)
> > String type is a special type, which fetches a "null-terminated" string from
> > kernel space. This means it will fail and store NULL if the string container
> > -has been paged out.
> > +has been paged out. "ustring" type is an alternative of string for user-space.
> > +Note that kprobe-event provides string/ustring types, but doesn't change it
> > +automatically. So user has to decide if the targe string in kernel or in user
> > +space carefully. On some arch, if you choose wrong one, it always fails to
> > +record string data.
> > The string array type is a bit different from other types. For other base
> > types, <base-type>[1] is equal to <base-type> (e.g. +0(%di):x32[1] is same
> > as +0(%di):x32.) But string[1] is not equal to string. The string type itself
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index dcb9adb44be9..101a5d09a632 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -4858,7 +4858,7 @@ static const char readme_msg[] =
> > "\t $stack<index>, $stack, $retval, $comm\n"
> > #endif
> > "\t type: s8/16/32/64, u8/16/32/64, x8/16/32/64, string, symbol,\n"
> > - "\t b<bit-width>@<bit-offset>/<container-size>,\n"
> > + "\t b<bit-width>@<bit-offset>/<container-size>, ustring,\n"
> > "\t <type>\\[<array-size>\\]\n"
> > #ifdef CONFIG_HIST_TRIGGERS
> > "\t field: <stype> <name>;\n"
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 7d736248a070..fcb8806fc93c 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -886,6 +886,14 @@ fetch_store_strlen(unsigned long addr)
> > return (ret < 0) ? ret : len;
> > }
> >
> > +/* Return the length of string -- including null terminal byte */
> > +static nokprobe_inline int
> > +fetch_store_strlen_user(unsigned long addr)
> > +{
> > + return strnlen_unsafe_user((__force const void __user *)addr,
> > + MAX_STRING_SIZE);
> > +}
> > +
> > /*
> > * Fetch a null-terminated string. Caller MUST set *(u32 *)buf with max
> > * length and relative data location.
> > @@ -910,6 +918,27 @@ fetch_store_string(unsigned long addr, void *dest, void *base)
> > return ret;
> > }
> >
> > +/*
> > + * Fetch a null-terminated string from user. Caller MUST set *(u32 *)buf
> > + * with max length and relative data location.
> > + */
> > +static nokprobe_inline int
> > +fetch_store_string_user(unsigned long addr, void *dest, void *base)
> > +{
> > + int maxlen = get_loc_len(*(u32 *)dest);
> > + u8 *dst = get_loc_data(dest, base);
> > + long ret;
> > +
> > + if (unlikely(!maxlen))
> > + return -ENOMEM;
> > + ret = strncpy_from_unsafe_user(dst, (__force const void __user *)addr,
> > + maxlen);
>
> Pointless line break.
OK.
>
> > +
> > + if (ret >= 0)
> > + *(u32 *)dest = make_data_loc(ret, (void *)dst - base);
> > + return ret;
> > +}
>
> Plus the problem as in the earlier patch.
OK, I'll fix it too.
Thank you!
>
> Thanks,
>
> Ingo
--
Masami Hiramatsu <mhiramat@...nel.org>
Powered by blists - more mailing lists