[PATCH] use %pK for /proc/kallsyms and /proc/modules

[PATCH] use %pK for /proc/kallsyms and /proc/modules

[Kees Cook]

Instead of messing with permissions on these files, use %pK for kernel
addresses to reduce potential information leaks that might be used to
help target kernel privilege escalation exploits.

Note that this changes %x to %p, so some legitimately 0 values in
/proc/kallsyms would have changed from 00000000 to "(null)". To avoid
this, "(null)" is not used when using the "K" format. Anything parsing
such addresses should have no problem with this change. (Thanks to Joe
Perches for the suggestion.)

Note that when compiling with -Wformat, these harmless warnings will
be emitted, and can be ignored:
  warning: '0' flag used with ‘%p’ gnu_printf format

Signed-off-by: Kees Cook <kees.cook@canonical.com>
---
 kernel/kallsyms.c |    4 ++--
 kernel/module.c   |    4 ++--
 lib/vsprintf.c    |    2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 6f6d091..074b762 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -477,11 +477,11 @@ static int s_show(struct seq_file *m, void *p)
 		 */
 		type = iter->exported ? toupper(iter->type) :
 					tolower(iter->type);
-		seq_printf(m, "%0*lx %c %s\t[%s]\n",
+		seq_printf(m, "%0*pK %c %s\t[%s]\n",
 			   (int)(2 * sizeof(void *)),
 			   iter->value, type, iter->name, iter->module_name);
 	} else
