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

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

[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:
-               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

-- 
Kees Cook
Ubuntu Security Team

[PATCH 1/2] 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 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 <kees.cook@canonical.com>
---
 kernel/kallsyms.c |    4 ++--
 kernel/module.c   |    4 ++--
 2 files changed, 4 insertions(+), 4 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)
-- 
1.7.2.3


-- 
Kees Cook
Ubuntu Security Team

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

[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 <kees.cook@canonical.com>
---
 kernel/kallsyms.c |    4 ++--
 kernel/module.c   |    4 ++--
 lib/vsprintf.c    |   23 +++++++++++++++++++++++
 3 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 6f6d091..080294d 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, "%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;
diff --git a/kernel/module.c b/kernel/module.c
index 34e00b7..0f46775 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%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)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d3023df..736c174 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -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;
 		}
 
-- 
1.7.2.3


-- 
Kees Cook
Ubuntu Security Team

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

[Joe Perches]

On Mon, 2011-01-24 at 18:03 -0800, Kees Cook wrote:
> 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",

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

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

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
--- 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.

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

[Kees Cook]

Hi Joe,

On Mon, Jan 24, 2011 at 06:17:04PM -0800, Joe Perches wrote:
> On Mon, 2011-01-24 at 18:03 -0800, Kees Cook wrote:
> > 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

-- 
Kees Cook
Ubuntu Security Team

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

[Joe Perches]

On Tue, 2011-01-25 at 09:28 -0800, Kees Cook wrote:
> On Mon, Jan 24, 2011 at 06:17:04PM -0800, Joe Perches wrote:
> > On Mon, 2011-01-24 at 18:03 -0800, Kees Cook wrote:
> > > 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.)

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

> 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