[PATCH V9] printk: hash addresses printed with %p

[PATCH V9] printk: hash addresses printed with %p

[Tobin C. Harding]

Currently there are many places in the kernel where addresses are being
printed using an unadorned %p. Kernel pointers should be printed using
%pK allowing some control via the kptr_restrict sysctl. Exposing addresses
gives attackers sensitive information about the kernel layout in memory.

We can reduce the attack surface by hashing all addresses printed with
%p. This will of course break some users, forcing code printing needed
addresses to be updated.

For what it's worth, usage of unadorned %p can be broken down as
follows (thanks to Joe Perches).

$ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
   1084 arch
     20 block
     10 crypto
     32 Documentation
   8121 drivers
   1221 fs
    143 include
    101 kernel
     69 lib
    100 mm
   1510 net
     40 samples
      7 scripts
     11 security
    166 sound
    152 tools
      2 virt

Add function ptr_to_id() to map an address to a 32 bit unique
identifier. Hash any unadorned usage of specifier %p and any malformed
specifiers. 

Signed-off-by: Tobin C. Harding <me@tobin.cc>

---

It seems we don't have consensus on a couple of things

1. The size of the hashed address on 64 bit architectures.
2. The use of '0x' pre-fix for hashed addresses.

In regards to (1), we are agreed that we only need 32 bits of
information. There is some questions however that outputting _only_ 32
bits may break userland.

In regards to (2), irrespective of the arguments for and against, if
point 1 is correct and changing the format will break userland then we
can't add the '0x' suffix for the same reason.

Therefore this patch masks off the first 32 bits, retaining
only 32 bits of information. We do not add a '0x' suffix. All in all,
that results in _no_ change to the format of output only the content of
the output.

The leading 0's also make explicit that we have messed with the address,
maybe this will save some debugging time by doing so. Although this
would probably already be obvious since there is no leading 'ffff'.

We hash malformed specifiers also. Malformed specifiers include
incomplete (e.g %pi) and also non-existent specifiers. checkpatch should
warn for non-existent specifiers but AFAICT won't warn for incomplete
specifiers.

Here is the behaviour that this patch implements.

For kpt_restrict==0

Randomness not ready:
  printed with %p: 		(pointer value)     # NOTE: with padding
Valid pointer:
  printed with %pK: 		deadbeefdeadbeef
  printed with %p: 		00000000deadbeef
  malformed specifier (eg %i):  00000000deadbeef
NULL pointer:
  printed with %pK: 		0000000000000000
  printed with %p: 		         (null)     # NOTE: with padding
  malformed specifier (eg %i):           (null)

For kpt_restrict==2

Valid pointer:
  printed with %pK: 		0000000000000000

All other output as for kptr_restrict==0

V9:
 - Drop the initial patch from V8, leaving null pointer handling as is.
 - Print the hashed ID _without_ a '0x' suffix.
 - Mask the first 32 bits of the hashed ID to all zeros on 64 bit
   architectures.

V8:
 - Add second patch cleaning up null pointer printing in pointer()
 - Move %pK handling to separate function, further cleaning up pointer()
 - Move ptr_to_id() call outside of switch statement making hashing
   the default behaviour (including malformed specifiers).
 - Remove use of static_key, replace with simple boolean.

V7:
 - Use tabs instead of spaces (ouch!).

V6:
 - Use __early_initcall() to fill the SipHash key.
 - Use static keys to guard hashing before the key is available.

V5:
 - Remove spin lock.
 - Add Jason A. Donenfeld to CC list by request.
 - Add Theodore Ts'o to CC list due to comment on previous version.

V4:
 - Remove changes to siphash.{ch}
 - Do word size check, and return value cast, directly in ptr_to_id().
 - Use add_ready_random_callback() to guard call to get_random_bytes()

V3:
 - Use atomic_xchg() to guard setting [random] key.
 - Remove erroneous white space change.

V2:
 - Use SipHash to do the hashing.

The discussion related to this patch has been fragmented. There are
three threads associated with this patch. Email threads by subject:

