[kernel-hardening] [PATCH 0/3] kallsyms: don't leak address

[kernel-hardening] [PATCH 0/3] kallsyms: don't leak address

[Tobin C. Harding]

This set plugs a kernel address leak that occurs if kallsyms symbol
look up fails. This set was prompted by a leaking address found using
scripts/leaking_addresses.pl on a PowerPC machine in the wild.

Patch set does not change behaviour when KALLSYMS is not defined
(suggested by Linus).

RFC has been in flight for 3 weeks with no negative response.

Patch 1 - return error code if symbol look up fails.
Patch 2 - print <no-symbol> to buffer if symbol look up returns an error.
Patch 3 - maintain current behaviour in ftrace.

Patch 3 (the ftrace stuff) is untested.

thanks,
Tobin.

Tobin C. Harding (3):
  kallsyms: don't leak address when symbol not found
  vsprintf: print <no-symbol> if symbol not found
  trace: print address if symbol not found

 include/linux/kernel.h           |  2 ++
 kernel/kallsyms.c                |  6 ++++--
 kernel/trace/trace.h             | 24 ++++++++++++++++++++++++
 kernel/trace/trace_events_hist.c |  6 +++---
 lib/vsprintf.c                   | 18 +++++++++++++++---
 5 files changed, 48 insertions(+), 8 deletions(-)

-- 
2.7.4

[kernel-hardening] [PATCH 1/3] kallsyms: don't leak address when symbol not found

[Tobin C. Harding]

Currently if kallsyms_lookup() fails to find the symbol then the address
is printed. This potentially leaks sensitive information. Instead of
printing the address we can return an error, giving the calling code the
option to print the address or print some sanitized message.

Return error instead of printing address to argument buffer. Leave
buffer in a sane state.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 kernel/kallsyms.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index d5fa4116688a..23b9336c1461 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -394,8 +394,10 @@ 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 (!name) {
+		buffer[0] = '\0';
+		return -1;
+	}
 
 	if (name != buffer)
 		strcpy(buffer, name);
-- 
2.7.4

[kernel-hardening] [PATCH 2/3] vsprintf: print <no-symbol> if symbol not found

[Tobin C. Harding]

