Skip to contentSkip to conversation

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

PATCH Superseded Kees Cook 3 participants 5 messages Jan 27, 2011 – Feb 4, 2011

Subsystems (2)
Library code Vsprintf

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Kees Cook

author
#0 lore raw

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

In an effort to reduce kernel address leaks that might be used to help target kernel privilege escalation exploits, this patch uses %pK when displaying addresses in /proc/kallsyms, /proc/modules, and /sys/module/*/sections/*.

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 that was already successfully parsing "(null)" in addition to full hex digits should have no problem with this change. (Thanks to Joe Perches for the suggestion.)

Signed-off-by Kees Cook <[email protected]>

---
v2:
- ditch %0* with 2*sizeof(void*) prefixing since %p is already rendered to that width, thanks to Joe Perches.

---
 kernel/kallsyms.c |    6 ++----
 kernel/module.c   |    4 ++--
 lib/vsprintf.c    |    2 +-
 3 files changed, 5 insertions(+), 7 deletions(-)
kernel/kallsyms.c +2 -4
index 6f6d091..ff37fbd 100644
@@ -477,12 +477,10 @@ 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",
-			   (int)(2 * sizeof(void *)),
+		seq_printf(m, "%pK %c %s\t[%s]\n",
 			   iter->value, type, iter->name, iter->module_name);
 	} else
-		seq_printf(m, "%0*lx %c %s\n",
-			   (int)(2 * sizeof(void *)),
+		seq_printf(m, "%pK %c %s\n",
 			   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 · 6 lines
1.7.2.3


-- 
Kees Cook
Ubuntu Security Team
maintainer
#1 lore raw
Quoted 21 lines · On Wed, 26 Jan 2011 16:41:29 -0800 Kees Cook <[email protected]> wrote:

In an effort to reduce kernel address leaks that might be used to help target kernel privilege escalation exploits, this patch uses %pK when displaying addresses in /proc/kallsyms, /proc/modules, and /sys/module/*/sections/*.

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 that was already successfully parsing "(null)" in addition to full hex digits should have no problem with this change. (Thanks to Joe Perches for the suggestion.)

...

kernel/kallsyms.c +0 -1
@@ -477,12 +477,10 @@ 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",

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

Quoted 10 lines
kernel/kallsyms.c +2 -3
-			   (int)(2 * sizeof(void *)),
+		seq_printf(m, "%pK %c %s\t[%s]\n",
 			   iter->value, type, iter->name, iter->module_name);
 	} else
-		seq_printf(m, "%0*lx %c %s\n",
-			   (int)(2 * sizeof(void *)),
+		seq_printf(m, "%pK %c %s\n",
 			   iter->value, iter->type, iter->name);
 	return 0;
 }

kernel/kallsyms.c: In function 's_show':
kernel/kallsyms.c:481: warning: format '%p' expects type 'void *', but argument 3 has type 'long unsigned int' kernel/kallsyms.c:484: warning: format '%p' expects type 'void *', but argument 3 has type 'long unsigned int' kernel/module.c: In function 'module_sect_show':
kernel/module.c:1171: warning: format '%p' expects type 'void *', but argument 3 has type 'long unsigned int' kernel/module.c:1171: warning: format '%p' expects type 'void *', but argument 3 has type 'long unsigned int'

I'm struggling to see how this could have been compile-time or runtime tested?

author
#2 lore raw

Hi Andrew,

Quoted 9 lines · Andrew Morton

kernel/kallsyms.c: In function 's_show':
kernel/kallsyms.c:481: warning: format '%p' expects type 'void *', but argument 3 has type 'long unsigned int' kernel/kallsyms.c:484: warning: format '%p' expects type 'void *', but argument 3 has type 'long unsigned int' kernel/module.c: In function 'module_sect_show':
kernel/module.c:1171: warning: format '%p' expects type 'void *', but argument 3 has type 'long unsigned int' kernel/module.c:1171: warning: format '%p' expects type 'void *', but argument 3 has type 'long unsigned int'

I'm struggling to see how this could have been compile-time or runtime tested?

I run-time tested it plenty. The thread contains the various discussions about compile-time warnings, so I suspect in the last version, I didn't go examine the warnings (since the origin of the other warnings went away).

I can send a patch to fix it up to cast everything to (void*) if you want?

-Kees

Signature · 2 lines
Kees Cook
Ubuntu Security Team
maintainer
#3 lore raw
Quoted 10 lines · On Fri, 4 Feb 2011 14:09:44 -0800 Kees Cook <[email protected]> wrote:

Hi Andrew,

Quoted 7 lines · Andrew Morton

kernel/kallsyms.c: In function 's_show':
kernel/kallsyms.c:481: warning: format '%p' expects type 'void *', but argument 3 has type 'long unsigned int' kernel/kallsyms.c:484: warning: format '%p' expects type 'void *', but argument 3 has type 'long unsigned int' kernel/module.c: In function 'module_sect_show':
kernel/module.c:1171: warning: format '%p' expects type 'void *', but argument 3 has type 'long unsigned int' kernel/module.c:1171: warning: format '%p' expects type 'void *', but argument 3 has type 'long unsigned int'

Quoted 7 lines
Quoted 2 lines

I'm struggling to see how this could have been compile-time or runtime tested?

I run-time tested it plenty. The thread contains the various discussions about compile-time warnings, so I suspect in the last version, I didn't go examine the warnings (since the origin of the other warnings went away).

It's passing `unsigned long kallsym_iter.value' into vsprintf as a pointer. Won't vsprintf end up dereferenceing that unsigned long?

Quoted 1 line

I can send a patch to fix it up to cast everything to (void*) if you want?

No typecasts, please. Get the types *correct* and they won't be needed.

#4 lore raw
Quoted 11 lines · Andrew Morton
Quoted 7 lines · On Fri, 4 Feb 2011 14:09:44 -0800 Kees Cook <[email protected]> wrote:
Quoted 3 lines · Andrew Morton

kernel/kallsyms.c:481: warning: format '%p' expects type 'void *', but argument 3 has type 'long unsigned int' I'm struggling to see how this could have been compile-time or runtime
tested?

I run-time tested it plenty. The thread contains the various discussions about compile-time warnings, so I suspect in the last version, I didn't go examine the warnings (since the origin of the other warnings went away).

It's passing `unsigned long kallsym_iter.value' into vsprintf as a pointer. Won't vsprintf end up dereferenceing that unsigned long?

No it won't. %p and %pK output the pointer value.

Quoted 2 lines
Quoted 1 line

I can send a patch to fix it up to cast everything to (void*) if you want?

No typecasts, please. Get the types *correct* and they won't be needed.

There are many printk casts of longs to void *.

grep -rP --include=*.[ch] "\b(printk|pr_*[a-z]+).*\".*\(void \*\)" *

Typecast is the right way to fix this warning using %pK, though there might be a different/better way altogether.