[PATCH 0/5] add printk specifier %px, unique identifier

[PATCH 0/5] add printk specifier %px, unique identifier

[Tobin C. Harding]

Linus,

I know you are bored of this patch set already and this pits your vast
experience against my eight months kernel dev experience ;)

I humbly maintain that hashing %p and suggesting people use %x
_correctly_ isn't a WIN solution.

Please don't go easy on me because I'm new, if I'm out of line - say
so.

This set is based on the following assumptions.

1. We now have leaking_addresses.pl illuminating leaking addresses.
2. We have no _clear_ strategy for fixing leaks once found.
3. We do not have a proposed non opt-in solution.
4. There is a distinct use case for this specifier.

Patch 1: Corrects the docs for %pK.

Patch 2: Refactors %pK code out of pointer() into helper function.

Patch 3: Adds specifier %px, small 'x' was chosen because the hashed hex
         value is printed in lower case.

Patch 4/5: Provides example usage of new specifier.

The hashing code is based on the work done hashing %p during 4.14 dev
cycle.

Finally, with this patch set in place, we have the added benefit that
newbies (me) can quietly go around the kernel 'sweeping up' after
leaking addresses. This as apposed to using a hammer and hashing all
%p. And if this is deemed too little and too slow we can always search
and replace '%p' with '%px'.

thanks,
Tobin.

Tobin C. Harding (5):
  docs: correct documentation for %pK
  vsprintf: refactor pK code out of pointer()
  vsprintf: add specifier %px, unique identifier
  KVM: use %px to print token identifier
  vfio_pci: use %px to print token identifier

 Documentation/printk-formats.txt  |   2 +-
 drivers/vfio/pci/vfio_pci_intrs.c |   2 +-
 lib/test_printf.c                 |  74 +++++++++++++++++
 lib/vsprintf.c                    | 166 ++++++++++++++++++++++++++++----------
 scripts/checkpatch.pl             |   2 +-
 virt/kvm/eventfd.c                |   2 +-
 6 files changed, 202 insertions(+), 46 deletions(-)

-- 
2.7.4

[PATCH 1/5] docs: correct documentation for %pK

[Tobin C. Harding]

Current documentation indicates that %pK prints a leading '0x'. This is
not the case.

Correct documentation for printk specifier %pK.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 Documentation/printk-formats.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 361789df51ec..b4fe3c5f3b44 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -91,7 +91,7 @@ Kernel Pointers
 
 ::
 
-	%pK	0x01234567 or 0x0123456789abcdef
+	%pK	01234567 or 0123456789abcdef
 
 For printing kernel pointers which should be hidden from unprivileged
 users. The behaviour of ``%pK`` depends on the ``kptr_restrict sysctl`` - see
-- 
2.7.4

[PATCH 2/5] vsprintf: refactor pK code out of pointer()

[Tobin C. Harding]

Currently code to handle %pK is all within the switch statement in
pointer(). This is the wrong level of abstraction. Each of the other switch
clauses call a helper function, pK should do the same.

Refactor code out of pointer() to new function kernel_pointer().

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 lib/vsprintf.c | 97 ++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 54 insertions(+), 43 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 1746bae94d41..360731f81dd3 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1343,6 +1343,59 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 	return string(buf, end, uuid, spec);
 }
 
