use %pK for /proc/kallsyms and /proc/modules

PATCH Superseded Kees Cook 3 participants 7 messages Jan 25, 2011 – Jan 27, 2011

Subsystems (2)

Library code Vsprintf

lore

Share & tools

lore

Kees Cook

author

2011-01-25 18:11 UTC #0 lore raw

PATCH 1/1 use %pK for /proc/kallsyms and /proc/modules

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 <[email protected]>

---
 kernel/kallsyms.c |    4 ++--
 kernel/module.c   |    4 ++--
 lib/vsprintf.c    |    2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

kernel/kallsyms.c +2 -2

index 6f6d091..074b762 100644
@@ -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;

kernel/module.c +2 -2

index 34e00b7..748465c 100644
@@ -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)

lib/vsprintf.c +1 -1

index d3023df..288d770 100644
@@ -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.

Signature · 5 lines

1.7.2.3

-- 
Kees Cook
Ubuntu Security Team

Andrew Morton

maintainer

2011-01-26 23:57 UTC #1 lore raw

Quoted 1 line · On Tue, 25 Jan 2011 10:10:58 -0800 Kees Cook <[email protected]> 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!

Quoted 10 lines

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

Quoted 3 lines

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?

Kees Cook

author

2011-01-27 00:29 UTC #2 lore raw

Hi,

Quoted 7 lines · Andrew Morton

Quoted 1 line · On Tue, 25 Jan 2011 10:10:58 -0800 Kees Cook <[email protected]> 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.

Quoted 7 lines

Quoted 5 lines
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

Signature · 2 lines

Kees Cook
Ubuntu Security Team

Andrew Morton

maintainer

2011-01-27 00:47 UTC #3 lore raw

Quoted 9 lines · On Wed, 26 Jan 2011 16:29:36 -0800 Kees Cook <[email protected]> wrote:

Quoted 7 lines
Quoted 5 lines
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"?

Kees Cook

author

2011-01-27 01:30 UTC #4 lore raw

Quoted 9 lines · Andrew Morton

Quoted 6 lines · On Wed, 26 Jan 2011 16:29:36 -0800 Kees Cook <[email protected]> wrote:
Quoted 6 lines
Quoted 5 lines
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.)

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
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. :)

Quoted 2 lines

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

Signature · 2 lines

Kees Cook
Ubuntu Security Team

Joe Perches

2011-01-27 00:15 UTC #5 lore raw

In reply to Kees Cook

kernel/kallsyms.c

Quoted 5 lines · Kees Cook

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
kernel/kallsyms.c +0 -0

[]

kernel/kallsyms.c static int s_show(struct seq_file *m, void *p) · line 482

Quoted 8 lines

kernel/kallsyms.c +1 -1

@@ -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.

kernel/kallsyms.c static int s_show(struct seq_file *m, void *p) · line 485

Quoted 4 lines

kernel/kallsyms.c +1 -1

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

Here too.

Kees Cook

author

2011-01-27 00:29 UTC #6 lore raw

Quoted 18 lines · Joe Perches

kernel/kallsyms.c
Quoted 5 lines · Kees Cook
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
kernel/kallsyms.c +0 -0
[]
kernel/kallsyms.c static int s_show(struct seq_file *m, void *p) · line 482
Quoted 8 lines
kernel/kallsyms.c +1 -1
@@ -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

Quoted 6 lines

		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.

Signature · 2 lines

Kees Cook
Ubuntu Security Team