RFC: introduce "K" flag for printf, similar to %pK

PATCH New Kees Cook 2 participants 6 messages Jan 25, 2011

Subsystems (2)

Library code Vsprintf

lore

Share & tools

lore

Kees Cook

author

2011-01-25 02:04 UTC #0 lore raw

In the interests of hiding kernel addresses from userspace (without messing with file permissions), I want to use %pK for /proc/kallsyms and /proc/modules, but this results in changing several %x's to %p's. The primary side-effects is that some legitimately "0" value things in /proc/kallsyms turn into "(null)".

For example in kernel/kallsyms.c:
- seq_printf(m, "%0*lx %c %s\t[%s]\n",
+ seq_printf(m, "%0*pK %c %s\t[%s]\n",

This results in /proc/kallsyms looking like this:
(null) D irq_stack_union
(null) D __per_cpu_start
0000000000004000 D gdt_page
...

(Secondary effect is building with -Wformat results in harmless warnings "warning: '0' flag used with ‘%p’ gnu_printf format".)

If, on the other hand, I introduce a printf flag "K" for numbers, the original behavior is left, and kernel/kallsyms.c changes like this:
- seq_printf(m, "%0*lx %c %s\t[%s]\n",
+ seq_printf(m, "%K0*lx %c %s\t[%s]\n",

The only side-effect from this is when compiling with -Wformat, now we get these harmless warnings "warning: unknown conversion type character 'K' in format", as well as breaking the warning parser, so it can't count arguments correctly any more "warning: format '%s' expects type 'char *', but argument 4 has type 'long unsigned int'" etc.

I'm not very happy with either situation, but I'll reply to this email with both versions of the potential patch...

-Kees

Signature · 2 lines

Kees Cook
Ubuntu Security Team

Kees Cook

author

2011-01-25 02:05 UTC #1 lore raw

PATCH 1/2 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 will change from 00000000 to "(null)". Additionally, when compiling with -Wformat, these harmless warnings are emitted:
warning: '0' flag used with ‘%p’ gnu_printf format

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

---
 kernel/kallsyms.c |    4 ++--
 kernel/module.c   |    4 ++--
 2 files changed, 4 insertions(+), 4 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)

Signature · 6 lines

1.7.2.3


-- 
Kees Cook
Ubuntu Security Team

Kees Cook

author

2011-01-25 02:08 UTC #2 lore raw

PATCH 2/2 use %pK and %Klx for /proc/kallsyms and /proc/modules

In reply to Kees Cook

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

Note that instead of changing %x to %p, this introduces the "K" flag to printf numbers. Without this, some legitimately 0x0 values in /proc/kallsyms would have changed from 00000000 to "(null)".

However, build-time warnings are emitted when built with -Wformat
warning: unknown conversion type character 'K' in format
even though it produced sensible results.

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

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

kernel/kallsyms.c +2 -2

index 6f6d091..080294d 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, "%K0*lx %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, "%K0*lx %c %s\n",
 			   (int)(2 * sizeof(void *)),
 			   iter->value, iter->type, iter->name);
 	return 0;

kernel/module.c +2 -2

index 34e00b7..0f46775 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%Klx\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 +23 -0

index d3023df..736c174 100644
@@ -382,6 +382,7 @@ char *put_dec(char *buf, unsigned long long num)
 #define LEFT	16		/* left justified */
 #define SMALL	32		/* use lowercase in hex (must be 32 == 0x20) */
 #define SPECIAL	64		/* prefix hex with "0x", octal with "0" */
+#define HIDEVAL	128		/* only CAP_SYSLOG can see real value */
 
 enum format_type {
 	FORMAT_TYPE_NONE, /* Just a string part */

@@ -416,6 +417,9 @@ struct printf_spec {
 };
 
 static noinline_for_stack
+char *string(char *buf, char *end, const char *s, struct printf_spec spec);
+
+static noinline_for_stack
 char *number(char *buf, char *end, unsigned long long num,
 	     struct printf_spec spec)
 {

@@ -428,6 +432,24 @@ char *number(char *buf, char *end, unsigned long long num,
 	int need_pfx = ((spec.flags & SPECIAL) && spec.base != 10);
 	int i;
 
+	if (spec.flags & HIDEVAL) {
+		/*
+		 * HIDEVAL cannot be used in IRQ context because its test
+		 * for CAP_SYSLOG would be meaningless.
+		 */
+		if (in_irq() || in_serving_softirq() || in_nmi()) {
+			if (spec.field_width == -1)
+				spec.field_width = 2 * sizeof(void *);
+			return string(buf, end, "K-error", spec);
+		} else if ((kptr_restrict == 0) ||
+			 (kptr_restrict == 1 &&
+			  has_capability_noaudit(current, CAP_SYSLOG))) {
+			/* do not hide value */
+		} else {
+			num = 0;
+		}
+	}
+
 	/* locase = 0 or 0x20. ORing digits or letters with 'locase'
 	 * produces same digits or (maybe lowercased) letters */
 	locase = (spec.flags & SMALL);

@@ -1138,6 +1160,7 @@ int format_decode(const char *fmt, struct printf_spec *spec)
 		case ' ': spec->flags |= SPACE;   break;
 		case '#': spec->flags |= SPECIAL; break;
 		case '0': spec->flags |= ZEROPAD; break;
+		case 'K': spec->flags |= HIDEVAL; break;
 		default:  found = false;
 		}

Signature · 6 lines

1.7.2.3


-- 
Kees Cook
Ubuntu Security Team

Joe Perches

2011-01-25 02:17 UTC #3 lore raw

In reply to Kees Cook

Quoted 22 lines · Kees Cook

In the interests of hiding kernel addresses from userspace (without messing with file permissions), I want to use %pK for /proc/kallsyms and /proc/modules, but this results in changing several %x's to %p's. The primary side-effects is that some legitimately "0" value things in /proc/kallsyms turn into "(null)".
For example in kernel/kallsyms.c:
(quoted diff) +1 -1
-               seq_printf(m, "%0*lx %c %s\t[%s]\n",
+               seq_printf(m, "%0*pK %c %s\t[%s]\n",
This results in /proc/kallsyms looking like this:
(null) D irq_stack_union
(null) D __per_cpu_start
0000000000004000 D gdt_page
...
(Secondary effect is building with -Wformat results in harmless warnings "warning: '0' flag used with ‘%p’ gnu_printf format".)
If, on the other hand, I introduce a printf flag "K" for numbers, the original behavior is left, and kernel/kallsyms.c changes like this:

Review

Quoted 2 lines

(quoted diff) +1 -1

-               seq_printf(m, "%0*lx %c %s\t[%s]\n",
+               seq_printf(m, "%K0*lx %c %s\t[%s]\n",

Another option would be to allow '0' for
kernel pointers.

Something like (copy/paste, won't apply):

lib/vsprintf.c +1 -1

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

Kees Cook

author

2011-01-25 17:29 UTC #4 lore raw

Hi Joe,

Quoted 9 lines · Joe Perches

Quoted 5 lines · Kees Cook
In the interests of hiding kernel addresses from userspace (without messing with file permissions), I want to use %pK for /proc/kallsyms and /proc/modules, but this results in changing several %x's to %p's. The primary side-effects is that some legitimately "0" value things in /proc/kallsyms turn into "(null)".
Another option would be to allow '0' for
kernel pointers.

But then this changes the behavior of %p where (null) is expected. (i.e. when switching from %p to %pK.)

I'm personally fine with that, as I suspect anything parsing the output that can handle finding "(null)" will be fine with "0" too. But the other way around, not so much. :)

-Kees

Signature · 2 lines

Kees Cook
Ubuntu Security Team

Joe Perches

2011-01-25 17:41 UTC #5 lore raw

Quoted 8 lines · Kees Cook

Quoted 7 lines · Joe Perches
Quoted 5 lines · Kees Cook
In the interests of hiding kernel addresses from userspace (without messing with file permissions), I want to use %pK for /proc/kallsyms and /proc/modules, but this results in changing several %x's to %p's. The primary side-effects is that some legitimately "0" value things in /proc/kallsyms turn into "(null)".

Quoted 5 lines

Quoted 2 lines
Another option would be to allow '0' for
kernel pointers.
But then this changes the behavior of %p where (null) is expected. (i.e. when switching from %p to %pK.)

If you really want no change to any existing cases, change it to "%pk" and a new case label.

Quoted 3 lines

I'm personally fine with that, as I suspect anything parsing the output that can handle finding "(null)" will be fine with "0" too. But the other way around, not so much. :)

Maybe there's a case where somebody changed a kernel pointer from %p to %pK that demands "(null)", but I can't think of one.

cheers, Joe