+int kptr_restrict __read_mostly;
+
+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(ptr);
+		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)
 {
@@ -1591,8 +1644,6 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
-int kptr_restrict __read_mostly;
-
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
@@ -1792,47 +1843,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':
-- 
2.7.4

[PATCH 3/5] vsprintf: add specifier %px, unique identifier

[Tobin C. Harding]

The typical in tree method for obtaining a unique identifier when
printing kernel objects (structs) is to use the address of the struct
i.e the pointer. This potentially leaks sensitive information to user
space because it reveals information about the kernel layout in
memory. We can get a unique identifier based on an address by hashing
the address first before printing.

Add printk specifier %px which hashes the address before printing.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 lib/test_printf.c     | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/vsprintf.c        | 71 ++++++++++++++++++++++++++++++++++++++++++++++++
 scripts/checkpatch.pl |  2 +-
 3 files changed, 146 insertions(+), 1 deletion(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 563f10e6876a..8da57205f1ea 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -462,10 +462,84 @@ flags(void)
 	kfree(cmp_buffer);
 }
 
+#define HASHED_BUF_SIZE 64	/* leave some space so we don't oops */
+
+#if BITS_PER_LONG == 64
+
+#define PTR ((void *)0xffff0123456789ab)
+#define PTR_STR "ffff0123456789ab"
+#define ZEROS "00000000"	/* hex 32 zero bits */
+
+static int __init
+hashed_format(void)
+{
+	char buf[HASHED_BUF_SIZE];
+	int nchars;
+
+	nchars = snprintf(buf, HASHED_BUF_SIZE, "%px", PTR);
+
+	if (nchars != PTR_WIDTH || strncmp(buf, ZEROS, strlen(ZEROS)) != 0)
+		return -1;
+
+	return 0;
+}
+
+#else
+
+#define PTR ((void *)0x456789ab)
+#define PTR_STR "456789ab"
+
+static int __init
+hashed_format(void)
+{
+	/* Format is implicitly tested for 32 bit machines by hashed_hash() */
+	return 0;
+}
+
+#endif	/* BITS_PER_LONG == 64 */
+
+static int __init
+hashed_hash(void)
+{
+	char buf[HASHED_BUF_SIZE];
+	int nchars;
+
+	nchars = snprintf(buf, HASHED_BUF_SIZE, "%px", PTR);
+
+	if (nchars != PTR_WIDTH || strncmp(buf, PTR_STR, PTR_WIDTH) == 0)
+		return -1;
+
+	return 0;
+}
+
+/*
+ * We can't use test() to test %px because we don't know what output to expect
+ * after an address is hashed.
+ */
+static void __init
+hashed(void)
+{
+	int err;
+
+	err = hashed_hash();
+	if (err) {
+		pr_warn("specifier px does not appear to be hashed\n");
+		failed_tests++;
+		return;
+	}
+
+	err = hashed_format();
+	if (err) {
+		pr_warn("hashed output from px has unexpected format\n");
+		failed_tests++;
+	}
+}
+
 static void __init
 test_pointer(void)
 {
 	plain();
+	hashed();
 	symbol_ptr();
 	kernel_ptr();
 	struct_resource();
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 360731f81dd3..3b0de95c3498 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
@@ -1644,6 +1646,73 @@ 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));
+	/*
+	 * have_filled_random_ptr_key==true is dependent on get_random_bytes().
+	 * ptr_to_id() needs to see have_filled_random_ptr_key==true
+	 * after get_random_bytes() returns.
+	 */
+	smp_mb();
+	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(ptr);
+
+	if (unlikely(!have_filled_random_ptr_key)) {
+		spec.field_width = default_width;
+		/* string length must be less than default_width */
+		return string(buf, end, "(ptrval)", 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);
+}
+
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
@@ -1868,6 +1937,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		case 'F':
 			return device_node_string(buf, end, ptr, spec, fmt + 1);
 		}
+	case 'x':
+		return ptr_to_id(buf, end, ptr, spec);
 	}
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 95cda3ecc66b..040aa79e1d9d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5753,7 +5753,7 @@ sub process {
 		        for (my $count = $linenr; $count <= $lc; $count++) {
 				my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0));
 				$fmt =~ s/%%//g;
-				if ($fmt =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGNO]).)/) {
+				if ($fmt =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGNOx]).)/) {
 					$bad_extension = $1;
 					last;
 				}
-- 
2.7.4

[PATCH 4/5] KVM: use %px to print token identifier

[Tobin C. Harding]

Currently token address is printed using %p. This exposes the address of
the token in dmesg and potentially leaks sensitive information to
userspace. In this instance the address is being used as a unique
identifier for the token, we can use the newly defined printk specifier
%px for exactly this purpose.