-		seq_printf(m, "%0*lx %c %s\n",
+		seq_printf(m, "%0*pK %c %s\n",
 			   (int)(2 * sizeof(void *)),
 			   iter->value, iter->type, iter->name);
 	return 0;
diff --git a/kernel/module.c b/kernel/module.c
index 34e00b7..748465c 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1168,7 +1168,7 @@ static ssize_t module_sect_show(struct module_attribute *mattr,
 {
 	struct module_sect_attr *sattr =
 		container_of(mattr, struct module_sect_attr, mattr);
-	return sprintf(buf, "0x%lx\n", sattr->address);
+	return sprintf(buf, "0x%pK\n", sattr->address);
 }
 
 static void free_sect_attrs(struct module_sect_attrs *sect_attrs)
@@ -3224,7 +3224,7 @@ static int m_show(struct seq_file *m, void *p)
 		   mod->state == MODULE_STATE_COMING ? "Loading":
 		   "Live");
 	/* Used by oprofile and other similar tools. */
-	seq_printf(m, " 0x%p", mod->module_core);
+	seq_printf(m, " 0x%pK", mod->module_core);
 
 	/* Taints info */
 	if (mod->taints)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d3023df..288d770 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -991,7 +991,7 @@ static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	      struct printf_spec spec)
 {
-	if (!ptr) {
+	if (!ptr && *fmt != 'K') {
 		/*
 		 * Print (null) with the same width as a pointer so it makes
 		 * tabular output look nice.
-- 
1.7.2.3

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH] use %pK for /proc/kallsyms and /proc/modules

[Andrew Morton]

On Tue, 25 Jan 2011 10:10:58 -0800
Kees Cook <kees.cook@canonical.com> wrote:

> Instead of messing with permissions on these files,

This implies that the patch alters permission handling, only it
doesn't.  But I worked it out!

> use %pK for kernel
> addresses to reduce potential information leaks that might be used to
> help target kernel privilege escalation exploits.
> 
> Note that this changes %x to %p, so some legitimately 0 values in
> /proc/kallsyms would have changed from 00000000 to "(null)". To avoid
> this, "(null)" is not used when using the "K" format. Anything parsing
> such addresses should have no problem with this change. (Thanks to Joe
> Perches for the suggestion.)
> 
> Note that when compiling with -Wformat, these harmless warnings will
> be emitted, and can be ignored:
>   warning: '0' flag used with ___%p___ gnu_printf format

OK, so what applications did this patch just break?

Re: [PATCH] use %pK for /proc/kallsyms and /proc/modules

[Joe Perches]

On Tue, 2011-01-25 at 10:10 -0800, Kees Cook wrote:
> Note that when compiling with -Wformat, these harmless warnings will
> be emitted, and can be ignored:
>   warning: '0' flag used with ‘%p’ gnu_printf format

> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
[]
> @@ -477,11 +477,11 @@ static int s_show(struct seq_file *m, void *p)
>  		 */
>  		type = iter->exported ? toupper(iter->type) :
>  					tolower(iter->type);
> -		seq_printf(m, "%0*lx %c %s\t[%s]\n",
> +		seq_printf(m, "%0*pK %c %s\t[%s]\n",
>  			   (int)(2 * sizeof(void *)),
>  			   iter->value, type, iter->name, iter->module_name);

You can change this to

		seq_printf(m, "%pK %c %s\t[%s]\n",
  			   iter->value, type, iter->name, iter->module_name);

as that's the normal size.

Presto.  No warnings.  Same output.

>  	} else
> -		seq_printf(m, "%0*lx %c %s\n",
> +		seq_printf(m, "%0*pK %c %s\n",
>  			   (int)(2 * sizeof(void *)),

Here too.

Re: [PATCH] use %pK for /proc/kallsyms and /proc/modules

[Kees Cook]

On Wed, Jan 26, 2011 at 04:15:09PM -0800, Joe Perches wrote:
> On Tue, 2011-01-25 at 10:10 -0800, Kees Cook wrote:
> > Note that when compiling with -Wformat, these harmless warnings will
> > be emitted, and can be ignored:
> >   warning: '0' flag used with ‘%p’ gnu_printf format
> 
> > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> []
> > @@ -477,11 +477,11 @@ static int s_show(struct seq_file *m, void *p)
> >  		 */
> >  		type = iter->exported ? toupper(iter->type) :
> >  					tolower(iter->type);
> > -		seq_printf(m, "%0*lx %c %s\t[%s]\n",
> > +		seq_printf(m, "%0*pK %c %s\t[%s]\n",
> >  			   (int)(2 * sizeof(void *)),
> >  			   iter->value, type, iter->name, iter->module_name);
> 
> You can change this to
> 
> 		seq_printf(m, "%pK %c %s\t[%s]\n",
>   			   iter->value, type, iter->name, iter->module_name);
> 
> as that's the normal size.
> 
> Presto.  No warnings.  Same output.

Ah-ha! I was comparing against POSIX %p, which doesn't zeropad. The
kernel's %p does, so that's perfect! Yay, no warnings. Thanks! I'll send an
updated patch.

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH] use %pK for /proc/kallsyms and /proc/modules

[Kees Cook]

Hi,

On Wed, Jan 26, 2011 at 03:57:06PM -0800, Andrew Morton wrote:
> On Tue, 25 Jan 2011 10:10:58 -0800
> Kees Cook <kees.cook@canonical.com> wrote:
> 
> > Instead of messing with permissions on these files,
> 
> This implies that the patch alters permission handling, only it
> doesn't.  But I worked it out!

Ah yeah, this was related to the earlier attempts to remove the contents
of, or set permissions on, /proc/kallsyms.

> > Note that this changes %x to %p, so some legitimately 0 values in
> > /proc/kallsyms would have changed from 00000000 to "(null)". To avoid
> > this, "(null)" is not used when using the "K" format. Anything parsing
> > such addresses should have no problem with this change. (Thanks to Joe
> > Perches for the suggestion.)
> 
> OK, so what applications did this patch just break?

I'm not aware of any breakage as a result of this yet.

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH] use %pK for /proc/kallsyms and /proc/modules

[Andrew Morton]

On Wed, 26 Jan 2011 16:29:36 -0800
Kees Cook <kees.cook@canonical.com> wrote:

> > > Note that this changes %x to %p, so some legitimately 0 values in
> > > /proc/kallsyms would have changed from 00000000 to "(null)". To avoid
> > > this, "(null)" is not used when using the "K" format. Anything parsing
> > > such addresses should have no problem with this change. (Thanks to Joe
> > > Perches for the suggestion.)
> > 
> > OK, so what applications did this patch just break?
> 
> I'm not aware of any breakage as a result of this yet.

There will be some - there always are :( But users will only see
problems if they've set kptr_restrict.

Which they shall do.  How come we defaulted kptr_restrict to "true"?

Re: [PATCH] use %pK for /proc/kallsyms and /proc/modules

[Kees Cook]

On Wed, Jan 26, 2011 at 04:46:50PM -0800, Andrew Morton wrote:
> On Wed, 26 Jan 2011 16:29:36 -0800
> Kees Cook <kees.cook@canonical.com> wrote:
> 
> > > > Note that this changes %x to %p, so some legitimately 0 values in
> > > > /proc/kallsyms would have changed from 00000000 to "(null)". To avoid
> > > > this, "(null)" is not used when using the "K" format. Anything parsing
> > > > such addresses should have no problem with this change. (Thanks to Joe
> > > > Perches for the suggestion.)
> > > 
> > > OK, so what applications did this patch just break?
> > 
> > I'm not aware of any breakage as a result of this yet.
> 
> There will be some - there always are :( But users will only see
> problems if they've set kptr_restrict.

If something can parse "null", "00000001" through "99999999", and _not_
"00000000", I will happily giggle at them. :)

> 
> Which they shall do.  How come we defaulted kptr_restrict to "true"?

Because that's the correct value! :) Unprivileged userspace doesn't need to
see kernel addresses by default, that's for CAP_SYSLOG.

-Kees

-- 
Kees Cook
Ubuntu Security Team