[RFC 0/3] kallsyms: don't leak address when printing symbol

[RFC 0/3] kallsyms: don't leak address when printing symbol

[Tobin C. Harding]

This is an RFC for two reasons.

1) I don't know who this patch set may break?
2) Patch set includes a function that is not called. Function is there
   to facilitate fixing breakages.

_If_ no one gets broken then we can remove the unused function.

Thanks for looking at this.

Currently if a pointer is printed using %p[ssB] and the symbol is not
found (kallsyms_lookup() fails) then we print the actual address. This
potentially leaks kernel addresses. We could instead print something
_safe_. If kallsyms is made to return an error then callers of
sprint_symbol() can decide what to do.

In the case of vsprintf we can print '<no-symbol>' (patch 2).

In the case of trace we want the address so we can check the return code
and print the address if no symbol is found (patch 3).

Design for this set loosely suggested by Steve Rostedt (so as not to
break ftrace).


Patch 1 and 2 tested, patch 3 (trace stuff) 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

[RFC 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 531ffa984bc2..4bfa4ee3ce93 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

Re: [RFC 1/3] kallsyms: don't leak address when symbol not found

[Tobin C. Harding]

I reordered the To's and CC's, I hope this doesn't break
threading. (clearly I haven't groked email yet :( ) 

On Tue, Nov 28, 2017 at 09:30:17AM +1100, 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>
> ---
>  kernel/kallsyms.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 531ffa984bc2..4bfa4ee3ce93 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
> 

Do you want a Suggested-by: tag for this patch Steve? I mentioned you in
the cover letter but as far as going into the git history I'm not
entirely sure on the protocol for adding suggested-by. The kernel docs
say not to add it without authorization, so ...

thanks,
Tobin.

[RFC 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.

Future users of vsprintf may wish to know, after a call that uses
specifier %p[sSB], whether or not a symbol was found. The actual
sanitized string should be contained (isolated) within the vsprintf.c
therefore we should provide a predicate function. This also allows the
sanitized string to be updated at a later stage with minimal risk to
calling code.

Print '<no-symbol>' for printk specifier %s[sSB] if no symbol is
found. Provide predicate function string_is_no_symbol().

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 1746bae94d41..01e18a8c63fd 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -665,6 +665,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)
@@ -672,6 +674,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')
@@ -680,11 +683,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
@@ -692,6 +698,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

[RFC 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

Re: [RFC 0/3] kallsyms: don't leak address when printing symbol

[Kees Cook]

On Mon, Nov 27, 2017 at 2:30 PM, Tobin C. Harding <me@tobin.cc> wrote:
> This is an RFC for two reasons.
>
> 1) I don't know who this patch set may break?
> 2) Patch set includes a function that is not called. Function is there
>    to facilitate fixing breakages.
>
> _If_ no one gets broken then we can remove the unused function.
>
> Thanks for looking at this.
>
> Currently if a pointer is printed using %p[ssB] and the symbol is not
> found (kallsyms_lookup() fails) then we print the actual address. This
> potentially leaks kernel addresses. We could instead print something
> _safe_. If kallsyms is made to return an error then callers of
> sprint_symbol() can decide what to do.
>
> In the case of vsprintf we can print '<no-symbol>' (patch 2).
>
> In the case of trace we want the address so we can check the return code
> and print the address if no symbol is found (patch 3).
>
> Design for this set loosely suggested by Steve Rostedt (so as not to
> break ftrace).

If tracing is the only place this is needed, this series seems reasonable to me.

-Kees

>
>
> Patch 1 and 2 tested, patch 3 (trace stuff) 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
>



-- 
Kees Cook
Pixel Security

Re: [RFC 0/3] kallsyms: don't leak address when printing symbol

[Tobin C. Harding]

On Mon, Nov 27, 2017 at 04:52:21PM -0800, Kees Cook wrote:
> On Mon, Nov 27, 2017 at 2:30 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > This is an RFC for two reasons.
> >
> > 1) I don't know who this patch set may break?
> > 2) Patch set includes a function that is not called. Function is there
> >    to facilitate fixing breakages.
> >
> > _If_ no one gets broken then we can remove the unused function.
> >
> > Thanks for looking at this.
> >
> > Currently if a pointer is printed using %p[ssB] and the symbol is not
> > found (kallsyms_lookup() fails) then we print the actual address. This
> > potentially leaks kernel addresses. We could instead print something
> > _safe_. If kallsyms is made to return an error then callers of
> > sprint_symbol() can decide what to do.
> >
> > In the case of vsprintf we can print '<no-symbol>' (patch 2).
> >
> > In the case of trace we want the address so we can check the return code
> > and print the address if no symbol is found (patch 3).
> >
> > Design for this set loosely suggested by Steve Rostedt (so as not to
> > break ftrace).
> 
> If tracing is the only place this is needed, this series seems reasonable to me.

Noob question: how do we _know_ this. In other words how do we know no
userland tools rely on the current behaviour? No stress to answer Kees,
this is a pretty general kernel dev question.

thanks,
Tobin.

Re: [kernel-hardening] Re: [RFC 0/3] kallsyms: don't leak address when printing symbol

[Kaiwan N Billimoria]

On Tue, Nov 28, 2017 at 7:20 AM, Tobin C. Harding <me@tobin.cc> wrote:
>
> Noob question: how do we _know_ this. In other words how do we know no
> userland tools rely on the current behaviour? No stress to answer Kees,
> this is a pretty general kernel dev question.

Perhaps I'm reading this wrong, but anyway: besides ftrace, kprobes
will require a
symbol-to-address lookup. Specifically, in the function
kprobe_lookup_name() which
in turn invokes kallsyms_lookup_name().
AFAIK, SystemTap (userland) is built on top of the kprobes infrastructure..

thanks,
Kaiwan.

Re: [kernel-hardening] Re: [RFC 0/3] kallsyms: don't leak address when printing symbol

[Tobin C. Harding]

On Tue, Nov 28, 2017 at 08:58:44AM +0530, Kaiwan N Billimoria wrote:
> On Tue, Nov 28, 2017 at 7:20 AM, Tobin C. Harding <me@tobin.cc> wrote:
> >
> > Noob question: how do we _know_ this. In other words how do we know no
> > userland tools rely on the current behaviour? No stress to answer Kees,
> > this is a pretty general kernel dev question.
> 
> Perhaps I'm reading this wrong, but anyway: besides ftrace, kprobes
> will require a
> symbol-to-address lookup. Specifically, in the function
> kprobe_lookup_name() which
> in turn invokes kallsyms_lookup_name().

We should be right for this call chain because the patch doesn't touch
kallsyms_lookup_name().

> AFAIK, SystemTap (userland) is built on top of the kprobes infrastructure..

This actually indirectly answers the concern. Since no userland tool
should be looking up a kernel address the only code we can break is
kernel code.


thanks,
Tobin