Depends on: commit bd6b239cdbb2 ("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 tha sprint_symbol()
returns an error if symbol not found. With this patch in place we can
print a sanitized message '<no-symbol>' instead of leaking the address.

Print '<no-symbol>' for printk specifier %s[sSB] if no symbol is found.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 include/linux/kernel.h |  2 ++
 lib/vsprintf.c         | 18 +++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index ce51455e2adf..89e8ce79c2d1 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -460,6 +460,8 @@ char *kvasprintf(gfp_t gfp, const char *fmt, va_list args);
 extern __printf(2, 0)
 const char *kvasprintf_const(gfp_t gfp, const char *fmt, va_list args);
 
+extern int string_is_no_symbol(const char *s);
+
 extern __scanf(2, 3)
 int sscanf(const char *, const char *, ...);
 extern __scanf(2, 0)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 01c3957b2de6..c112b0980ead 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -667,6 +667,8 @@ char *bdev_name(char *buf, char *end, struct block_device *bdev,
 }
 #endif
 
+#define PRINTK_NO_SYMBOL_STR "<no-symbol>"
+
 static noinline_for_stack
 char *symbol_string(char *buf, char *end, void *ptr,
 		    struct printf_spec spec, const char *fmt)
@@ -674,6 +676,7 @@ char *symbol_string(char *buf, char *end, void *ptr,
 	unsigned long value;
 #ifdef CONFIG_KALLSYMS
 	char sym[KSYM_SYMBOL_LEN];
+	int ret;
 #endif
 
 	if (fmt[1] == 'R')
@@ -682,11 +685,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, PRINTK_NO_SYMBOL_STR);
 
 	return string(buf, end, sym, spec);
 #else
@@ -694,6 +700,12 @@ char *symbol_string(char *buf, char *end, void *ptr,
 #endif
 }
 
+int string_is_no_symbol(const char *s)
+{
+	return !!strstr(s, PRINTK_NO_SYMBOL_STR);
+}
+EXPORT_SYMBOL(string_is_no_symbol);
+
 static noinline_for_stack
 char *resource_string(char *buf, char *end, struct resource *res,
 		      struct printf_spec spec, const char *fmt)
-- 
2.7.4

[kernel-hardening] [PATCH 3/3] trace: print address if symbol not found

[Tobin C. Harding]

Fixes behaviour modified by: commit bd6b239cdbb2 ("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 +++---
 2 files changed, 27 insertions(+), 3 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..3e28522a76f4 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_addr(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) {
-- 
2.7.4

[kernel-hardening] Re: [PATCH 2/3] vsprintf: print <no-symbol> if symbol not found

[Joe Perches]

On Mon, 2017-12-18 at 10:53 +1100, Tobin C. Harding wrote:
> Depends on: commit bd6b239cdbb2 ("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 tha sprint_symbol()

tha->that

> returns an error if symbol not found. With this patch in place we can
> print a sanitized message '<no-symbol>' instead of leaking the address.
> 
> Print '<no-symbol>' for printk specifier %s[sSB] if no symbol is found.

%s->%ps

> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
[]
> @@ -460,6 +460,8 @@ char *kvasprintf(gfp_t gfp, const char *fmt, va_list args);
>  extern __printf(2, 0)
>  const char *kvasprintf_const(gfp_t gfp, const char *fmt, va_list args);
>  
> +extern int string_is_no_symbol(const char *s);
> +
[]
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> 
> +#define PRINTK_NO_SYMBOL_STR "<no-symbol>"

"<symbol unavailable>" ?  "not found"?

[]

> +int string_is_no_symbol(const char *s)
> +{
> +	return !!strstr(s, PRINTK_NO_SYMBOL_STR);
> +}
> +EXPORT_SYMBOL(string_is_no_symbol);

Why should string_is_no_symbol be exported?

[kernel-hardening] Re: [PATCH 2/3] vsprintf: print <no-symbol> if symbol not found

[Tobin C. Harding]

On Sun, Dec 17, 2017 at 04:04:14PM -0800, Joe Perches wrote:
> On Mon, 2017-12-18 at 10:53 +1100, Tobin C. Harding wrote:
> > Depends on: commit bd6b239cdbb2 ("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 tha sprint_symbol()
> 
> tha->that
> 
> > returns an error if symbol not found. With this patch in place we can
> > print a sanitized message '<no-symbol>' instead of leaking the address.
> > 
> > Print '<no-symbol>' for printk specifier %s[sSB] if no symbol is found.
> 
> %s->%ps
> 
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> []
> > @@ -460,6 +460,8 @@ char *kvasprintf(gfp_t gfp, const char *fmt, va_list args);
> >  extern __printf(2, 0)
> >  const char *kvasprintf_const(gfp_t gfp, const char *fmt, va_list args);
> >  
> > +extern int string_is_no_symbol(const char *s);
> > +
> []
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > 
> > +#define PRINTK_NO_SYMBOL_STR "<no-symbol>"
> 
> "<symbol unavailable>" ?  "not found"?

<symbol not found>?

> []
> 
> > +int string_is_no_symbol(const char *s)
> > +{
> > +	return !!strstr(s, PRINTK_NO_SYMBOL_STR);
> > +}
> > +EXPORT_SYMBOL(string_is_no_symbol);
> 
> Why should string_is_no_symbol be exported?

I ummed and ahhed about this. By your comment I'm guessing I made the
wrong choice. The idea behind exporting the symbol was so users of
vsprintf could have a way to see if the symbol was found or not without
having to know what string was actually printed.

I'll remove it for v3 and implement your other suggestions.

thanks for the review,
Tobin.

Re: [kernel-hardening] [PATCH 0/3] kallsyms: don't leak address

[Michael Ellerman]

"Tobin C. Harding" <me@tobin.cc> writes:

> This set plugs a kernel address leak that occurs if kallsyms symbol
> look up fails. This set was prompted by a leaking address found using
> scripts/leaking_addresses.pl on a PowerPC machine in the wild.

Any details on that? I haven't heard about it.

cheers

Re: [kernel-hardening] [PATCH 0/3] kallsyms: don't leak address

[Tobin C. Harding]

On Mon, Dec 18, 2017 at 04:31:25PM +1100, Michael Ellerman wrote:
> "Tobin C. Harding" <me@tobin.cc> writes:
> 
> > This set plugs a kernel address leak that occurs if kallsyms symbol
> > look up fails. This set was prompted by a leaking address found using
> > scripts/leaking_addresses.pl on a PowerPC machine in the wild.
> 
> Any details on that? I haven't heard about it.

I have an account on a server in USA. I'm not sure how much detail I
should give on the mailing list in case it is sensitive information. I
will email you off list. Responding here for the benefit of the list.

If this is not the correct way to handle this please say so.

thanks,
Tobin.

Re: [kernel-hardening] [PATCH 0/3] kallsyms: don't leak address

[Tobin C. Harding]

On Mon, Dec 18, 2017 at 05:00:48PM +1100, Tobin C. Harding wrote:
> On Mon, Dec 18, 2017 at 04:31:25PM +1100, Michael Ellerman wrote:
> > "Tobin C. Harding" <me@tobin.cc> writes:
> > 
> > > This set plugs a kernel address leak that occurs if kallsyms symbol
> > > look up fails. This set was prompted by a leaking address found using
> > > scripts/leaking_addresses.pl on a PowerPC machine in the wild.
> > 
> > Any details on that? I haven't heard about it.
> 
> I have an account on a server in USA. I'm not sure how much detail I
> should give on the mailing list in case it is sensitive information. I
> will email you off list. Responding here for the benefit of the list.

Geez I'm a goose. Here is the details with a sanitized address.

/proc/8025/task/8025/stack: [<0000000000000000>] 0xc0000001XXXXXXXX

$ uname -r
4.4.0-79-powerpc64-smp

thanks,
Tobin.

[kernel-hardening] Re: [PATCH 1/3] kallsyms: don't leak address when symbol not found

[Felix Fietkau]

On 2017-12-18 00:53, Tobin C. Harding wrote:
> Currently if kallsyms_lookup() fails to find the symbol then the address
> is printed. This potentially leaks sensitive information. Instead of
> printing the address we can return an error, giving the calling code the
> option to print the address or print some sanitized message.
> 
> Return error instead of printing address to argument buffer. Leave
> buffer in a sane state.
> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
I think there should be a way to keep the old behavior for debugging.

- Felix

[kernel-hardening] Re: [PATCH 3/3] trace: print address if symbol not found

[Steven Rostedt]

On Mon, 18 Dec 2017 10:53:32 +1100
"Tobin C. Harding" <me@tobin.cc> wrote:

> Fixes behaviour modified by: commit bd6b239cdbb2 ("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 +++---
>  2 files changed, 27 insertions(+), 3 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..3e28522a76f4 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_addr(str, stacktrace_entries[i]);

Hmm, where is trace_sprint_symbol_addr() defined?

-- Steve

>  		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) {

[kernel-hardening] Re: [PATCH 3/3] trace: print address if symbol not found

[Tobin C. Harding]

On Mon, Dec 18, 2017 at 11:49:47AM -0500, Steven Rostedt wrote:
> On Mon, 18 Dec 2017 10:53:32 +1100
> "Tobin C. Harding" <me@tobin.cc> wrote:
> 
> > Fixes behaviour modified by: commit bd6b239cdbb2 ("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 +++---
> >  2 files changed, 27 insertions(+), 3 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..3e28522a76f4 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_addr(str, stacktrace_entries[i]);
> 
> Hmm, where is trace_sprint_symbol_addr() defined?

Gee, seems I can't build a kernel and I can't read a diff - epic
fail. Sorry for wasting your time Steve I'll try to be more careful.

FTR This should have been 

-		sprint_symbol(str, stacktrace_entries[i]);
+		trace_sprint_symbol(str, stacktrace_entries[i]);

If you have the time to give me some brief pointers on how I should go
about testing this I'd love to test it before the next version. I know
very little about ftrace.

thanks,
Tobin.

[kernel-hardening] Re: [PATCH 3/3] trace: print address if symbol not found

[Tobin C. Harding]

On Mon, Dec 18, 2017 at 11:49:47AM -0500, Steven Rostedt wrote:
> On Mon, 18 Dec 2017 10:53:32 +1100
> "Tobin C. Harding" <me@tobin.cc> wrote:
> 
> > Fixes behaviour modified by: commit bd6b239cdbb2 ("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 +++---
> >  2 files changed, 27 insertions(+), 3 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..3e28522a76f4 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_addr(str, stacktrace_entries[i]);
> 
> Hmm, where is trace_sprint_symbol_addr() defined?
> 
> -- Steve

Also, I missed one in kernel/trace/trace_output.c

Added for next version.

thanks,
Tobin.

[kernel-hardening] Re: [PATCH 1/3] kallsyms: don't leak address when symbol not found

[Tobin C. Harding]

On Mon, Dec 18, 2017, at 20:55, Felix Fietkau wrote:
> On 2017-12-18 00:53, Tobin C. Harding wrote:
> > Currently if kallsyms_lookup() fails to find the symbol then the address
> > is printed. This potentially leaks sensitive information. Instead of
> > printing the address we can return an error, giving the calling code the
> > option to print the address or print some sanitized message.
> > 
> > Return error instead of printing address to argument buffer. Leave
> > buffer in a sane state.
> > 
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> I think there should be a way to keep the old behavior for debugging.

That was the intended use of

EXPORT_SYMBOL(string_is_no_symbol);

in patch 2 of this series. Then if debugging behaviour is adversely
effected one could use string_is_no_symbol() on a case by case basis to
add back in the original behaviour. 

Current suggestion on list is to remove this function. Do you have a use
case in mind where debugging will break? We could add a fix to this
series if so. Otherwise next version will likely drop
string_is_no_symbol()

thanks,
Tobin. 

[kernel-hardening] Re: [PATCH 1/3] kallsyms: don't leak address when symbol not found

[Steven Rostedt]

On Tue, 19 Dec 2017 09:41:29 +1100
"Tobin C. Harding" <me@tobin.cc> wrote:

> Current suggestion on list is to remove this function. Do you have a use
> case in mind where debugging will break? We could add a fix to this
> series if so. Otherwise next version will likely drop
> string_is_no_symbol()

What about adding a kernel command line parameter that lets one put
back the old behavior.

"insecure_print_all_symbols" ?

-- Steve

[kernel-hardening] Re: [PATCH 3/3] trace: print address if symbol not found

[Steven Rostedt]

On Tue, 19 Dec 2017 08:16:14 +1100
"Tobin C. Harding" <me@tobin.cc> wrote:

> > >  #endif /* _LINUX_KERNEL_TRACE_H */
> > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> > > index 1e1558c99d56..3e28522a76f4 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_addr(str, stacktrace_entries[i]);  
> > 

> 
> If you have the time to give me some brief pointers on how I should go
> about testing this I'd love to test it before the next version. I know
> very little about ftrace.

For hitting the histogram stacktrace trigger (this code path), make
sure you have CONFIG_HIST_TRIGGERS enabled. And then do:

 # cd /sys/kernel/debug/tracing
 # echo 'hist:keys=common_pid.execname,stacktrace:vals=prev_state' > \
     events/sched/sched_switch/trigger
 # cat events/sched/sched_switch/hist

For the "sym" part, you can do (from the same directory):

 # echo 'hist:keys=call_site.sym:vals=bytes_req' > \
     events/kmem/kmalloc/trigger
 # cat events/kmem/kmalloc/hist


And for sym-offset:

 # echo 'hist:keys=call_site.sym-offset:vals=bytes_req' > \
    events/kmem/kmalloc/trigger
 # cat events/kmem/kmalloc/hist

-- Steve

[kernel-hardening] Re: [PATCH 3/3] trace: print address if symbol not found

[Tobin C. Harding]

On Mon, Dec 18, 2017 at 06:51:43PM -0500, Steven Rostedt wrote:
> On Tue, 19 Dec 2017 08:16:14 +1100
> "Tobin C. Harding" <me@tobin.cc> wrote:
> 
> > > >  #endif /* _LINUX_KERNEL_TRACE_H */
> > > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> > > > index 1e1558c99d56..3e28522a76f4 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_addr(str, stacktrace_entries[i]);  
> > > 
> 
> > 
> > If you have the time to give me some brief pointers on how I should go
> > about testing this I'd love to test it before the next version. I know
> > very little about ftrace.
> 
> For hitting the histogram stacktrace trigger (this code path), make
> sure you have CONFIG_HIST_TRIGGERS enabled. And then do:
> 
>  # cd /sys/kernel/debug/tracing
>  # echo 'hist:keys=common_pid.execname,stacktrace:vals=prev_state' > \
>      events/sched/sched_switch/trigger
>  # cat events/sched/sched_switch/hist
> 
> For the "sym" part, you can do (from the same directory):
> 
>  # echo 'hist:keys=call_site.sym:vals=bytes_req' > \
>      events/kmem/kmalloc/trigger
>  # cat events/kmem/kmalloc/hist
> 
> 
> And for sym-offset:
> 
>  # echo 'hist:keys=call_site.sym-offset:vals=bytes_req' > \
>     events/kmem/kmalloc/trigger
>  # cat events/kmem/kmalloc/hist
> 
> -- Steve

Thanks, you're the man

[kernel-hardening] Re: [PATCH 1/3] kallsyms: don't leak address when symbol not found

[Tobin C. Harding]

On Mon, Dec 18, 2017 at 06:43:24PM -0500, Steven Rostedt wrote:
> On Tue, 19 Dec 2017 09:41:29 +1100
> "Tobin C. Harding" <me@tobin.cc> wrote:
> 
> > Current suggestion on list is to remove this function. Do you have a use
> > case in mind where debugging will break? We could add a fix to this
> > series if so. Otherwise next version will likely drop
> > string_is_no_symbol()
> 
> What about adding a kernel command line parameter that lets one put
> back the old behavior.
> 
> "insecure_print_all_symbols" ?

Cool. I've not done that before it will be a good learning
experience. I'll hack it up and see what people think.

thanks,
Tobin.

[kernel-hardening] Re: [PATCH 3/3] trace: print address if symbol not found

[Tobin C. Harding]

On Mon, Dec 18, 2017 at 06:51:43PM -0500, Steven Rostedt wrote:
> On Tue, 19 Dec 2017 08:16:14 +1100
> "Tobin C. Harding" <me@tobin.cc> wrote:
> 
> > > >  #endif /* _LINUX_KERNEL_TRACE_H */
> > > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> > > > index 1e1558c99d56..3e28522a76f4 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_addr(str, stacktrace_entries[i]);  
> > > 
> 
> > 
> > If you have the time to give me some brief pointers on how I should go
> > about testing this I'd love to test it before the next version. I know
> > very little about ftrace.
> 
> For hitting the histogram stacktrace trigger (this code path), make
> sure you have CONFIG_HIST_TRIGGERS enabled. And then do:
> 
>  # cd /sys/kernel/debug/tracing
>  # echo 'hist:keys=common_pid.execname,stacktrace:vals=prev_state' > \
>      events/sched/sched_switch/trigger
>  # cat events/sched/sched_switch/hist
> 
> For the "sym" part, you can do (from the same directory):
> 
>  # echo 'hist:keys=call_site.sym:vals=bytes_req' > \
>      events/kmem/kmalloc/trigger
>  # cat events/kmem/kmalloc/hist
> 
> 
> And for sym-offset:
> 
>  # echo 'hist:keys=call_site.sym-offset:vals=bytes_req' > \
>     events/kmem/kmalloc/trigger
>  # cat events/kmem/kmalloc/hist

I ran through these as outlined here for the new version (v4). This hits
the modified code but doesn't test symbol look up failure.

I also configured kernel with 'Perform a startup test on ftrace' for
good luck.

Are you happy with this level of testing?

thanks,
Tobin.

[kernel-hardening] Re: [PATCH 3/3] trace: print address if symbol not found

[Tobin C. Harding]

On Tue, Dec 19, 2017 at 02:00:11PM +1100, Tobin C. Harding wrote:
> On Mon, Dec 18, 2017 at 06:51:43PM -0500, Steven Rostedt wrote:
> > On Tue, 19 Dec 2017 08:16:14 +1100
> > "Tobin C. Harding" <me@tobin.cc> wrote:
> > 
> > > > >  #endif /* _LINUX_KERNEL_TRACE_H */
> > > > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> > > > > index 1e1558c99d56..3e28522a76f4 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_addr(str, stacktrace_entries[i]);  
> > > > 
> > 
> > > 
> > > If you have the time to give me some brief pointers on how I should go
> > > about testing this I'd love to test it before the next version. I know
> > > very little about ftrace.
> > 
> > For hitting the histogram stacktrace trigger (this code path), make
> > sure you have CONFIG_HIST_TRIGGERS enabled. And then do:
> > 
> >  # cd /sys/kernel/debug/tracing
> >  # echo 'hist:keys=common_pid.execname,stacktrace:vals=prev_state' > \
> >      events/sched/sched_switch/trigger
> >  # cat events/sched/sched_switch/hist
> > 
> > For the "sym" part, you can do (from the same directory):
> > 
> >  # echo 'hist:keys=call_site.sym:vals=bytes_req' > \
> >      events/kmem/kmalloc/trigger
> >  # cat events/kmem/kmalloc/hist
> > 
> > 
> > And for sym-offset:
> > 
> >  # echo 'hist:keys=call_site.sym-offset:vals=bytes_req' > \
> >     events/kmem/kmalloc/trigger
> >  # cat events/kmem/kmalloc/hist
> 
> I ran through these as outlined here for the new version (v4). This hits

Should have been:

                                                            v2

thanks,
Tobin.

[kernel-hardening] Re: [PATCH 3/3] trace: print address if symbol not found

[Steven Rostedt]

On Tue, 19 Dec 2017 14:00:11 +1100
"Tobin C. Harding" <me@tobin.cc> wrote:

> I ran through these as outlined here for the new version (v4). This hits
> the modified code but doesn't test symbol look up failure.

stacktrace shouldn't post non kernel values, unless there's a frame
pointer that isn't handled by kallsyms.

As for the other two, we could probably force a failure, like:

 # echo 'hist:keys=hrtimer.sym' > \
     events/timer/hrtimer_start/trigger
 # cat events/timer/hrtimer_start/hist

And then just add sym-offset too.

> 
> I also configured kernel with 'Perform a startup test on ftrace' for
> good luck.
> 
> Are you happy with this level of testing?

Can you try the above.

-- Steve

[kernel-hardening] Re: [PATCH 3/3] trace: print address if symbol not found

[Tobin C. Harding]

On Mon, Dec 18, 2017 at 10:37:38PM -0500, Steven Rostedt wrote:
> On Tue, 19 Dec 2017 14:00:11 +1100
> "Tobin C. Harding" <me@tobin.cc> wrote:
> 
> > I ran through these as outlined here for the new version (v4). This hits
> > the modified code but doesn't test symbol look up failure.
> 
> stacktrace shouldn't post non kernel values, unless there's a frame
> pointer that isn't handled by kallsyms.
> 
> As for the other two, we could probably force a failure, like:
> 
>  # echo 'hist:keys=hrtimer.sym' > \
>      events/timer/hrtimer_start/trigger
>  # cat events/timer/hrtimer_start/hist
> 
> And then just add sym-offset too.
>
> > I also configured kernel with 'Perform a startup test on ftrace' for
> > good luck.
> > 
> > Are you happy with this level of testing?
> 
> Can you try the above.

Did both and in both cases we get the addresses as hoped :)

thanks,
Tobin.

[kernel-hardening] Re: [PATCH 3/3] trace: print address if symbol not found

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1801 bytes --]

Hi Tobin,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.15-rc4 next-20171219]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Tobin-C-Harding/kallsyms-don-t-leak-address/20171220-062707
config: i386-randconfig-x016-201751 (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   kernel/trace/trace_events_hist.c: In function 'hist_trigger_stacktrace_print':
>> kernel/trace/trace_events_hist.c:985:3: error: implicit declaration of function 'trace_sprint_symbol_addr'; did you mean 'trace_sprint_symbol'? [-Werror=implicit-function-declaration]
      trace_sprint_symbol_addr(str, stacktrace_entries[i]);
      ^~~~~~~~~~~~~~~~~~~~~~~~
      trace_sprint_symbol
   cc1: some warnings being treated as errors

vim +985 kernel/trace/trace_events_hist.c

   971	
   972	static void hist_trigger_stacktrace_print(struct seq_file *m,
   973						  unsigned long *stacktrace_entries,
   974						  unsigned int max_entries)
   975	{
   976		char str[KSYM_SYMBOL_LEN];
   977		unsigned int spaces = 8;
   978		unsigned int i;
   979	
   980		for (i = 0; i < max_entries; i++) {
   981			if (stacktrace_entries[i] == ULONG_MAX)
   982				return;
   983	
   984			seq_printf(m, "%*c", 1 + spaces, ' ');
 > 985			trace_sprint_symbol_addr(str, stacktrace_entries[i]);
   986			seq_printf(m, "%s\n", str);
   987		}
   988	}
   989	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29958 bytes --]

[kernel-hardening] Re: [PATCH 3/3] trace: print address if symbol not found

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]

Hi Tobin,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.15-rc4 next-20171219]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Tobin-C-Harding/kallsyms-don-t-leak-address/20171220-062707
config: i386-randconfig-s1-201751 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   kernel/trace/trace_events_hist.c: In function 'hist_trigger_stacktrace_print':
>> kernel/trace/trace_events_hist.c:985:3: error: implicit declaration of function 'trace_sprint_symbol_addr' [-Werror=implicit-function-declaration]
      trace_sprint_symbol_addr(str, stacktrace_entries[i]);
      ^~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/trace_sprint_symbol_addr +985 kernel/trace/trace_events_hist.c

   971	
   972	static void hist_trigger_stacktrace_print(struct seq_file *m,
   973						  unsigned long *stacktrace_entries,
   974						  unsigned int max_entries)
   975	{
   976		char str[KSYM_SYMBOL_LEN];
   977		unsigned int spaces = 8;
   978		unsigned int i;
   979	
   980		for (i = 0; i < max_entries; i++) {
   981			if (stacktrace_entries[i] == ULONG_MAX)
   982				return;
   983	
   984			seq_printf(m, "%*c", 1 + spaces, ' ');
 > 985			trace_sprint_symbol_addr(str, stacktrace_entries[i]);
   986			seq_printf(m, "%s\n", str);
   987		}
   988	}
   989	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31282 bytes --]