* [PATCH 1/2] use %pK for /proc/kallsyms and /proc/modules
2011-01-25 2:03 RFC: introduce "K" flag for printf, similar to %pK Kees Cook
@ 2011-01-25 2:04 ` Kees Cook
2011-01-25 2:07 ` [PATCH 2/2] use %pK and %Klx " Kees Cook
2011-01-25 2:17 ` RFC: introduce "K" flag for printf, similar to %pK Joe Perches
2 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2011-01-25 2:04 UTC (permalink / raw)
To: linux-kernel
Cc: Rusty Russell, Tejun Heo, Marcus Meissner, Jason Wessel,
Eugene Teo, Andrew Morton, Joe Perches, Bjorn Helgaas, Len Brown,
Changli Gao, Dan Rosenberg
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] use %pK and %Klx for /proc/kallsyms and /proc/modules
2011-01-25 2:03 RFC: introduce "K" flag for printf, similar to %pK Kees Cook
2011-01-25 2:04 ` [PATCH 1/2] use %pK for /proc/kallsyms and /proc/modules Kees Cook
@ 2011-01-25 2:07 ` Kees Cook
2011-01-25 2:17 ` RFC: introduce "K" flag for printf, similar to %pK Joe Perches
2 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2011-01-25 2:07 UTC (permalink / raw)
To: linux-kernel
Cc: Rusty Russell, Tejun Heo, Marcus Meissner, Jason Wessel,
Eugene Teo, Andrew Morton, Joe Perches, Bjorn Helgaas, Len Brown,
Changli Gao, Dan Rosenberg
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: RFC: introduce "K" flag for printf, similar to %pK
2011-01-25 2:03 RFC: introduce "K" flag for printf, similar to %pK Kees Cook
2011-01-25 2:04 ` [PATCH 1/2] use %pK for /proc/kallsyms and /proc/modules Kees Cook
2011-01-25 2:07 ` [PATCH 2/2] use %pK and %Klx " Kees Cook
@ 2011-01-25 2:17 ` Joe Perches
2011-01-25 17:28 ` Kees Cook
2 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2011-01-25 2:17 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Rusty Russell, Tejun Heo, Marcus Meissner,
Jason Wessel, Eugene Teo, Andrew Morton, Bjorn Helgaas, Len Brown,
Changli Gao, Dan Rosenberg
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: RFC: introduce "K" flag for printf, similar to %pK
2011-01-25 2:17 ` RFC: introduce "K" flag for printf, similar to %pK Joe Perches
@ 2011-01-25 17:28 ` Kees Cook
2011-01-25 17:41 ` Joe Perches
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2011-01-25 17:28 UTC (permalink / raw)
To: Joe Perches
Cc: linux-kernel, Rusty Russell, Tejun Heo, Marcus Meissner,
Jason Wessel, Eugene Teo, Andrew Morton, Bjorn Helgaas, Len Brown,
Changli Gao, Dan Rosenberg
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: RFC: introduce "K" flag for printf, similar to %pK
2011-01-25 17:28 ` Kees Cook
@ 2011-01-25 17:41 ` Joe Perches
0 siblings, 0 replies; 6+ messages in thread
From: Joe Perches @ 2011-01-25 17:41 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Rusty Russell, Tejun Heo, Marcus Meissner,
Jason Wessel, Eugene Teo, Andrew Morton, Bjorn Helgaas, Len Brown,
Changli Gao, Dan Rosenberg
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
^ permalink raw reply [flat|nested] 6+ messages in thread