Use the new %px specifier to print a unique identifier for the token.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 virt/kvm/eventfd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index f2ac53ab8243..9c700fdf571c 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -416,7 +416,7 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
 		irqfd->consumer.start = kvm_arch_irq_bypass_start;
 		ret = irq_bypass_register_consumer(&irqfd->consumer);
 		if (ret)
-			pr_info("irq bypass consumer (token %p) registration fails: %d\n",
+			pr_info("irq bypass consumer (token %px) registration fails: %d\n",
 				irqfd->consumer.token, ret);
 	}
 #endif
-- 
2.7.4

[PATCH 5/5] vfio_pci: use %px to print token identifier

[Tobin C. Harding]

Currently token address is printed using %p. This exposes the address of
the token in dmesg and potentially leaks sensitive information to
userspace. In this instance the address is being used as a unique
identifier for the token, we can use the newly defined printk specifier
%px for exactly this purpose.

Use the new %px specifier to print a unique identifier for the token.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 drivers/vfio/pci/vfio_pci_intrs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 1c46045b0e7f..73d3ae8b5784 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -346,7 +346,7 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
 	ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
 	if (unlikely(ret))
 		dev_info(&pdev->dev,
-		"irq bypass producer (token %p) registration fails: %d\n",
+		"irq bypass producer (token %px) registration fails: %d\n",
 		vdev->ctx[vector].producer.token, ret);
 
 	vdev->ctx[vector].trigger = trigger;
-- 
2.7.4

Re: [PATCH 0/5] add printk specifier %px, unique identifier

[Linus Torvalds]

On Mon, Nov 27, 2017 at 3:40 PM, Tobin C. Harding <me@tobin.cc> wrote:
> Finally, with this patch set in place, we have the added benefit that
> newbies (me) can quietly go around the kernel 'sweeping up' after
> leaking addresses. This as apposed to using a hammer and hashing all
> %p. And if this is deemed too little and too slow we can always search
> and replace '%p' with '%px'.