[PATCH] printk: hash addresses printed with %p
[PATCH 0/3] add %pX specifier
[kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options

 lib/vsprintf.c | 167 ++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 119 insertions(+), 48 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86c3385b9eb3..0c9a008fc256 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -33,6 +33,8 @@
 #include <linux/uuid.h>
 #include <linux/of.h>
 #include <net/addrconf.h>
+#include <linux/siphash.h>
+#include <linux/compiler.h>
 #ifdef CONFIG_BLOCK
 #include <linux/blkdev.h>
 #endif
@@ -1344,6 +1346,57 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 }
 
 static noinline_for_stack
+char *kernel_pointer(char *buf, char *end, const void *ptr,
+		     struct printf_spec spec)
+{
+	spec.base = 16;
+	spec.flags |= SMALL;
+	if (spec.field_width == -1) {
+		spec.field_width = 2 * sizeof(void *);
+		spec.flags |= ZEROPAD;
+	}
+
+	switch (kptr_restrict) {
+	case 0:
+		/* Always print %pK values */
+		break;
+	case 1: {
+		const struct cred *cred;
+
+		/*
+		 * kptr_restrict==1 cannot be used in IRQ context
+		 * because its test for CAP_SYSLOG would be meaningless.
+		 */
+		if (in_irq() || in_serving_softirq() || in_nmi())
+			return string(buf, end, "pK-error", spec);
+
+		/*
+		 * Only print the real pointer value if the current
+		 * process has CAP_SYSLOG and is running with the
+		 * same credentials it started with. This is because
+		 * access to files is checked at open() time, but %pK
+		 * checks permission at read() time. We don't want to
+		 * leak pointer values if a binary opens a file using
+		 * %pK and then elevates privileges before reading it.
+		 */
+		cred = current_cred();
+		if (!has_capability_noaudit(current, CAP_SYSLOG) ||
+		    !uid_eq(cred->euid, cred->uid) ||
+		    !gid_eq(cred->egid, cred->gid))
+			ptr = NULL;
+		break;
+	}
+	case 2:
+	default:
+		/* Always print 0's for %pK */
+		ptr = NULL;
+		break;
+	}
+
+	return number(buf, end, (unsigned long)ptr, spec);
+}
+
+static noinline_for_stack
 char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt)
 {
 	unsigned long long num;
@@ -1591,6 +1644,66 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
+static bool have_filled_random_ptr_key __read_mostly;
+static siphash_key_t ptr_key __read_mostly;
+
+static void fill_random_ptr_key(struct random_ready_callback *unused)
+{
+	get_random_bytes(&ptr_key, sizeof(ptr_key));
+	WRITE_ONCE(have_filled_random_ptr_key, true);
+}
+
+static struct random_ready_callback random_ready = {
+	.func = fill_random_ptr_key
+};
+
+static int __init initialize_ptr_random(void)
+{
+	int ret = add_random_ready_callback(&random_ready);
+
+	if (!ret)
+		return 0;
+	else if (ret == -EALREADY) {
+		fill_random_ptr_key(&random_ready);
+		return 0;
+	}
+
+	return ret;
+}
+early_initcall(initialize_ptr_random);
+
+/* Maps a pointer to a 32 bit unique identifier. */
+static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
+{
+	unsigned long hashval;
+	const int default_width = 2 * sizeof(void *);
+
+	if (unlikely(!have_filled_random_ptr_key)) {
+		spec.field_width = default_width;
+		return string(buf, end, "(pointer value)", spec);
+	}
+
+#ifdef CONFIG_64BIT
+	hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
+	/*
+	 * Mask off the first 32 bits, this makes explicit that we have
+	 * modified the address (and 32 bits is plenty for a unique ID).
+	 */
+	hashval = hashval & 0xffffffff;
+#else
+	hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
+#endif
+
+	spec.flags |= SMALL;
+	if (spec.field_width == -1) {
+		spec.field_width = default_width;
+		spec.flags |= ZEROPAD;
+	}
+	spec.base = 16;
+
+	return number(buf, end, hashval, spec);
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1703,6 +1816,9 @@ int kptr_restrict __read_mostly;
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a
  * pointer to the real address.
+ *
+ * Note: The default behaviour (unadorned %p) is to hash the address,
+ * rendering it useful as a unique identifier.
  */
 static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
@@ -1792,47 +1908,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			return buf;
 		}
 	case 'K':
-		switch (kptr_restrict) {
-		case 0:
-			/* Always print %pK values */
-			break;
-		case 1: {
-			const struct cred *cred;
-
-			/*
-			 * kptr_restrict==1 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 = default_width;
-				return string(buf, end, "pK-error", spec);
-			}
-
-			/*
-			 * Only print the real pointer value if the current
-			 * process has CAP_SYSLOG and is running with the
-			 * same credentials it started with. This is because
-			 * access to files is checked at open() time, but %pK
-			 * checks permission at read() time. We don't want to
-			 * leak pointer values if a binary opens a file using
-			 * %pK and then elevates privileges before reading it.
-			 */
-			cred = current_cred();
-			if (!has_capability_noaudit(current, CAP_SYSLOG) ||
-			    !uid_eq(cred->euid, cred->uid) ||
-			    !gid_eq(cred->egid, cred->gid))
-				ptr = NULL;
-			break;
-		}
-		case 2:
-		default:
-			/* Always print 0's for %pK */
-			ptr = NULL;
-			break;
-		}
-		break;
-
+		return kernel_pointer(buf, end, ptr, spec);
 	case 'N':
 		return netdev_bits(buf, end, ptr, fmt);
 	case 'a':
@@ -1858,14 +1934,9 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			return device_node_string(buf, end, ptr, spec, fmt + 1);
 		}
 	}
