[PATCH] kallsyms: don't leak address when printing symbol

[PATCH] kallsyms: don't leak address when printing symbol

[Tobin C. Harding]

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
leaks kernel addresses. We should instead print something _safe_.

Print "<no-symbol>" instead of kernel address.

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

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 127e7cfafa55..182e7592be9c 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -390,7 +390,7 @@ 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);
+		return sprintf(buffer, "<no-symbol>");
 
 	if (name != buffer)
 		strcpy(buffer, name);
-- 
2.7.4

Re: [PATCH] kallsyms: don't leak address when printing symbol

[Steven Rostedt]

On Thu,  9 Nov 2017 12:50:29 +1100
"Tobin C. Harding" <me@tobin.cc> wrote:

> 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
> leaks kernel addresses. We should instead print something _safe_.
> 
> Print "<no-symbol>" instead of kernel address.

Ug, ftrace requires this to work as is, as it uses it to print some
addresses that may or may not be a symbol.

If anything, can this return a success or failure if it were to find a
symbol or not, and then something like ftrace could decide to use %x if
it does not.

And yes, ftrace leaks kernel addresses all over the place, that's just
the nature of tracing the kernel.

-- Steve


> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> ---
>  kernel/kallsyms.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 127e7cfafa55..182e7592be9c 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -390,7 +390,7 @@ 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);
> +		return sprintf(buffer, "<no-symbol>");
>  
>  	if (name != buffer)
>  		strcpy(buffer, name);

Re: [PATCH] kallsyms: don't leak address when printing symbol

[Sergey Senozhatsky]

On (11/08/17 22:35), Steven Rostedt wrote:
> On Thu,  9 Nov 2017 12:50:29 +1100
> "Tobin C. Harding" <me@tobin.cc> wrote:
> 
> > 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
> > leaks kernel addresses. We should instead print something _safe_.
> > 
> > Print "<no-symbol>" instead of kernel address.
> 
> Ug, ftrace requires this to work as is, as it uses it to print some
> addresses that may or may not be a symbol.
> 
> If anything, can this return a success or failure if it were to find a
> symbol or not, and then something like ftrace could decide to use %x if
> it does not.
> 
> And yes, ftrace leaks kernel addresses all over the place, that's just
> the nature of tracing the kernel.
> 

agree, I kinda suspect that that "0x%lx" sometimes can be useful.
at least one can tell if the frame is from modules or kernel, and
objdump it may be.

	-ss

Re: [PATCH] kallsyms: don't leak address when printing symbol

[Tobin C. Harding]

On Wed, Nov 08, 2017 at 10:35:55PM -0500, Steven Rostedt wrote:
> On Thu,  9 Nov 2017 12:50:29 +1100
> "Tobin C. Harding" <me@tobin.cc> wrote:
> 
> > 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
> > leaks kernel addresses. We should instead print something _safe_.
> > 
> > Print "<no-symbol>" instead of kernel address.
> 
> Ug, ftrace requires this to work as is, as it uses it to print some
> addresses that may or may not be a symbol.
> 
> If anything, can this return a success or failure if it were to find a
> symbol or not, and then something like ftrace could decide to use %x if
> it does not.

Thanks for the feed back Steve. Propagating the error back up through
the call chain may require a little bit of thought so we don't upset the
apple cart. sprint_symbol() never currently (as far as I can see)
returns an error. I can go through the other callers of sprint_symbol()
(there aren't many) and check if it is going to upset anything.

> And yes, ftrace leaks kernel addresses all over the place, that's just
> the nature of tracing the kernel.

Would it be good for you (for this change and future changes aimed at
closing leaks) if any changes like this include patches to ftrace to
maintain the current behaviour?

You have been on the CC list for the printk hashing and what not since
the start I believe so you know I'm a noob, feel free to scream bloody
murder if I'm breaching protocol.

thanks,
Tobin.

Re: [PATCH] kallsyms: don't leak address when printing symbol

[Steven Rostedt]

On Thu, 9 Nov 2017 16:45:48 +1100
"Tobin C. Harding" <me@tobin.cc> wrote:

> On Wed, Nov 08, 2017 at 10:35:55PM -0500, Steven Rostedt wrote:
> > On Thu,  9 Nov 2017 12:50:29 +1100
> > "Tobin C. Harding" <me@tobin.cc> wrote:
> >   
> > > 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
> > > leaks kernel addresses. We should instead print something _safe_.
> > > 
> > > Print "<no-symbol>" instead of kernel address.  
> > 
> > Ug, ftrace requires this to work as is, as it uses it to print some
> > addresses that may or may not be a symbol.
> > 
> > If anything, can this return a success or failure if it were to find a
> > symbol or not, and then something like ftrace could decide to use %x if
> > it does not.  
> 
> Thanks for the feed back Steve. Propagating the error back up through
> the call chain may require a little bit of thought so we don't upset the
> apple cart. sprint_symbol() never currently (as far as I can see)
> returns an error. I can go through the other callers of sprint_symbol()
> (there aren't many) and check if it is going to upset anything.

As the functions are currently void, adding a return value if it
doesn't print the symbol shouldn't break anything, unless the other
users required an address as well. Which stack traces might!

> 
> > And yes, ftrace leaks kernel addresses all over the place, that's just
> > the nature of tracing the kernel.  
> 
> Would it be good for you (for this change and future changes aimed at
> closing leaks) if any changes like this include patches to ftrace to
> maintain the current behaviour?
> 
> You have been on the CC list for the printk hashing and what not since
> the start I believe so you know I'm a noob, feel free to scream bloody
> murder if I'm breaching protocol.

No, you have been fine. I've been quickly scanning your patches looking
for things like this. Please keep Ccing me. I'm currently busy fighting
other fires, but I try to at least take a quick look at patches like
this. I may not reply, but you are on my radar ;-)

-- Steve