So the big remaining ones for me are the /proc/<pid>/stack (stack
pointers) and the /proc/net/* ones.

I'm a bit disappointed that those haven't been fixed already and
aren't even in this series..

Since I was the proponent of the whole "leaking_addresses" script
model, I guess I can't complain when %p isn't then just made to hash
everything, but it does feel like this has been dragging out a bit..

            Linus

Re: [PATCH 1/5] docs: correct documentation for %pK

[Kees Cook]

On Mon, Nov 27, 2017 at 3:40 PM, Tobin C. Harding <me@tobin.cc> wrote:
> Current documentation indicates that %pK prints a leading '0x'. This is
> not the case.
>
> Correct documentation for printk specifier %pK.

Yup, quite true. :)

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

>
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> ---
>  Documentation/printk-formats.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index 361789df51ec..b4fe3c5f3b44 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -91,7 +91,7 @@ Kernel Pointers
>
>  ::
>
> -       %pK     0x01234567 or 0x0123456789abcdef
> +       %pK     01234567 or 0123456789abcdef
>
>  For printing kernel pointers which should be hidden from unprivileged
>  users. The behaviour of ``%pK`` depends on the ``kptr_restrict sysctl`` - see
> --
> 2.7.4
>



-- 
Kees Cook
Pixel Security

Re: [PATCH 0/5] add printk specifier %px, unique identifier

[Kees Cook]

On Mon, Nov 27, 2017 at 3:40 PM, Tobin C. Harding <me@tobin.cc> wrote:
> Linus,
>
> I know you are bored of this patch set already and this pits your vast
> experience against my eight months kernel dev experience ;)
>
> I humbly maintain that hashing %p and suggesting people use %x
> _correctly_ isn't a WIN solution.
>
> Please don't go easy on me because I'm new, if I'm out of line - say
> so.
>
> This set is based on the following assumptions.
>
> 1. We now have leaking_addresses.pl illuminating leaking addresses.
> 2. We have no _clear_ strategy for fixing leaks once found.
> 3. We do not have a proposed non opt-in solution.
> 4. There is a distinct use case for this specifier.
>
> Patch 1: Corrects the docs for %pK.
>
> Patch 2: Refactors %pK code out of pointer() into helper function.
>
> Patch 3: Adds specifier %px, small 'x' was chosen because the hashed hex
>          value is printed in lower case.
>
> Patch 4/5: Provides example usage of new specifier.
>
> The hashing code is based on the work done hashing %p during 4.14 dev
> cycle.
>
> Finally, with this patch set in place, we have the added benefit that
> newbies (me) can quietly go around the kernel 'sweeping up' after
> leaking addresses. This as apposed to using a hammer and hashing all
> %p. And if this is deemed too little and too slow we can always search
> and replace '%p' with '%px'.

How does this opt-in to %px help? We'll still have %p everywhere. :(
Why not invert this? %p is hashed and %px is the old %p? Then we can
move %x users to %px.

I'd still like to see a default-on solution for this class of leaks...

-Kees

>
> thanks,
> Tobin.
>
> Tobin C. Harding (5):
>   docs: correct documentation for %pK
>   vsprintf: refactor pK code out of pointer()
>   vsprintf: add specifier %px, unique identifier
>   KVM: use %px to print token identifier
>   vfio_pci: use %px to print token identifier
>
>  Documentation/printk-formats.txt  |   2 +-
>  drivers/vfio/pci/vfio_pci_intrs.c |   2 +-
>  lib/test_printf.c                 |  74 +++++++++++++++++
>  lib/vsprintf.c                    | 166 ++++++++++++++++++++++++++++----------
>  scripts/checkpatch.pl             |   2 +-
>  virt/kvm/eventfd.c                |   2 +-
>  6 files changed, 202 insertions(+), 46 deletions(-)
>
> --
> 2.7.4
>



-- 
Kees Cook
Pixel Security

Re: [PATCH 0/5] add printk specifier %px, unique identifier

[Linus Torvalds]

On Mon, Nov 27, 2017 at 4:03 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So the big remaining ones for me are the /proc/<pid>/stack (stack
> pointers) and the /proc/net/* ones.
>
> I'm a bit disappointed that those haven't been fixed already and
> aren't even in this series..

Oh well, I just did /proc/<pid>/stack by making it just print 0
unconditionally rather than the hex number.

Looking around, not even proc-ps actually uses that file, and it's
conditional on PROC_STACKTRACE anyway. And can't recall ever having
seen a report of something breaking due to CONFIG_STACKTRACE not being
enabled, so I suspect nothing really uses /proc/<pid>/stack at all.

But rather than removing it, making it report 0 seemed the smaller change.

I'd be inclined to do the same for /proc/*/net/* too, but I would
actually expect that there are tools that cross-reference the sockets
by socket address (ie "fuser" and similar tools). So I'd like to have
that hashing for that.

Although maybe I'm wrong, and they simply use the socket number, and
the socket address could just be scrubbed entirely.

                  Linus

Re: [PATCH 0/5] add printk specifier %px, unique identifier

[Tobin C. Harding]

On Mon, Nov 27, 2017 at 04:57:18PM -0800, Kees Cook wrote:
> On Mon, Nov 27, 2017 at 3:40 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > Linus,
> >
> > I know you are bored of this patch set already and this pits your vast
> > experience against my eight months kernel dev experience ;)
> >
> > I humbly maintain that hashing %p and suggesting people use %x
> > _correctly_ isn't a WIN solution.
> >
> > Please don't go easy on me because I'm new, if I'm out of line - say
> > so.
> >
> > This set is based on the following assumptions.
> >
> > 1. We now have leaking_addresses.pl illuminating leaking addresses.
> > 2. We have no _clear_ strategy for fixing leaks once found.
> > 3. We do not have a proposed non opt-in solution.
> > 4. There is a distinct use case for this specifier.
> >
> > Patch 1: Corrects the docs for %pK.
> >
> > Patch 2: Refactors %pK code out of pointer() into helper function.
> >
> > Patch 3: Adds specifier %px, small 'x' was chosen because the hashed hex
> >          value is printed in lower case.
> >
> > Patch 4/5: Provides example usage of new specifier.
> >
> > The hashing code is based on the work done hashing %p during 4.14 dev
> > cycle.
> >
> > Finally, with this patch set in place, we have the added benefit that
> > newbies (me) can quietly go around the kernel 'sweeping up' after
> > leaking addresses. This as apposed to using a hammer and hashing all
> > %p. And if this is deemed too little and too slow we can always search
> > and replace '%p' with '%px'.
> 
> How does this opt-in to %px help? We'll still have %p everywhere. :(
> Why not invert this? %p is hashed and %px is the old %p? Then we can
> move %x users to %px.

This is a really nice twist, I don't know why it hasn't come up
before. For the record it

- Plugs a bunch of potential current leaks.
- Is on by default (*not* opt-in).
- Is easy to use (%p if you don't care, %px if you _really_ want the address).
- Reduces risk of future developers creating grep hell by using %x

(- makes Linus happy because it does everything he has suggested except
promote use of %x)

> I'd still like to see a default-on solution for this class of leaks...

I'll re-spin this tomorrow and see if we can't stop bothering everyone
with it :)

thanks,
Tobin.

Re: [PATCH 0/5] add printk specifier %px, unique identifier

[Eric W. Biederman]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Mon, Nov 27, 2017 at 4:03 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> So the big remaining ones for me are the /proc/<pid>/stack (stack
>> pointers) and the /proc/net/* ones.
>>
>> I'm a bit disappointed that those haven't been fixed already and
>> aren't even in this series..
>
> Oh well, I just did /proc/<pid>/stack by making it just print 0
> unconditionally rather than the hex number.

Patch?

I know I have used /proc/<pid>/stack manually many times when looking
at a system where something is hung/weird and I needed to see what is
going on.  The backtrace inside the kernel can be invaluable.

At the same time I don't know if we actually need the hex address.
But please don't break that interface it is very useful.


Eric

RE: [PATCH 0/5] add printk specifier %px, unique identifier

[David Laight]

From: Eric W. Biederman
> Sent: 28 November 2017 06:27
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > On Mon, Nov 27, 2017 at 4:03 PM, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> >>
> >> So the big remaining ones for me are the /proc/<pid>/stack (stack
> >> pointers) and the /proc/net/* ones.
> >>
> >> I'm a bit disappointed that those haven't been fixed already and
> >> aren't even in this series..
> >
> > Oh well, I just did /proc/<pid>/stack by making it just print 0
> > unconditionally rather than the hex number.
> 
> Patch?
> 
> I know I have used /proc/<pid>/stack manually many times when looking
> at a system where something is hung/weird and I needed to see what is
> going on.  The backtrace inside the kernel can be invaluable.

Ditto - after I spotted it.
Also the similar tracebacks from echo t >/proc/sysrq-trigger
although they are less useful unless you've a big kernel message buffer.
Although they can be requested from a keyboard if everything except the
keyboard interrupt is borked.

> At the same time I don't know if we actually need the hex address.
> But please don't break that interface it is very useful.

Definitely need to know which addresses are zero (or near zero).
I will have tied the addresses there to ones available elsewhere.
(In private trace that won't be affected by whatever kernel printf
does with %p.)

If you want to hide addresses, then maybe use a write-only sysctl.

	David

Re: [PATCH 0/5] add printk specifier %px, unique identifier

[Linus Torvalds]

On Mon, Nov 27, 2017 at 10:26 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>>
>> Oh well, I just did /proc/<pid>/stack by making it just print 0
>> unconditionally rather than the hex number.
>
> Patch?

Oh, apparently I never pushed out yesterday.

The patch literally just affects the (useless) hex number. So:

    cat /proc/self/stack

now prints out

    [<0>] proc_pid_stack+0xaa/0x100
    [<0>] proc_single_show+0x48/0x80
    [<0>] seq_read+0xd2/0x410
    ...

instead of putting some randomized kernel address there.

I considered getting rid of the whole "[<>]" thing, but that's where
"maybe there are tools that parse this" came in.

I doubt there are any, though. If proc-ps doesn't look at this, I
don't know what could. But the format change might as well be a
separate thing if somebody cares deeply.

                     Linus

Re: [PATCH 0/5] add printk specifier %px, unique identifier

[Joe Perches]

On Tue, 2017-11-28 at 09:33 -0800, Linus Torvalds wrote:
> On Mon, Nov 27, 2017 at 10:26 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
> > > 
> > > Oh well, I just did /proc/<pid>/stack by making it just print 0
> > > unconditionally rather than the hex number.
> > 
> > Patch?
> 
> Oh, apparently I never pushed out yesterday.
> 
> The patch literally just affects the (useless) hex number. So:
> 
>     cat /proc/self/stack
> 
> now prints out
> 
>     [<0>] proc_pid_stack+0xaa/0x100
>     [<0>] proc_single_show+0x48/0x80
>     [<0>] seq_read+0xd2/0x410

> I considered getting rid of the whole "[<>]" thing, but that's where
> "maybe there are tools that parse this" came in.
> 
> I doubt there are any, though. If proc-ps doesn't look at this, I
> don't know what could. But the format change might as well be a
> separate thing if somebody cares deeply.

Perhaps if there are really tools that parse this
the [<leading-0-width>] should be kept the same too.

> 

RE: [PATCH 0/5] add printk specifier %px, unique identifier

[David Laight]

From: Linus Torvalds
> Sent: 28 November 2017 17:33
> 
> On Mon, Nov 27, 2017 at 10:26 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
> >>
> >> Oh well, I just did /proc/<pid>/stack by making it just print 0
> >> unconditionally rather than the hex number.
> >
> > Patch?
> 
> Oh, apparently I never pushed out yesterday.
> 
> The patch literally just affects the (useless) hex number. So:
> 
>     cat /proc/self/stack
> 
> now prints out
> 
>     [<0>] proc_pid_stack+0xaa/0x100
>     [<0>] proc_single_show+0x48/0x80
>     [<0>] seq_read+0xd2/0x410
>     ...
> 
> instead of putting some randomized kernel address there.

Not sure I've done it on Linux - getting a hexdump of the stack is hard.
But I know I've used the absolute return addresses to help hand-decode
the stack.
Usually needed to work out which stack frame is which - especially when the
stack decode doesn't actually (obviously) contain the addresses of each frame.

I don't know how these new stack traceback methods work, but the best one
I've seen in the past disassembled forwards remembering the stack offset
and unprocessed branch targets until it found a return address.
It only had to track %sp and %bp.

	David

Re: [PATCH 0/5] add printk specifier %px, unique identifier

[Linus Torvalds]

On Tue, Nov 28, 2017 at 9:41 AM, Joe Perches <joe@perches.com> wrote:
>
> Perhaps if there are really tools that parse this
> the [<leading-0-width>] should be kept the same too.

If we really get failures, we can do that.

But since the width is already different on 32-bit and 64-bit, I
seriously doubt there are any tools that would care about the width.
They'd already be horribly broken.

So let's not waste space with pointless zeroes unless somebody finds a
case where it matters.

            Linus

Re: [PATCH 0/5] add printk specifier %px, unique identifier

[Linus Torvalds]

On Tue, Nov 28, 2017 at 10:04 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> If we really get failures, we can do that.

Anyway, it's pushed out now so people can test whatever workflows they have.

As mentioned, I doubt anybody cares. That file is already conditional
on CONFIG_STACKTRACE, and while that may be something that all distros
do enable, I know that I have run without it and never even realized.

So it's not just that the numbers are different widths on different
architectures (including the "running 32-bit user space x86 on a
64-bit kernel" case), the whole file isn't even always there, and I
can't say that I've ever heard of problems with /proc/<pid>/stack.

So this file almost certainly doesn't matter to begin with, and with
KASLR (which everybody should have anyway) the numerical values are
useless to anybody except for some attacker that wants to get the
kaslr offset.

We've had kasrl for a long time, this is just a (small) part of
actually making it halfway relevant.

               Linus