-	spec.flags |= SMALL;
-	if (spec.field_width == -1) {
-		spec.field_width = default_width;
-		spec.flags |= ZEROPAD;
-	}
-	spec.base = 16;
 
-	return number(buf, end, (unsigned long) ptr, spec);
+	/* default is to _not_ leak addresses, hash before printing */
+	return ptr_to_id(buf, end, ptr, spec);
 }
 
 /*
-- 
2.7.4

Re: [PATCH V9] printk: hash addresses printed with %p

[Kees Cook]

On Sun, Oct 29, 2017 at 3:59 PM, Tobin C. Harding <me@tobin.cc> wrote:
> Currently there are many places in the kernel where addresses are being
> printed using an unadorned %p. Kernel pointers should be printed using
> %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> gives attackers sensitive information about the kernel layout in memory.
>
> We can reduce the attack surface by hashing all addresses printed with
> %p. This will of course break some users, forcing code printing needed
> addresses to be updated.
>
> For what it's worth, usage of unadorned %p can be broken down as
> follows (thanks to Joe Perches).
>
> $ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
>    1084 arch
>      20 block
>      10 crypto
>      32 Documentation
>    8121 drivers
>    1221 fs
>     143 include
>     101 kernel
>      69 lib
>     100 mm
>    1510 net
>      40 samples
>       7 scripts
>      11 security
>     166 sound
>     152 tools
>       2 virt
>
> Add function ptr_to_id() to map an address to a 32 bit unique
> identifier. Hash any unadorned usage of specifier %p and any malformed
> specifiers.
>
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
>
> ---
>
> It seems we don't have consensus on a couple of things
>
> 1. The size of the hashed address on 64 bit architectures.
> 2. The use of '0x' pre-fix for hashed addresses.
>
> In regards to (1), we are agreed that we only need 32 bits of
> information. There is some questions however that outputting _only_ 32
> bits may break userland.
>
> In regards to (2), irrespective of the arguments for and against, if
> point 1 is correct and changing the format will break userland then we
> can't add the '0x' suffix for the same reason.
>
> Therefore this patch masks off the first 32 bits, retaining
> only 32 bits of information. We do not add a '0x' suffix. All in all,
> that results in _no_ change to the format of output only the content of
> the output.
>
> The leading 0's also make explicit that we have messed with the address,
> maybe this will save some debugging time by doing so. Although this
> would probably already be obvious since there is no leading 'ffff'.
>
> We hash malformed specifiers also. Malformed specifiers include
> incomplete (e.g %pi) and also non-existent specifiers. checkpatch should
> warn for non-existent specifiers but AFAICT won't warn for incomplete
> specifiers.
>
> Here is the behaviour that this patch implements.
>
> For kpt_restrict==0
>
> Randomness not ready:
>   printed with %p:              (pointer value)     # NOTE: with padding
> Valid pointer:
>   printed with %pK:             deadbeefdeadbeef
>   printed with %p:              00000000deadbeef
>   malformed specifier (eg %i):  00000000deadbeef
> NULL pointer:
>   printed with %pK:             0000000000000000
>   printed with %p:                       (null)     # NOTE: with padding
>   malformed specifier (eg %i):           (null)
>
> For kpt_restrict==2
>
> Valid pointer:
>   printed with %pK:             0000000000000000
>
> All other output as for kptr_restrict==0
>
> V9:
>  - Drop the initial patch from V8, leaving null pointer handling as is.
>  - Print the hashed ID _without_ a '0x' suffix.
>  - Mask the first 32 bits of the hashed ID to all zeros on 64 bit
>    architectures.

Oops, I had missed v9. This addresses my concerns. I think the leading
zeros are a good way to identify the "this is clearly not a kernel
address" issue (though the 32-bit folks may remain confused, but we
can fix that later, IMO).

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH V9] printk: hash addresses printed with %p

[Tobin C. Harding]

On Mon, Oct 30, 2017 at 03:31:41PM -0700, Kees Cook wrote:
> On Sun, Oct 29, 2017 at 3:59 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > Currently there are many places in the kernel where addresses are being
> > printed using an unadorned %p. Kernel pointers should be printed using
> > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> > gives attackers sensitive information about the kernel layout in memory.
> >
> > We can reduce the attack surface by hashing all addresses printed with
> > %p. This will of course break some users, forcing code printing needed
> > addresses to be updated.
> >
> > For what it's worth, usage of unadorned %p can be broken down as
> > follows (thanks to Joe Perches).
> >
> > $ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
> >    1084 arch
> >      20 block
> >      10 crypto
> >      32 Documentation
> >    8121 drivers
> >    1221 fs
> >     143 include
> >     101 kernel
> >      69 lib
> >     100 mm
> >    1510 net
> >      40 samples
> >       7 scripts
> >      11 security
> >     166 sound
> >     152 tools
> >       2 virt
> >
> > Add function ptr_to_id() to map an address to a 32 bit unique
> > identifier. Hash any unadorned usage of specifier %p and any malformed
> > specifiers.
> >
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> >
> > ---
> >
> > It seems we don't have consensus on a couple of things
> >
> > 1. The size of the hashed address on 64 bit architectures.
> > 2. The use of '0x' pre-fix for hashed addresses.
> >
> > In regards to (1), we are agreed that we only need 32 bits of
> > information. There is some questions however that outputting _only_ 32
> > bits may break userland.
> >
> > In regards to (2), irrespective of the arguments for and against, if
> > point 1 is correct and changing the format will break userland then we
> > can't add the '0x' suffix for the same reason.
> >
> > Therefore this patch masks off the first 32 bits, retaining
> > only 32 bits of information. We do not add a '0x' suffix. All in all,
> > that results in _no_ change to the format of output only the content of
> > the output.
> >
> > The leading 0's also make explicit that we have messed with the address,
> > maybe this will save some debugging time by doing so. Although this
> > would probably already be obvious since there is no leading 'ffff'.
> >
> > We hash malformed specifiers also. Malformed specifiers include
> > incomplete (e.g %pi) and also non-existent specifiers. checkpatch should
> > warn for non-existent specifiers but AFAICT won't warn for incomplete
> > specifiers.
> >
> > Here is the behaviour that this patch implements.
> >
> > For kpt_restrict==0
> >
> > Randomness not ready:
> >   printed with %p:              (pointer value)     # NOTE: with padding
> > Valid pointer:
> >   printed with %pK:             deadbeefdeadbeef
> >   printed with %p:              00000000deadbeef
> >   malformed specifier (eg %i):  00000000deadbeef
> > NULL pointer:
> >   printed with %pK:             0000000000000000
> >   printed with %p:                       (null)     # NOTE: with padding
> >   malformed specifier (eg %i):           (null)
> >
> > For kpt_restrict==2
> >
> > Valid pointer:
> >   printed with %pK:             0000000000000000
> >
> > All other output as for kptr_restrict==0
> >
> > V9:
> >  - Drop the initial patch from V8, leaving null pointer handling as is.
> >  - Print the hashed ID _without_ a '0x' suffix.
> >  - Mask the first 32 bits of the hashed ID to all zeros on 64 bit
> >    architectures.
> 
> Oops, I had missed v9. This addresses my concerns. I think the leading
> zeros are a good way to identify the "this is clearly not a kernel
> address" issue (though the 32-bit folks may remain confused, but we
> can fix that later, IMO).

Awesome. Yeah this patch (coupled with the leaking_addresses.pl script)
is turning out to be a bit 64-bit centric. However, as we plug more
leaks in 64-bit kernels hopefully they will be plugged in 32-bit ones
too.

I can't think of any way to have leaking_addresses.pl grep for 32-bit
addresses, especially once/if this patch gets merged. We will not be
able to differentiate between hashed addresses and real addresses on
32-bit machines.

thanks,
Tobin.

Re: [PATCH V9] printk: hash addresses printed with %p

[Petr Mladek]

On Mon 2017-10-30 09:59:16, Tobin C. Harding wrote:
> Currently there are many places in the kernel where addresses are being
> printed using an unadorned %p. Kernel pointers should be printed using
> %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> gives attackers sensitive information about the kernel layout in memory.
> 
> We can reduce the attack surface by hashing all addresses printed with
> %p. This will of course break some users, forcing code printing needed
> addresses to be updated.

I am sorry for my ignorance but what is the right update, please?
I expect that there are several possibilities:

  + remove the pointer at all

  + replace it with %pK so that it honors kptr_restrict setting

  + any other option?

Is kptr_restrict considered a safe mechanism?

Also kptr_restrict seems to be primary for the messages that are available
via /proc and /sys. Is it good enough for the messages logged by
printk()?

Will there be a debug option that would allow to see the original
pointers? Or what is the preferred way for debug messages?


> For what it's worth, usage of unadorned %p can be broken down as
> follows (thanks to Joe Perches).
> 
> $ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
>    1084 arch
>      20 block
>      10 crypto
>      32 Documentation
>    8121 drivers
>    1221 fs
>     143 include
>     101 kernel
>      69 lib
>     100 mm
>    1510 net
>      40 samples
>       7 scripts
>      11 security
>     166 sound
>     152 tools
>       2 virt

It is evident that it will hit many people. I guess that they will
be suprised and might have similar questions. It might make sense
to decribe this in Documentation/printk-formats.txt.

Best Regards,
Petr

Re: [PATCH V9] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 31, 2017 at 04:39:44PM +0100, Petr Mladek wrote:
> On Mon 2017-10-30 09:59:16, Tobin C. Harding wrote:
> > Currently there are many places in the kernel where addresses are being
> > printed using an unadorned %p. Kernel pointers should be printed using
> > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> > gives attackers sensitive information about the kernel layout in memory.
> > 
> > We can reduce the attack surface by hashing all addresses printed with
> > %p. This will of course break some users, forcing code printing needed
> > addresses to be updated.
> 
> I am sorry for my ignorance but what is the right update, please?

Can I say first that I am in no way an expert, I am new to both this
problem and kernel dev in general.

> I expect that there are several possibilities:
> 
>   + remove the pointer at all

This definitely stops the leak!

>   + replace it with %pK so that it honors kptr_restrict setting

I think this is the option of choice, see concerns below however. I get
the feeling that the hope with this patch is that a vast majority of
users of %p won't care so stopping all those addresses is the real win
for this patch. 

The next hoped benefit is that the hashing will shed light on this topic
and get developers to think about the issue before _wildly_ printing
addresses. Having to work harder to print the address in future will aid
this (assuming everyone doesn't just start using %x).

>   + any other option?

Use %x or %X - really bad, this will create more pain in the future.

> Is kptr_restrict considered a safe mechanism?
> 
> Also kptr_restrict seems to be primary for the messages that are available
> via /proc and /sys. Is it good enough for the messages logged by
> printk()?

There is some concern that kptr_restrict is not overly great. Linus is
the main purveyor of this argument. I won't paraphrase here because I
will not do the argument justice.

See this thread for the whole discussion

[kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options

Side note: If this (mentioning a previous thread) is bad form, or there
is a better way to do it, could you please tell me.

> Will there be a debug option that would allow to see the original
> pointers? Or what is the preferred way for debug messages?

It seems to me that there is a fundamental conflict between security and
debugging here. To debug we need the addresses, for a secure kernel we
need there to be _no_ addresses shown to user space.

I don't think we have the final solution to this at the moment.

> > For what it's worth, usage of unadorned %p can be broken down as
> > follows (thanks to Joe Perches).
> > 
> > $ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
> >    1084 arch
> >      20 block
> >      10 crypto
> >      32 Documentation
> >    8121 drivers
> >    1221 fs
> >     143 include
> >     101 kernel
> >      69 lib
> >     100 mm
> >    1510 net
> >      40 samples
> >       7 scripts
> >      11 security
> >     166 sound
> >     152 tools
> >       2 virt
> 
> It is evident that it will hit many people. I guess that they will
> be suprised and might have similar questions. It might make sense
> to decribe this in Documentation/printk-formats.txt.

Very good idea, V10 to include.

thanks,
Tobin.

Re: [PATCH V9] printk: hash addresses printed with %p

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2787 bytes --]

Hi Tobin,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.14-rc7 next-20171018]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Tobin-C-Harding/printk-hash-addresses-printed-with-p/20171101-080718
config: i386-tinyconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   lib/vsprintf.c: In function 'kernel_pointer':
>> lib/vsprintf.c:1359:10: error: 'kptr_restrict' undeclared (first use in this function)
     switch (kptr_restrict) {
             ^~~~~~~~~~~~~
   lib/vsprintf.c:1359:10: note: each undeclared identifier is reported only once for each function it appears in

vim +/kptr_restrict +1359 lib/vsprintf.c

  1347	
  1348	static noinline_for_stack
  1349	char *kernel_pointer(char *buf, char *end, const void *ptr,
  1350			     struct printf_spec spec)
  1351	{
  1352		spec.base = 16;
  1353		spec.flags |= SMALL;
  1354		if (spec.field_width == -1) {
  1355			spec.field_width = 2 * sizeof(void *);
  1356			spec.flags |= ZEROPAD;
  1357		}
  1358	
> 1359		switch (kptr_restrict) {
  1360		case 0:
  1361			/* Always print %pK values */
  1362			break;
  1363		case 1: {
  1364			const struct cred *cred;
  1365	
  1366			/*
  1367			 * kptr_restrict==1 cannot be used in IRQ context
  1368			 * because its test for CAP_SYSLOG would be meaningless.
  1369			 */
  1370			if (in_irq() || in_serving_softirq() || in_nmi())
  1371				return string(buf, end, "pK-error", spec);
  1372	
  1373			/*
  1374			 * Only print the real pointer value if the current
  1375			 * process has CAP_SYSLOG and is running with the
  1376			 * same credentials it started with. This is because
  1377			 * access to files is checked at open() time, but %pK
  1378			 * checks permission at read() time. We don't want to
  1379			 * leak pointer values if a binary opens a file using
  1380			 * %pK and then elevates privileges before reading it.
  1381			 */
  1382			cred = current_cred();
  1383			if (!has_capability_noaudit(current, CAP_SYSLOG) ||
  1384			    !uid_eq(cred->euid, cred->uid) ||
  1385			    !gid_eq(cred->egid, cred->gid))
  1386				ptr = NULL;
  1387			break;
  1388		}
  1389		case 2:
  1390		default:
  1391			/* Always print 0's for %pK */
  1392			ptr = NULL;
  1393			break;
  1394		}
  1395	
  1396		return number(buf, end, (unsigned long)ptr, spec);
  1397	}
  1398	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6685 bytes --]

