* [kernel-hardening] [PATCH v2 1/3] kallsyms: don't leak address when symbol not found
2017-12-19 3:28 [kernel-hardening] [PATCH v2 0/3] kallsyms: don't leak address Tobin C. Harding
@ 2017-12-19 3:28 ` Tobin C. Harding
2017-12-19 3:28 ` [kernel-hardening] [PATCH v2 2/3] vsprintf: print <no-symbol> if " Tobin C. Harding
2017-12-19 3:28 ` [kernel-hardening] [PATCH v2 3/3] trace: print address " Tobin C. Harding
2 siblings, 0 replies; 6+ messages in thread
From: Tobin C. Harding @ 2017-12-19 3:28 UTC (permalink / raw)
To: kernel-hardening
Cc: Tobin C. Harding, Steven Rostedt, Tycho Andersen, Linus Torvalds,
Kees Cook, Andrew Morton, Daniel Borkmann, Masahiro Yamada,
Alexei Starovoitov, linux-kernel, Network Development
Currently if kallsyms_lookup() fails to find the symbol then the address
is printed. This potentially leaks sensitive information but is useful
for debugging. We would like to stop the leak but keep the current
behaviour when needed for debugging. To achieve this we can add a
command-line parameter that if enabled maintains the current
behaviour. If the command-line parameter is not enabled we can return an
error instead of printing the address giving the calling code the option
of how to handle the look up failure.
Add command-line parameter 'insecure_print_all_symbols'. If parameter is
not enabled return an error value instead of printing the raw address.
Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
kernel/kallsyms.c | 31 +++++++++++++++++++++++++------
1 file changed, 25 insertions(+), 6 deletions(-)
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index d5fa4116688a..2707cf751437 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -383,6 +383,16 @@ int lookup_symbol_attrs(unsigned long addr, unsigned long *size,
return lookup_module_symbol_attrs(addr, size, offset, modname, name);
}
+/* Enables printing of raw address when symbol look up fails */
+static bool insecure_print_all_symbols;
+
+static int __init enable_insecure_print_all_symbols(char *unused)
+{
+ insecure_print_all_symbols = true;
+ return 0;
+}
+early_param("insecure_print_all_symbols", enable_insecure_print_all_symbols);
+
/* Look up a kernel symbol and return it in a text buffer. */
static int __sprint_symbol(char *buffer, unsigned long address,
int symbol_offset, int add_offset)
@@ -394,8 +404,15 @@ static int __sprint_symbol(char *buffer, unsigned long address,
address += symbol_offset;
name = kallsyms_lookup(address, &size, &offset, &modname, buffer);
- if (!name)
- return sprintf(buffer, "0x%lx", address - symbol_offset);
+ if (insecure_print_all_symbols) {
+ if (!name)
+ return sprintf(buffer, "0x%lx", address - symbol_offset);
+ } else {
+ if (!name) {
+ buffer[0] = '\0';
+ return -1;
+ }
+ }
if (name != buffer)
strcpy(buffer, name);
@@ -417,8 +434,9 @@ static int __sprint_symbol(char *buffer, unsigned long address,
* @address: address to lookup
*
* This function looks up a kernel symbol with @address and stores its name,
- * offset, size and module name to @buffer if possible. If no symbol was found,
- * just saves its @address as is.
+ * offset, size and module name to @buffer if possible. If no symbol was found
+ * returns -1 unless kernel command-line parameter 'insecure_print_all_symbols'
+ * is enabled, in which case saves @address as is to buffer.
*
* This function returns the number of bytes stored in @buffer.
*/
@@ -434,8 +452,9 @@ EXPORT_SYMBOL_GPL(sprint_symbol);
* @address: address to lookup
*
* This function looks up a kernel symbol with @address and stores its name
- * and module name to @buffer if possible. If no symbol was found, just saves
- * its @address as is.
+ * and module name to @buffer if possible. If no symbol was found, returns -1
+ * unless kernel command-line parameter 'insecure_print_all_symbols' is enabled,
+ * in which case saves @address as is to buffer.
*
* This function returns the number of bytes stored in @buffer.
*/
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* [kernel-hardening] [PATCH v2 2/3] vsprintf: print <no-symbol> if symbol not found
2017-12-19 3:28 [kernel-hardening] [PATCH v2 0/3] kallsyms: don't leak address Tobin C. Harding
2017-12-19 3:28 ` [kernel-hardening] [PATCH v2 1/3] kallsyms: don't leak address when symbol not found Tobin C. Harding
@ 2017-12-19 3:28 ` Tobin C. Harding
2017-12-19 6:18 ` [kernel-hardening] " Joe Perches
2017-12-19 3:28 ` [kernel-hardening] [PATCH v2 3/3] trace: print address " Tobin C. Harding
2 siblings, 1 reply; 6+ messages in thread
From: Tobin C. Harding @ 2017-12-19 3:28 UTC (permalink / raw)
To: kernel-hardening
Cc: Tobin C. Harding, Steven Rostedt, Tycho Andersen, Linus Torvalds,
Kees Cook, Andrew Morton, Daniel Borkmann, Masahiro Yamada,
Alexei Starovoitov, linux-kernel, Network Development
Depends on: commit 40eee173a35e ("kallsyms: don't leak address when
symbol not found")
Currently vsprintf for specifiers %p[SsB] relies on the behaviour of
kallsyms (sprint_symbol()) and prints the actual address if a symbol is
not found. Previous patch changes this behaviour so that sprint_symbol()
returns an error if symbol not found. With this patch in place we can
print a sanitized message '<symbol not found>' instead of leaking the
address.
Print '<symbol not found>' for printk specifier %p[sSB] if symbol look
up fails.
Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
lib/vsprintf.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 01c3957b2de6..820ed4fe6e6c 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -674,6 +674,8 @@ char *symbol_string(char *buf, char *end, void *ptr,
unsigned long value;
#ifdef CONFIG_KALLSYMS
char sym[KSYM_SYMBOL_LEN];
+ const char *sym_not_found = "<symbol not found>";
+ int ret;
#endif
if (fmt[1] == 'R')
@@ -682,11 +684,14 @@ char *symbol_string(char *buf, char *end, void *ptr,
#ifdef CONFIG_KALLSYMS
if (*fmt == 'B')
- sprint_backtrace(sym, value);
+ ret = sprint_backtrace(sym, value);
else if (*fmt != 'f' && *fmt != 's')
- sprint_symbol(sym, value);
+ ret = sprint_symbol(sym, value);
else
- sprint_symbol_no_offset(sym, value);
+ ret = sprint_symbol_no_offset(sym, value);
+
+ if (ret == -1)
+ strcpy(sym, sym_not_found);
return string(buf, end, sym, spec);
#else
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* [kernel-hardening] Re: [PATCH v2 2/3] vsprintf: print <no-symbol> if symbol not found
2017-12-19 3:28 ` [kernel-hardening] [PATCH v2 2/3] vsprintf: print <no-symbol> if " Tobin C. Harding
@ 2017-12-19 6:18 ` Joe Perches
2017-12-19 6:33 ` Tobin C. Harding
0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2017-12-19 6:18 UTC (permalink / raw)
To: Tobin C. Harding, kernel-hardening
Cc: Steven Rostedt, Tycho Andersen, Linus Torvalds, Kees Cook,
Andrew Morton, Daniel Borkmann, Masahiro Yamada,
Alexei Starovoitov, linux-kernel, Network Development
On Tue, 2017-12-19 at 14:28 +1100, Tobin C. Harding wrote:
> Depends on: commit 40eee173a35e ("kallsyms: don't leak address when
> symbol not found")
>
> Currently vsprintf for specifiers %p[SsB] relies on the behaviour of
> kallsyms (sprint_symbol()) and prints the actual address if a symbol is
> not found. Previous patch changes this behaviour so that sprint_symbol()
> returns an error if symbol not found. With this patch in place we can
> print a sanitized message '<symbol not found>' instead of leaking the
> address.
>
> Print '<symbol not found>' for printk specifier %p[sSB] if symbol look
> up fails.
>
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> ---
> lib/vsprintf.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 01c3957b2de6..820ed4fe6e6c 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -674,6 +674,8 @@ char *symbol_string(char *buf, char *end, void *ptr,
> unsigned long value;
> #ifdef CONFIG_KALLSYMS
> char sym[KSYM_SYMBOL_LEN];
> + const char *sym_not_found = "<symbol not found>";
This will be reinitialized on every use.
> + int ret;
> #endif
>
> if (fmt[1] == 'R')
> @@ -682,11 +684,14 @@ char *symbol_string(char *buf, char *end, void *ptr,
>
> #ifdef CONFIG_KALLSYMS
> if (*fmt == 'B')
> - sprint_backtrace(sym, value);
> + ret = sprint_backtrace(sym, value);
> else if (*fmt != 'f' && *fmt != 's')
> - sprint_symbol(sym, value);
> + ret = sprint_symbol(sym, value);
> else
> - sprint_symbol_no_offset(sym, value);
> + ret = sprint_symbol_no_offset(sym, value);
> +
> + if (ret == -1)
> + strcpy(sym, sym_not_found);
This could avoid the unnecessary strcpy if sym_not_found
was not used at all and this was used instead
if (ret == -1)
return string(buf, end, "<symbol not found>", spec);
return string(buf, end, sym, spec);
or maybe
return string(buf, end, ret == -1 ? "<symbol not found>" : sum, spec);
>
> return string(buf, end, sym, spec);
> #else
^ permalink raw reply [flat|nested] 6+ messages in thread* [kernel-hardening] Re: [PATCH v2 2/3] vsprintf: print <no-symbol> if symbol not found
2017-12-19 6:18 ` [kernel-hardening] " Joe Perches
@ 2017-12-19 6:33 ` Tobin C. Harding
0 siblings, 0 replies; 6+ messages in thread
From: Tobin C. Harding @ 2017-12-19 6:33 UTC (permalink / raw)
To: Joe Perches
Cc: kernel-hardening, Steven Rostedt, Tycho Andersen, Linus Torvalds,
Kees Cook, Andrew Morton, Daniel Borkmann, Masahiro Yamada,
Alexei Starovoitov, linux-kernel, Network Development
On Mon, Dec 18, 2017 at 10:18:27PM -0800, Joe Perches wrote:
> On Tue, 2017-12-19 at 14:28 +1100, Tobin C. Harding wrote:
> > Depends on: commit 40eee173a35e ("kallsyms: don't leak address when
> > symbol not found")
> >
> > Currently vsprintf for specifiers %p[SsB] relies on the behaviour of
> > kallsyms (sprint_symbol()) and prints the actual address if a symbol is
> > not found. Previous patch changes this behaviour so that sprint_symbol()
> > returns an error if symbol not found. With this patch in place we can
> > print a sanitized message '<symbol not found>' instead of leaking the
> > address.
> >
> > Print '<symbol not found>' for printk specifier %p[sSB] if symbol look
> > up fails.
> >
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > ---
> > lib/vsprintf.c | 11 ++++++++---
> > 1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 01c3957b2de6..820ed4fe6e6c 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -674,6 +674,8 @@ char *symbol_string(char *buf, char *end, void *ptr,
> > unsigned long value;
> > #ifdef CONFIG_KALLSYMS
> > char sym[KSYM_SYMBOL_LEN];
> > + const char *sym_not_found = "<symbol not found>";
>
> This will be reinitialized on every use.
>
> > + int ret;
> > #endif
> >
> > if (fmt[1] == 'R')
> > @@ -682,11 +684,14 @@ char *symbol_string(char *buf, char *end, void *ptr,
> >
> > #ifdef CONFIG_KALLSYMS
> > if (*fmt == 'B')
> > - sprint_backtrace(sym, value);
> > + ret = sprint_backtrace(sym, value);
> > else if (*fmt != 'f' && *fmt != 's')
> > - sprint_symbol(sym, value);
> > + ret = sprint_symbol(sym, value);
> > else
> > - sprint_symbol_no_offset(sym, value);
> > + ret = sprint_symbol_no_offset(sym, value);
> > +
> > + if (ret == -1)
> > + strcpy(sym, sym_not_found);
>
>
> This could avoid the unnecessary strcpy if sym_not_found
> was not used at all and this was used instead
>
> if (ret == -1)
> return string(buf, end, "<symbol not found>", spec);
>
> return string(buf, end, sym, spec);
>
> or maybe
>
> return string(buf, end, ret == -1 ? "<symbol not found>" : sum, spec);
Oh, thanks. This is much cleaner. Will re-spin.
thanks,
Tobin.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [kernel-hardening] [PATCH v2 3/3] trace: print address if symbol not found
2017-12-19 3:28 [kernel-hardening] [PATCH v2 0/3] kallsyms: don't leak address Tobin C. Harding
2017-12-19 3:28 ` [kernel-hardening] [PATCH v2 1/3] kallsyms: don't leak address when symbol not found Tobin C. Harding
2017-12-19 3:28 ` [kernel-hardening] [PATCH v2 2/3] vsprintf: print <no-symbol> if " Tobin C. Harding
@ 2017-12-19 3:28 ` Tobin C. Harding
2 siblings, 0 replies; 6+ messages in thread
From: Tobin C. Harding @ 2017-12-19 3:28 UTC (permalink / raw)
To: kernel-hardening
Cc: Tobin C. Harding, Steven Rostedt, Tycho Andersen, Linus Torvalds,
Kees Cook, Andrew Morton, Daniel Borkmann, Masahiro Yamada,
Alexei Starovoitov, linux-kernel, Network Development
Fixes behaviour modified by: commit 40eee173a35e ("kallsyms: don't leak
address when symbol not found")
Previous patch changed behaviour of kallsyms function sprint_symbol() to
return an error code instead of printing the address if a symbol was not
found. Ftrace relies on the original behaviour. We should not break
tracing when applying the previous patch. We can maintain the original
behaviour by checking the return code on calls to sprint_symbol() and
friends.
Check return code and print actual address on error (i.e symbol not
found).
Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
kernel/trace/trace.h | 24 ++++++++++++++++++++++++
kernel/trace/trace_events_hist.c | 6 +++---
kernel/trace/trace_output.c | 2 +-
3 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 2a6d0325a761..881b1a577d75 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1814,4 +1814,28 @@ static inline void trace_event_eval_update(struct trace_eval_map **map, int len)
extern struct trace_iterator *tracepoint_print_iter;
+static inline int
+trace_sprint_symbol(char *buffer, unsigned long address)
+{
+ int ret;
+
+ ret = sprint_symbol(buffer, address);
+ if (ret == -1)
+ ret = sprintf(buffer, "0x%lx", address);
+
+ return ret;
+}
+
+static inline int
+trace_sprint_symbol_no_offset(char *buffer, unsigned long address)
+{
+ int ret;
+
+ ret = sprint_symbol_no_offset(buffer, address);
+ if (ret == -1)
+ ret = sprintf(buffer, "0x%lx", address);
+
+ return ret;
+}
+
#endif /* _LINUX_KERNEL_TRACE_H */
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 1e1558c99d56..ca523327c058 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -982,7 +982,7 @@ static void hist_trigger_stacktrace_print(struct seq_file *m,
return;
seq_printf(m, "%*c", 1 + spaces, ' ');
- sprint_symbol(str, stacktrace_entries[i]);
+ trace_sprint_symbol(str, stacktrace_entries[i]);
seq_printf(m, "%s\n", str);
}
}
@@ -1014,12 +1014,12 @@ hist_trigger_entry_print(struct seq_file *m,
seq_printf(m, "%s: %llx", field_name, uval);
} else if (key_field->flags & HIST_FIELD_FL_SYM) {
uval = *(u64 *)(key + key_field->offset);
- sprint_symbol_no_offset(str, uval);
+ trace_sprint_symbol_no_offset(str, uval);
seq_printf(m, "%s: [%llx] %-45s", field_name,
uval, str);
} else if (key_field->flags & HIST_FIELD_FL_SYM_OFFSET) {
uval = *(u64 *)(key + key_field->offset);
- sprint_symbol(str, uval);
+ trace_sprint_symbol(str, uval);
seq_printf(m, "%s: [%llx] %-55s", field_name,
uval, str);
} else if (key_field->flags & HIST_FIELD_FL_EXECNAME) {
diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index 90db994ac900..f3c3a0a60f72 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -365,7 +365,7 @@ seq_print_sym_offset(struct trace_seq *s, const char *fmt,
#ifdef CONFIG_KALLSYMS
const char *name;
- sprint_symbol(str, address);
+ trace_sprint_symbol(str, address);
name = kretprobed(str);
if (name && strlen(name)) {
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread