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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

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

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

[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

[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

[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 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 <kees...@canonical.com>

---
kernel/kallsyms.c | 4 ++--
kernel/module.c | 4 ++--

lib/vsprintf.c | 2 +-
3 files changed, 5 insertions(+), 5 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)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c

index d3023df..288d770 100644

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

1.7.2.3

--
Kees Cook
Ubuntu Security Team

[Andrew Morton]

On Tue, 25 Jan 2011 10:10:58 -0800
Kees Cook <kees...@canonical.com> 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!

> 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

OK, so what applications did this patch just break?

[Joe Perches]

On Tue, 2011-01-25 at 10:10 -0800, Kees Cook wrote:
> 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

> diff --git 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);

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.

> } 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]

Hi,

On Wed, Jan 26, 2011 at 03:57:06PM -0800, Andrew Morton wrote:
> On Tue, 25 Jan 2011 10:10:58 -0800
> Kees Cook <kees...@canonical.com> 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.

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

--
Kees Cook
Ubuntu Security Team

[Kees Cook]

On Wed, Jan 26, 2011 at 04:15:09PM -0800, Joe Perches wrote:
> On Tue, 2011-01-25 at 10:10 -0800, Kees Cook wrote:
> > 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
>
> > diff --git 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);
>
> 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.

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.

--
Kees Cook
Ubuntu Security Team

[Kees Cook]

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 <kees...@canonical.com>
---
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(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 6f6d091..ff37fbd 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -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",

[Andrew Morton]

On Wed, 26 Jan 2011 16:29:36 -0800
Kees Cook <kees...@canonical.com> wrote:

> > > 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]

On Wed, Jan 26, 2011 at 04:46:50PM -0800, Andrew Morton wrote:
> On Wed, 26 Jan 2011 16:29:36 -0800
> Kees Cook <kees...@canonical.com> wrote:
>
> > > > 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.

If something can parse "null", "00000001" through "99999999", and _not_
"00000000", I will happily giggle at them. :)

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

--
Kees Cook
Ubuntu Security Team

[Andrew Morton]

On Wed, 26 Jan 2011 16:41:29 -0800
Kees Cook <kees...@canonical.com> 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.)
>

> ...

>
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -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/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?

[Kees Cook]

Hi Andrew,

On Fri, Feb 04, 2011 at 02:03:51PM -0800, Andrew Morton wrote:
> 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

--
Kees Cook
Ubuntu Security Team

[Andrew Morton]

On Fri, 4 Feb 2011 14:09:44 -0800
Kees Cook <kees...@canonical.com> wrote:

> Hi Andrew,
>
> On Fri, Feb 04, 2011 at 02:03:51PM -0800, Andrew Morton wrote:
> > 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).
>

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

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

[Joe Perches]

On Fri, 2011-02-04 at 14:21 -0800, Andrew Morton wrote:
> On Fri, 4 Feb 2011 14:09:44 -0800
> Kees Cook <kees...@canonical.com> wrote:
> > On Fri, Feb 04, 2011 at 02:03:51PM -0800, Andrew Morton wrote:
> > > 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.

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

[Kees Cook]

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.) Due to the %x to %p, "void *" casts are needed since
these addresses are already "unsigned long" everywhere internally, due
to their starting life as ELF section offsets.

Signed-off-by: Kees Cook <kees...@canonical.com>
Cc: Eugene Teo <eug...@redhat.com>
Cc: Dan Rosenberg <drose...@vsecurity.com>

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

v3:
- add "void *" casts to avoid -Wformat warnings.
---
kernel/kallsyms.c | 10 ++++------

kernel/module.c | 4 ++--
lib/vsprintf.c | 2 +-

3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 6f6d091..75dcca3 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -477,13 +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",
- (int)(2 * sizeof(void *)),

- iter->value, type, iter->name, iter->module_name);
+ seq_printf(m, "%pK %c %s\t[%s]\n", (void *)iter->value,
+ type, iter->name, iter->module_name);

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

- iter->value, iter->type, iter->name);
+ seq_printf(m, "%pK %c %s\n", (void *)iter->value,
+ iter->type, iter->name);
return 0;
}

diff --git a/kernel/module.c b/kernel/module.c
index 34e00b7..54e76a7 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", (void *)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..288d770 100644
--- 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.
--
1.7.2.3

--

Kees Cook
Ubuntu Security Team