Re: [PATCH V9] printk: hash addresses printed with %p

[Petr Mladek]

On Wed 2017-11-01 10:53:45, Tobin C. Harding wrote:
> On Tue, Oct 31, 2017 at 04:39:44PM +0100, Petr Mladek wrote:
> > On Mon 2017-10-30 09:59:16, Tobin C. Harding wrote:
> > > Currently there are many places in the kernel where addresses are being
> > > printed using an unadorned %p. Kernel pointers should be printed using
> > > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> > > gives attackers sensitive information about the kernel layout in memory.
> > > 
> > > We can reduce the attack surface by hashing all addresses printed with
> > > %p. This will of course break some users, forcing code printing needed
> > > addresses to be updated.
> > 
> > I am sorry for my ignorance but what is the right update, please?
> 
> Can I say first that I am in no way an expert, I am new to both this
> problem and kernel dev in general.

Sure. There are many experienced people in CC. I hope that they might
have some opinion on this.

My concern is that this patch breaks some functionality because it
is dangerous. This makes perfect sense. But people will want to fix
it a safe way. And the way is not clear to me.


> > I expect that there are several possibilities:
> > 
> >   + remove the pointer at all
> 
> This definitely stops the leak!

Sure. But this is not a solution for debugging tools, e.g. lockdep.
sysrq-related dumps. Of course, the pointers must not be visible
on production system to normal users but there should be a way
to see them for debugging purposes.

> >   + replace it with %pK so that it honors kptr_restrict setting
> 
> I think this is the option of choice, see concerns below however. I get
> the feeling that the hope with this patch is that a vast majority of
> users of %p won't care so stopping all those addresses is the real win
> for this patch.
> 
> The next hoped benefit is that the hashing will shed light on this topic
> and get developers to think about the issue before _wildly_ printing
> addresses. Having to work harder to print the address in future will aid
> this (assuming everyone doesn't just start using %x).



> >   + any other option?
> 
> Use %x or %X - really bad, this will create more pain in the future.

Yes, using %x or %X is dangerous. It would be used by users
that do not mind about security. Or is there any situation when
always using the original pointer as a string is needed
for a real functionality?

IMHO, string is needed only for human readable usage. Then it always
smells with information leak.


> > Is kptr_restrict considered a safe mechanism?
> > 
> > Also kptr_restrict seems to be primary for the messages that are available
> > via /proc and /sys. Is it good enough for the messages logged by
> > printk()?
> 
> There is some concern that kptr_restrict is not overly great. Linus is
> the main purveyor of this argument. I won't paraphrase here because I
> will not do the argument justice.
> 
> See this thread for the whole discussion
> 
> [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options

I saw the discussion but it was a bit hard to follow. I would
highlight the following three concerns related to kptr_restrict:

First, it did not help to improve the situation. IMHO, this is mainly
because it was opt-in and did not force people to fix the code. This
is being addressed by this patch that actively breaks all users.

Second, the user credentials are checked when the string is formatted.
They do not reflect the path how the string is passed to the user.
Therefore it might be cheated. This opens a question if kptr_restrict
would stay in kernel and whether the conversion from %p to %pK
is one of the proposed solutions.

Third, kptr_restrict can be modified too late. It means that
the pointers printed during the early boot will never or always
be readable. It means that you would need two different kernel
builds for production and debugging. Well, this is probably
the smallest issue.


All in all. I wonder if it would make sense to introduce
some compiler or command line option that would allow
to disable the hashing of pointers for debugging
purposes.

Best Regards,
Petr