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

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

[Tobin C. Harding]

Version 2 of Greg's patch series with changes made as suggested by comments to V1.

Applies on top of Linus' current development tree

a8c964eacb21288b2dbfa9d80cee5968a3b8fb21

V1 cover letter:

Here's a short patch series from Chris Fries and Dave Weinstein that
implements some new restrictions when printing out kernel pointers, as
well as the ability to whitelist kernel pointers where needed.

These patches are based on work from William Roberts, and also are
inspired by grsecurity's %pP to specifically whitelist a kernel pointer,
where it is always needed, like the last patch in the series shows, in
the UIO drivers (UIO requires that you know the address, it's a hardware
address, nothing wrong with seeing that...)

I haven't done much to this patch series, only forward porting it from
an older kernel release (4.4) and a few minor tweaks. [snip]

V1 -> V2:

* Renamed function kptr_restrict_always_cleanse_pointers() to
  kptr_restrict_cleanse_kernel_pointers() (as suggested by Petr Mladek).

* Re-ordered switch statement (within pointer()) to place default at the end
  of the statement (as suggested by Petr Mladek).

* Updated Documentation/printk-formats.txt (as suggested by Joe Perches).

* Updated Documentation/sysctl/kernel.txt (as suggested by Petr Mladek).

Suggestion by Ian Campbell to add comments on the threat model being mitigated
by use of %pa vs %paP etc is not implemented because I do not know the threat
model (I'm only the janitor). Happy to add them if someone writes them.

thanks,
Tobin.

Tobin C. Harding (6):
  lib: vsprintf: additional kernel pointer filtering options
  lib: vsprintf: whitelist stack traces
  lib: vsprintf: physical address kernel pointer filtering options
  lib: vsprintf: default kptr_restrict to the maximum value
  lib: vsprintf: add "%paP", "%papP", and "%padP" specifiers
  drivers: uio: un-restrict sysfs pointers for UIO

 Documentation/printk-formats.txt |  27 ++++++++--
 Documentation/sysctl/kernel.txt  |   9 ++++
 arch/arm64/kernel/traps.c        |   4 +-
 drivers/uio/uio.c                |   4 +-
 kernel/printk/printk.c           |   2 +-
 kernel/sysctl.c                  |   2 +-
 lib/vsprintf.c                   | 114 +++++++++++++++++++++++++++++----------
 scripts/checkpatch.pl            |   2 +-
 8 files changed, 124 insertions(+), 40 deletions(-)

-- 
2.7.4

[kernel-hardening] [RFC V2 1/6] lib: vsprintf: additional kernel pointer filtering options

[Tobin C. Harding]

Add the kptr_restrict setting of 3 which results in both
%p and %pK values being replaced by zeros.

Add an additional %pP value inspired by the Grsecurity
option which explicitly whitelists pointers for output.

Amend scripts/checkpatch.pl to handle %pP.

This patch is based on work by William Roberts
<william.c.roberts@intel.com>

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 Documentation/printk-formats.txt |  8 +++++
 Documentation/sysctl/kernel.txt  |  4 +++
 kernel/sysctl.c                  |  3 +-
 lib/vsprintf.c                   | 78 ++++++++++++++++++++++++++--------------
 scripts/checkpatch.pl            |  2 +-
 5 files changed, 66 insertions(+), 29 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 361789d..16c9abc 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -97,6 +97,14 @@ For printing kernel pointers which should be hidden from unprivileged
 users. The behaviour of ``%pK`` depends on the ``kptr_restrict sysctl`` - see
 Documentation/sysctl/kernel.txt for more details.
 
+::
+
+        %pP     0x01234567 or 0x0123456789abcdef
+
+For printing kernel pointers which should always be shown, even to
+unprivileged users.
+
+
 Struct Resources
 ================
 
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 694968c..7ee183af 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -394,6 +394,10 @@ values to unprivileged users is a concern.
 When kptr_restrict is set to (2), kernel pointers printed using
 %pK will be replaced with 0's regardless of privileges.
 
+When kptr_restrict is set to (3), kernel pointers printed using
+%p and %pK will be replaced with 0's regardless of privileges,
+however kernel pointers printed using %pP will continue to be printed.
+
 ==============================================================
 
 l2cr: (PPC only)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 6648fbb..37ba637 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -129,6 +129,7 @@ static unsigned long one_ul = 1;
 static int one_hundred = 100;
 static int one_thousand = 1000;
 #ifdef CONFIG_PRINTK
+static int three = 3;
 static int ten_thousand = 10000;
 #endif
 #ifdef CONFIG_PERF_EVENTS
@@ -851,7 +852,7 @@ static struct ctl_table kern_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax_sysadmin,
 		.extra1		= &zero,
-		.extra2		= &two,
+		.extra2		= &three,
 	},
 #endif
 	{
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86c3385..e6eace0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -396,6 +396,16 @@ struct printf_spec {
 #define FIELD_WIDTH_MAX ((1 << 23) - 1)
 #define PRECISION_MAX ((1 << 15) - 1)
 
+int kptr_restrict __read_mostly;
+
+/*
+ * return non-zero if we should cleanse pointers for %p and %pK specifiers.
+ */
+static inline int kptr_restrict_cleanse_kernel_pointers(void)
+{
+	return kptr_restrict >= 3;
+}
+
 static noinline_for_stack
 char *number(char *buf, char *end, unsigned long long num,
 	     struct printf_spec spec)
@@ -1591,8 +1601,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
@@ -1664,6 +1672,7 @@ int kptr_restrict __read_mostly;
  *       Do not use this feature without some mechanism to verify the
  *       correctness of the format string and va_list arguments.
  * - 'K' For a kernel pointer that should be hidden from unprivileged users
+ * - 'P' For a kernel pointer that should be shown to all users
  * - 'NF' For a netdev_features_t
  * - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
  *            a certain separator (' ' by default):
@@ -1703,6 +1712,8 @@ 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: That for kptr_restrict set to 3, %p and %pK have the same meaning.
  */
 static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
@@ -1710,7 +1721,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 {
 	const int default_width = 2 * sizeof(void *);
 
-	if (!ptr && *fmt != 'K') {
+	if (!ptr && *fmt != 'K' && !kptr_restrict_cleanse_kernel_pointers()) {
 		/*
 		 * Print (null) with the same width as a pointer so it makes
 		 * tabular output look nice.
@@ -1791,6 +1802,31 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			va_end(va);
 			return buf;
 		}
+	case 'N':
+		return netdev_bits(buf, end, ptr, fmt);
+	case 'a':
+		return address_val(buf, end, ptr, fmt);
+	case 'd':
+		return dentry_name(buf, end, ptr, spec, fmt);
+	case 'C':
+		return clock(buf, end, ptr, spec, fmt);
+	case 'D':
+		return dentry_name(buf, end,
+				   ((const struct file *)ptr)->f_path.dentry,
+				   spec, fmt);
+#ifdef CONFIG_BLOCK
+	case 'g':
+		return bdev_name(buf, end, ptr, spec, fmt);
+#endif
+
+	case 'G':
+		return flags_string(buf, end, ptr, fmt);
+	case 'P':
+		/*
+		 * An explicitly whitelisted kernel pointer should never be
+		 * cleansed
+		 */
+		break;
 	case 'K':
 		switch (kptr_restrict) {
 		case 0:
@@ -1825,39 +1861,27 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 				ptr = NULL;
 			break;
 		}
-		case 2:
+		case 2: /* cleanse %pK for kptr_restrict >= 2 */
 		default:
-			/* Always print 0's for %pK */
 			ptr = NULL;
 			break;
 		}
-		break;
-
-	case 'N':
-		return netdev_bits(buf, end, ptr, fmt);
-	case 'a':
-		return address_val(buf, end, ptr, fmt);
-	case 'd':
-		return dentry_name(buf, end, ptr, spec, fmt);
-	case 'C':
-		return clock(buf, end, ptr, spec, fmt);
-	case 'D':
-		return dentry_name(buf, end,
-				   ((const struct file *)ptr)->f_path.dentry,
-				   spec, fmt);
-#ifdef CONFIG_BLOCK
-	case 'g':
-		return bdev_name(buf, end, ptr, spec, fmt);
-#endif
-
-	case 'G':
-		return flags_string(buf, end, ptr, fmt);
+		break; /* case 'K' */
 	case 'O':
 		switch (fmt[1]) {
 		case 'F':
 			return device_node_string(buf, end, ptr, spec, fmt + 1);
 		}
+		/* fall through */
+	default:
+		/*
+		 * Plain pointers (%p) are always cleansed in higher restriction
+		 * levels.
+		 */
+		if (kptr_restrict >= 3)
+			ptr = NULL;
 	}
+
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {
 		spec.field_width = default_width;
@@ -1865,7 +1889,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	}
 	spec.base = 16;
 
-	return number(buf, end, (unsigned long) ptr, spec);
+	return number(buf, end, (unsigned long long) ptr, spec);
 }
 
 /*
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index dd2c262..eabc56c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5762,7 +5762,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(?![\WFfSsBKRraEhMmIiUDdgVCbGNOP]).)/) {
 					$bad_extension = $1;
 					last;
 				}
-- 
2.7.4

[kernel-hardening] [RFC V2 2/6] lib: vsprintf: whitelist stack traces

[Tobin C. Harding]

Use the %pP functionality to explicitly allow kernel
pointers to be logged for stack traces.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 arch/arm64/kernel/traps.c | 4 ++--
 kernel/printk/printk.c    | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 5ea4b85..fe09660 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -147,7 +147,7 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 	struct stackframe frame;
 	int skip;
 
-	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
+	pr_debug("%s(regs = %pP tsk = %pP)\n", __func__, regs, tsk);
 
 	if (!tsk)
 		tsk = current;
@@ -233,7 +233,7 @@ static int __die(const char *str, int err, struct pt_regs *regs)
 
 	print_modules();
 	__show_regs(regs);
-	pr_emerg("Process %.*s (pid: %d, stack limit = 0x%p)\n",
+	pr_emerg("Process %.*s (pid: %d, stack limit = 0x%pP)\n",
 		 TASK_COMM_LEN, tsk->comm, task_pid_nr(tsk),
 		 end_of_stack(tsk));
 
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 512f7c2..af0bc8e 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3142,7 +3142,7 @@ void show_regs_print_info(const char *log_lvl)
 {
 	dump_stack_print_info(log_lvl);
 
-	printk("%stask: %p task.stack: %p\n",
+	printk("%stask: %pP task.stack: %pP\n",
 	       log_lvl, current, task_stack_page(current));
 }
 
-- 
2.7.4

[kernel-hardening] [RFC V2 3/6] lib: vsprintf: physical address kernel pointer filtering options

[Tobin C. Harding]

Add the kptr_restrict setting of 4 which results in %pa and
%p[rR] values being cleansed.

Address types printed with %pa are replaced by zeros. Resources printed
with %p[rR] have the starting address replaced by zeros, resource size
is still shown.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 Documentation/sysctl/kernel.txt |  5 +++++
 kernel/sysctl.c                 |  3 +--
 lib/vsprintf.c                  | 31 +++++++++++++++++++++++++++++--
 3 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 7ee183af..b6d45bc 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -398,6 +398,11 @@ When kptr_restrict is set to (3), kernel pointers printed using
 %p and %pK will be replaced with 0's regardless of privileges,
 however kernel pointers printed using %pP will continue to be printed.
 
+When kptr_restrict is set to (4), kernel pointers printed with
+%p, %pK, %pa, and %p[rR] will be replaced with 0's regardless of
+privileges. Kernel pointers printed using %pP will continue to be
+printed.
+
 ==============================================================
 
 l2cr: (PPC only)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 37ba637..f777b32 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -129,7 +129,6 @@ static unsigned long one_ul = 1;
 static int one_hundred = 100;
 static int one_thousand = 1000;
 #ifdef CONFIG_PRINTK
-static int three = 3;
 static int ten_thousand = 10000;
 #endif
 #ifdef CONFIG_PERF_EVENTS
@@ -852,7 +851,7 @@ static struct ctl_table kern_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax_sysadmin,
 		.extra1		= &zero,
-		.extra2		= &three,
+		.extra2		= &four,
 	},
 #endif
 	{
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index e6eace0..0271223 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -406,6 +406,22 @@ static inline int kptr_restrict_cleanse_kernel_pointers(void)
 	return kptr_restrict >= 3;
 }
 
+/*
+ * return non-zero if we should cleanse pointers for %pa* specifiers.
+ */
+static inline int kptr_restrict_cleanse_addresses(void)
+{
+	return kptr_restrict >= 4;
+}
+
+/*
+ * return non-zero if we should cleanse pointers for %p[rR] specifiers.
+ */
+static inline int kptr_restrict_cleanse_resources(void)
+{
+	return kptr_restrict >= 4;
+}
+
 static noinline_for_stack
 char *number(char *buf, char *end, unsigned long long num,
 	     struct printf_spec spec)
@@ -758,6 +774,7 @@ char *resource_string(char *buf, char *end, struct resource *res,
 
 	char *p = sym, *pend = sym + sizeof(sym);
 	int decode = (fmt[0] == 'R') ? 1 : 0;
+	int cleanse = kptr_restrict_cleanse_resources();
 	const struct printf_spec *specp;
 
 	*p++ = '[';
@@ -785,10 +802,11 @@ char *resource_string(char *buf, char *end, struct resource *res,
 		p = string(p, pend, "size ", str_spec);
 		p = number(p, pend, resource_size(res), *specp);
 	} else {
-		p = number(p, pend, res->start, *specp);
+		p = number(p, pend, cleanse ? 0UL : res->start, *specp);
 		if (res->start != res->end) {
 			*p++ = '-';
-			p = number(p, pend, res->end, *specp);
+			p = number(p, pend, cleanse ?
+				res->end - res->start : res->end, *specp);
 		}
 	}
 	if (decode) {
@@ -1391,6 +1409,9 @@ char *address_val(char *buf, char *end, const void *addr, const char *fmt)
 		break;
 	}
 
+	if (kptr_restrict_cleanse_addresses())
+		num = 0UL;
+
 	return special_hex_number(buf, end, num, size);
 }
 
@@ -1714,6 +1735,12 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
  * pointer to the real address.
  *
  * Note: That for kptr_restrict set to 3, %p and %pK have the same meaning.
+ *
+ * Note: That for kptr_restrict set to 4, %pa will null out the physical
+ * address.
+ *
+ * Note: That for kptr_restrict set to 4, %p[rR] will null out the memory
+ * address.
  */
 static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
-- 
2.7.4

[kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to the maximum value

[Tobin C. Harding]

Set the initial value of kptr_restrict to the maximum
setting rather than the minimum setting, to ensure that
early boot logging is not leaking information.

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

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 0271223..e009325 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -396,7 +396,7 @@ struct printf_spec {
 #define FIELD_WIDTH_MAX ((1 << 23) - 1)
 #define PRECISION_MAX ((1 << 15) - 1)
 
-int kptr_restrict __read_mostly;
+int kptr_restrict __read_mostly = 4; /* maximum setting */
 
 /*
  * return non-zero if we should cleanse pointers for %p and %pK specifiers.
-- 
2.7.4

[kernel-hardening] [RFC V2 5/6] lib: vsprintf: add "%paP", "%papP", and "%padP" specifiers

[Tobin C. Harding]

Add %papP and %padP for address types that need to always be shown
regardless of kptr restrictions. Add %paP is a synonym for %papP, this
is inline with current implementation (%pa is a synonym for %pap).

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 Documentation/printk-formats.txt | 19 +++++++++++++++----
 Documentation/sysctl/kernel.txt  |  4 ++--
 lib/vsprintf.c                   |  9 +++++++--
 3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 16c9abc..d247bc8 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -119,26 +119,37 @@ For printing struct resources. The ``R`` and ``r`` specifiers result in a
 printed resource with (``R``) or without (``r``) a decoded flags member.
 Passed by reference.
 
+Physical addresses types
+========================
+
+::
+
+	%pa[P]	0x01234567 or 0x0123456789abcdef
+
+Synonymous with ``%pap[P]`` (for printing ``phys_addr_t``). See below.
+
 Physical addresses types ``phys_addr_t``
 ========================================
 
 ::
 
-	%pa[p]	0x01234567 or 0x0123456789abcdef
+	%pap[P]	0x01234567 or 0x0123456789abcdef
 
 For printing a ``phys_addr_t`` type (and its derivatives, such as
 ``resource_size_t``) which can vary based on build options, regardless of
-the width of the CPU data path. Passed by reference.
+the width of the CPU data path. Passed by reference. Use the trailing 'P'
+if it needs to be always shown.
 
 DMA addresses types ``dma_addr_t``
 ==================================
 
 ::
 
-	%pad	0x01234567 or 0x0123456789abcdef
+	%pad[P]         0x01234567 or 0x0123456789abcdef
 
 For printing a ``dma_addr_t`` type which can vary based on build options,
-regardless of the width of the CPU data path. Passed by reference.
+regardless of the width of the CPU data path. Passed by reference. Use the
+trailing 'P' if it needs to be always shown.
 
 Raw buffer as an escaped string
 ===============================
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index b6d45bc..f1d52eb 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -400,8 +400,8 @@ however kernel pointers printed using %pP will continue to be printed.
 
 When kptr_restrict is set to (4), kernel pointers printed with
 %p, %pK, %pa, and %p[rR] will be replaced with 0's regardless of
-privileges. Kernel pointers printed using %pP will continue to be
-printed.
+privileges. Kernel pointers printed using %pP, %paP, %papP, %padP will
+continue to be printed.
 
 ==============================================================
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index e009325..1a35a7f 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1395,21 +1395,26 @@ static noinline_for_stack
 char *address_val(char *buf, char *end, const void *addr, const char *fmt)
 {
 	unsigned long long num;
+	int cleanse = kptr_restrict_cleanse_addresses();
+	int decleanse_idx = 1;
 	int size;
 
 	switch (fmt[1]) {
 	case 'd':
 		num = *(const dma_addr_t *)addr;
 		size = sizeof(dma_addr_t);
+		decleanse_idx = 2;
 		break;
 	case 'p':
+		decleanse_idx = 2;
+		/* fall through */
 	default:
 		num = *(const phys_addr_t *)addr;
 		size = sizeof(phys_addr_t);
 		break;
 	}
-
-	if (kptr_restrict_cleanse_addresses())
+	/* 'P' on the tail means don't restrict the pointer. */
+	if (cleanse && (fmt[decleanse_idx] != 'P'))
 		num = 0UL;
 
 	return special_hex_number(buf, end, num, size);
-- 
2.7.4

[kernel-hardening] [RFC V2 6/6] drivers: uio: un-restrict sysfs pointers for UIO

[Tobin C. Harding]

The address and size on the UIO devices are required by userspace to
function properly.  Let's un-restrict these by adding the 'P' modifier
to %pa.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 drivers/uio/uio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index ff04b7f..728ec8f 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -56,12 +56,12 @@ static ssize_t map_name_show(struct uio_mem *mem, char *buf)
 
 static ssize_t map_addr_show(struct uio_mem *mem, char *buf)
 {
-	return sprintf(buf, "%pa\n", &mem->addr);
+	return sprintf(buf, "%paP\n", &mem->addr);
 }
 
 static ssize_t map_size_show(struct uio_mem *mem, char *buf)
 {
-	return sprintf(buf, "%pa\n", &mem->size);
+	return sprintf(buf, "%paP\n", &mem->size);
 }
 
 static ssize_t map_offset_show(struct uio_mem *mem, char *buf)
-- 
2.7.4

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

[Tobin C. Harding]

On Sun, Oct 01, 2017 at 11:06:44AM +1100, Tobin C. Harding wrote:
> Version 2 of Greg's patch series with changes made as suggested by comments to V1.

Patch set tested by setting /proc/sys/kernel/kptr_restrict and inserting the following module

#include <linux/init.h>
#include <linux/module.h>

#define DRIVER_AUTHOR "Tobin C. Harding <me@tobin.cc>"
#define DRIVER_DESC "Testing module"

static void test_printk(void)
{
	char *ptr = "";

	pr_alert("printing with p: %p\n", ptr);
	pr_alert("printing with pK: %pK\n", ptr);
	pr_alert("printing with pP: %pP\n", ptr);

	pr_alert("printing with pa: %pa\n", ptr);
	pr_alert("printing with pad: %pad\n", ptr);
	pr_alert("printing with pap: %pap\n", ptr);

	pr_alert("printing with paP: %paP\n", ptr);
	pr_alert("printing with padP: %padP\n", ptr);
	pr_alert("printing with papP: %papP\n", ptr);

	pr_alert("printing with pr: %pr\n", ptr);
	pr_alert("printing with pR: %pR\n", ptr);
}

static int hello_init(void)
{
	pr_alert("Hello, world\n");

	test_printk();

	return 0;
}
module_init(hello_init);

static void hello_exit(void)
{
	pr_alert("Goodbye, world\n");
}
module_exit(hello_exit);

MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_AUTHOR(DRIVER_AUTHOR);
MODULE_LICENSE("GPL");

Re: [kernel-hardening] [RFC V2 2/6] lib: vsprintf: whitelist stack traces

[Will Deacon]

On Sun, Oct 01, 2017 at 11:06:46AM +1100, Tobin C. Harding wrote:
> Use the %pP functionality to explicitly allow kernel
> pointers to be logged for stack traces.
> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> ---
>  arch/arm64/kernel/traps.c | 4 ++--
>  kernel/printk/printk.c    | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 5ea4b85..fe09660 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -147,7 +147,7 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
>  	struct stackframe frame;
>  	int skip;
>  
> -	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
> +	pr_debug("%s(regs = %pP tsk = %pP)\n", __func__, regs, tsk);

Why do we care for pr_debug?

>  	if (!tsk)
>  		tsk = current;
> @@ -233,7 +233,7 @@ static int __die(const char *str, int err, struct pt_regs *regs)
>  
>  	print_modules();
>  	__show_regs(regs);
> -	pr_emerg("Process %.*s (pid: %d, stack limit = 0x%p)\n",
> +	pr_emerg("Process %.*s (pid: %d, stack limit = 0x%pP)\n",
>  		 TASK_COMM_LEN, tsk->comm, task_pid_nr(tsk),
>  		 end_of_stack(tsk));
>  
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 512f7c2..af0bc8e 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c

It probably makes sense to keep this separate from arch/ changes

Will

Re: [kernel-hardening] [RFC V2 2/6] lib: vsprintf: whitelist stack traces

[Tobin C. Harding]

On Mon, Oct 02, 2017 at 11:42:05AM +0100, Will Deacon wrote:
> On Sun, Oct 01, 2017 at 11:06:46AM +1100, Tobin C. Harding wrote:
> > Use the %pP functionality to explicitly allow kernel
> > pointers to be logged for stack traces.
> > 
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > ---
> >  arch/arm64/kernel/traps.c | 4 ++--
> >  kernel/printk/printk.c    | 2 +-
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > index 5ea4b85..fe09660 100644
> > --- a/arch/arm64/kernel/traps.c
> > +++ b/arch/arm64/kernel/traps.c
> > @@ -147,7 +147,7 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
> >  	struct stackframe frame;
> >  	int skip;
> >  
> > -	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
> > +	pr_debug("%s(regs = %pP tsk = %pP)\n", __func__, regs, tsk);
> 
> Why do we care for pr_debug?

Thanks for the review Will. I did not do the original work on this series so do not know the
original intent behind this change. I tend to agree with your comment. Will remove this change for
v3.

> >  	if (!tsk)
> >  		tsk = current;
> > @@ -233,7 +233,7 @@ static int __die(const char *str, int err, struct pt_regs *regs)
> >  
> >  	print_modules();
> >  	__show_regs(regs);
> > -	pr_emerg("Process %.*s (pid: %d, stack limit = 0x%p)\n",
> > +	pr_emerg("Process %.*s (pid: %d, stack limit = 0x%pP)\n",
> >  		 TASK_COMM_LEN, tsk->comm, task_pid_nr(tsk),
> >  		 end_of_stack(tsk));
> >  
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 512f7c2..af0bc8e 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> 
> It probably makes sense to keep this separate from arch/ changes

Point noted.

thanks,
Tobin.

Re: [kernel-hardening] [RFC V2 1/6] lib: vsprintf: additional kernel pointer filtering options

[Greg KH]

On Sun, Oct 01, 2017 at 11:06:45AM +1100, Tobin C. Harding wrote:
> Add the kptr_restrict setting of 3 which results in both
> %p and %pK values being replaced by zeros.
> 
> Add an additional %pP value inspired by the Grsecurity
> option which explicitly whitelists pointers for output.
> 
> Amend scripts/checkpatch.pl to handle %pP.
> 
> This patch is based on work by William Roberts
> <william.c.roberts@intel.com>
> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to the maximum value

[Greg KH]

On Sun, Oct 01, 2017 at 11:06:48AM +1100, Tobin C. Harding wrote:
> Set the initial value of kptr_restrict to the maximum
> setting rather than the minimum setting, to ensure that
> early boot logging is not leaking information.
> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [kernel-hardening] [RFC V2 2/6] lib: vsprintf: whitelist stack traces

[Greg KH]

On Mon, Oct 02, 2017 at 11:42:05AM +0100, Will Deacon wrote:
> On Sun, Oct 01, 2017 at 11:06:46AM +1100, Tobin C. Harding wrote:
> > Use the %pP functionality to explicitly allow kernel
> > pointers to be logged for stack traces.
> > 
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > ---
> >  arch/arm64/kernel/traps.c | 4 ++--
> >  kernel/printk/printk.c    | 2 +-
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > index 5ea4b85..fe09660 100644
> > --- a/arch/arm64/kernel/traps.c
> > +++ b/arch/arm64/kernel/traps.c
> > @@ -147,7 +147,7 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
> >  	struct stackframe frame;
> >  	int skip;
> >  
> > -	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
> > +	pr_debug("%s(regs = %pP tsk = %pP)\n", __func__, regs, tsk);
> 
> Why do we care for pr_debug?

Because you really want the real value?  Seems to make sense to me...

thanks,

greg k-h

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

[Greg KH]

On Sun, Oct 01, 2017 at 11:11:05AM +1100, Tobin C. Harding wrote:
> On Sun, Oct 01, 2017 at 11:06:44AM +1100, Tobin C. Harding wrote:
> > Version 2 of Greg's patch series with changes made as suggested by comments to V1.
> 
> Patch set tested by setting /proc/sys/kernel/kptr_restrict and inserting the following module
> 
> #include <linux/init.h>
> #include <linux/module.h>
> 
> #define DRIVER_AUTHOR "Tobin C. Harding <me@tobin.cc>"
> #define DRIVER_DESC "Testing module"
> 
> static void test_printk(void)
> {
> 	char *ptr = "";
> 
> 	pr_alert("printing with p: %p\n", ptr);
> 	pr_alert("printing with pK: %pK\n", ptr);
> 	pr_alert("printing with pP: %pP\n", ptr);
> 
> 	pr_alert("printing with pa: %pa\n", ptr);
> 	pr_alert("printing with pad: %pad\n", ptr);
> 	pr_alert("printing with pap: %pap\n", ptr);
> 
> 	pr_alert("printing with paP: %paP\n", ptr);
> 	pr_alert("printing with padP: %padP\n", ptr);
> 	pr_alert("printing with papP: %papP\n", ptr);
> 
> 	pr_alert("printing with pr: %pr\n", ptr);
> 	pr_alert("printing with pR: %pR\n", ptr);
> }
> 
> static int hello_init(void)
> {
> 	pr_alert("Hello, world\n");
> 
> 	test_printk();
> 
> 	return 0;
> }
> module_init(hello_init);
> 
> static void hello_exit(void)
> {
> 	pr_alert("Goodbye, world\n");
> }
> module_exit(hello_exit);
> 
> MODULE_DESCRIPTION(DRIVER_DESC);
> MODULE_AUTHOR(DRIVER_AUTHOR);
> MODULE_LICENSE("GPL");


Hm, any way to add something like this to the testing infrastructure?
Or maybe just at boot time?  It's nice to keep tests around to ensure
things do not break :)

thanks,

greg k-h

Re: [kernel-hardening] [RFC V2 3/6] lib: vsprintf: physical address kernel pointer filtering options

[Greg KH]

On Sun, Oct 01, 2017 at 11:06:47AM +1100, Tobin C. Harding wrote:
> Add the kptr_restrict setting of 4 which results in %pa and
> %p[rR] values being cleansed.
> 
> Address types printed with %pa are replaced by zeros. Resources printed
> with %p[rR] have the starting address replaced by zeros, resource size
> is still shown.
> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [kernel-hardening] [RFC V2 2/6] lib: vsprintf: whitelist stack traces

[Will Deacon]

On Wed, Oct 04, 2017 at 10:56:57AM +0200, Greg KH wrote:
> On Mon, Oct 02, 2017 at 11:42:05AM +0100, Will Deacon wrote:
> > On Sun, Oct 01, 2017 at 11:06:46AM +1100, Tobin C. Harding wrote:
> > > Use the %pP functionality to explicitly allow kernel
> > > pointers to be logged for stack traces.
> > > 
> > > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > > ---
> > >  arch/arm64/kernel/traps.c | 4 ++--
> > >  kernel/printk/printk.c    | 2 +-
> > >  2 files changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > > index 5ea4b85..fe09660 100644
> > > --- a/arch/arm64/kernel/traps.c
> > > +++ b/arch/arm64/kernel/traps.c
> > > @@ -147,7 +147,7 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
> > >  	struct stackframe frame;
> > >  	int skip;
> > >  
> > > -	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
> > > +	pr_debug("%s(regs = %pP tsk = %pP)\n", __func__, regs, tsk);
> > 
> > Why do we care for pr_debug?
> 
> Because you really want the real value?  Seems to make sense to me...

Just seems like anybody debugging the kernel using pr_debug can probably
change /proc/sys/kernel/kptr_restrict...

Will

Re: [kernel-hardening] [RFC V2 5/6] lib: vsprintf: add "%paP", "%papP", and "%padP" specifiers

[Greg KH]

On Sun, Oct 01, 2017 at 11:06:49AM +1100, Tobin C. Harding wrote:
> Add %papP and %padP for address types that need to always be shown
> regardless of kptr restrictions. Add %paP is a synonym for %papP, this
> is inline with current implementation (%pa is a synonym for %pap).
> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [kernel-hardening] [RFC V2 6/6] drivers: uio: un-restrict sysfs pointers for UIO

[Greg KH]

On Sun, Oct 01, 2017 at 11:06:50AM +1100, Tobin C. Harding wrote:
> The address and size on the UIO devices are required by userspace to
> function properly.  Let's un-restrict these by adding the 'P' modifier
> to %pa.
> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

[Greg KH]

On Sun, Oct 01, 2017 at 11:06:44AM +1100, Tobin C. Harding wrote:
> Version 2 of Greg's patch series with changes made as suggested by comments to V1.
> 
> Applies on top of Linus' current development tree
> 
> a8c964eacb21288b2dbfa9d80cee5968a3b8fb21
> 
> V1 cover letter:
> 
> Here's a short patch series from Chris Fries and Dave Weinstein that
> implements some new restrictions when printing out kernel pointers, as
> well as the ability to whitelist kernel pointers where needed.
> 
> These patches are based on work from William Roberts, and also are
> inspired by grsecurity's %pP to specifically whitelist a kernel pointer,
> where it is always needed, like the last patch in the series shows, in
> the UIO drivers (UIO requires that you know the address, it's a hardware
> address, nothing wrong with seeing that...)
> 
> I haven't done much to this patch series, only forward porting it from
> an older kernel release (4.4) and a few minor tweaks. [snip]

Nice!  Thanks for doing this work, looks great to me.  Care to resend
the next version as a "real" one (i.e. no RFC)?

thanks,

greg k-h

Re: [kernel-hardening] [RFC V2 2/6] lib: vsprintf: whitelist stack traces

[Greg KH]

On Wed, Oct 04, 2017 at 09:58:10AM +0100, Will Deacon wrote:
> On Wed, Oct 04, 2017 at 10:56:57AM +0200, Greg KH wrote:
> > On Mon, Oct 02, 2017 at 11:42:05AM +0100, Will Deacon wrote:
> > > On Sun, Oct 01, 2017 at 11:06:46AM +1100, Tobin C. Harding wrote:
> > > > Use the %pP functionality to explicitly allow kernel
> > > > pointers to be logged for stack traces.
> > > > 
> > > > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > > > ---
> > > >  arch/arm64/kernel/traps.c | 4 ++--
> > > >  kernel/printk/printk.c    | 2 +-
> > > >  2 files changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > > > index 5ea4b85..fe09660 100644
> > > > --- a/arch/arm64/kernel/traps.c
> > > > +++ b/arch/arm64/kernel/traps.c
> > > > @@ -147,7 +147,7 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
> > > >  	struct stackframe frame;
> > > >  	int skip;
> > > >  
> > > > -	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
> > > > +	pr_debug("%s(regs = %pP tsk = %pP)\n", __func__, regs, tsk);
> > > 
> > > Why do we care for pr_debug?
> > 
> > Because you really want the real value?  Seems to make sense to me...
> 
> Just seems like anybody debugging the kernel using pr_debug can probably
> change /proc/sys/kernel/kptr_restrict...

Ok, fair enough :)

Re: [kernel-hardening] [RFC V2 2/6] lib: vsprintf: whitelist stack traces

[Tobin C. Harding]

On Wed, Oct 04, 2017 at 11:02:55AM +0200, Greg KH wrote:
> On Wed, Oct 04, 2017 at 09:58:10AM +0100, Will Deacon wrote:
> > On Wed, Oct 04, 2017 at 10:56:57AM +0200, Greg KH wrote:
> > > On Mon, Oct 02, 2017 at 11:42:05AM +0100, Will Deacon wrote:
> > > > On Sun, Oct 01, 2017 at 11:06:46AM +1100, Tobin C. Harding wrote:
> > > > > Use the %pP functionality to explicitly allow kernel
> > > > > pointers to be logged for stack traces.
> > > > > 
> > > > > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > > > > ---
> > > > >  arch/arm64/kernel/traps.c | 4 ++--
> > > > >  kernel/printk/printk.c    | 2 +-
> > > > >  2 files changed, 3 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > > > > index 5ea4b85..fe09660 100644
> > > > > --- a/arch/arm64/kernel/traps.c
> > > > > +++ b/arch/arm64/kernel/traps.c
> > > > > @@ -147,7 +147,7 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
> > > > >  	struct stackframe frame;
> > > > >  	int skip;
> > > > >  
> > > > > -	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
> > > > > +	pr_debug("%s(regs = %pP tsk = %pP)\n", __func__, regs, tsk);
> > > > 
> > > > Why do we care for pr_debug?
> > > 
> > > Because you really want the real value?  Seems to make sense to me...
> > 
> > Just seems like anybody debugging the kernel using pr_debug can probably
> > change /proc/sys/kernel/kptr_restrict...
> 
> Ok, fair enough :)

For what its worth I agree with Will.

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

[Tobin C. Harding]

On Wed, Oct 04, 2017 at 10:57:56AM +0200, Greg KH wrote:
> On Sun, Oct 01, 2017 at 11:11:05AM +1100, Tobin C. Harding wrote:
> > On Sun, Oct 01, 2017 at 11:06:44AM +1100, Tobin C. Harding wrote:
> > > Version 2 of Greg's patch series with changes made as suggested by comments to V1.
> > 
> > Patch set tested by setting /proc/sys/kernel/kptr_restrict and inserting the following module
> > 
> > #include <linux/init.h>
> > #include <linux/module.h>
> > 
> > #define DRIVER_AUTHOR "Tobin C. Harding <me@tobin.cc>"
> > #define DRIVER_DESC "Testing module"
> > 
> > static void test_printk(void)
> > {
> > 	char *ptr = "";
> > 
> > 	pr_alert("printing with p: %p\n", ptr);
> > 	pr_alert("printing with pK: %pK\n", ptr);
> > 	pr_alert("printing with pP: %pP\n", ptr);
> > 
> > 	pr_alert("printing with pa: %pa\n", ptr);
> > 	pr_alert("printing with pad: %pad\n", ptr);
> > 	pr_alert("printing with pap: %pap\n", ptr);
> > 
> > 	pr_alert("printing with paP: %paP\n", ptr);
> > 	pr_alert("printing with padP: %padP\n", ptr);
> > 	pr_alert("printing with papP: %papP\n", ptr);
> > 
> > 	pr_alert("printing with pr: %pr\n", ptr);
> > 	pr_alert("printing with pR: %pR\n", ptr);
> > }
> > 
> > static int hello_init(void)
> > {
> > 	pr_alert("Hello, world\n");
> > 
> > 	test_printk();
> > 
> > 	return 0;
> > }
> > module_init(hello_init);
> > 
> > static void hello_exit(void)
> > {
> > 	pr_alert("Goodbye, world\n");
> > }
> > module_exit(hello_exit);
> > 
> > MODULE_DESCRIPTION(DRIVER_DESC);
> > MODULE_AUTHOR(DRIVER_AUTHOR);
> > MODULE_LICENSE("GPL");
> 
> 
> Hm, any way to add something like this to the testing infrastructure?
> Or maybe just at boot time?  It's nice to keep tests around to ensure
> things do not break :)

Sure, good idea, I'll look into it. It will be my first attempt at adding tests to the kernel so
will come as a separate patch set some time soon.

thanks,
Tobin.

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

[Tobin C. Harding]

On Wed, Oct 04, 2017 at 10:58:50AM +0200, Greg KH wrote:
> On Sun, Oct 01, 2017 at 11:06:44AM +1100, Tobin C. Harding wrote:
> > Version 2 of Greg's patch series with changes made as suggested by comments to V1.
> > 
> > Applies on top of Linus' current development tree
> > 
> > a8c964eacb21288b2dbfa9d80cee5968a3b8fb21
> > 
> > V1 cover letter:
> > 
> > Here's a short patch series from Chris Fries and Dave Weinstein that
> > implements some new restrictions when printing out kernel pointers, as
> > well as the ability to whitelist kernel pointers where needed.
> > 
> > These patches are based on work from William Roberts, and also are
> > inspired by grsecurity's %pP to specifically whitelist a kernel pointer,
> > where it is always needed, like the last patch in the series shows, in
> > the UIO drivers (UIO requires that you know the address, it's a hardware
> > address, nothing wrong with seeing that...)
> > 
> > I haven't done much to this patch series, only forward porting it from
> > an older kernel release (4.4) and a few minor tweaks. [snip]
> 
> Nice!  Thanks for doing this work, looks great to me.  Care to resend
> the next version as a "real" one (i.e. no RFC)?

First thing tomorrow!

Is correct protocol for me to add your Signed-off-by tag to each patch from this RFC? Or is the
protocol for you to add the tag yourself when the real version is posted? 

I intend splitting one of the patches into two as suggested by Will.


thanks,
Tobin.

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

[Greg KH]

On Wed, Oct 04, 2017 at 09:50:51PM +1100, Tobin C. Harding wrote:
> On Wed, Oct 04, 2017 at 10:58:50AM +0200, Greg KH wrote:
> > On Sun, Oct 01, 2017 at 11:06:44AM +1100, Tobin C. Harding wrote:
> > > Version 2 of Greg's patch series with changes made as suggested by comments to V1.
> > > 
> > > Applies on top of Linus' current development tree
> > > 
> > > a8c964eacb21288b2dbfa9d80cee5968a3b8fb21
> > > 
> > > V1 cover letter:
> > > 
> > > Here's a short patch series from Chris Fries and Dave Weinstein that
> > > implements some new restrictions when printing out kernel pointers, as
> > > well as the ability to whitelist kernel pointers where needed.
> > > 
> > > These patches are based on work from William Roberts, and also are
> > > inspired by grsecurity's %pP to specifically whitelist a kernel pointer,
> > > where it is always needed, like the last patch in the series shows, in
> > > the UIO drivers (UIO requires that you know the address, it's a hardware
> > > address, nothing wrong with seeing that...)
> > > 
> > > I haven't done much to this patch series, only forward porting it from
> > > an older kernel release (4.4) and a few minor tweaks. [snip]
> > 
> > Nice!  Thanks for doing this work, looks great to me.  Care to resend
> > the next version as a "real" one (i.e. no RFC)?
> 
> First thing tomorrow!
> 
> Is correct protocol for me to add your Signed-off-by tag to each patch from this RFC? Or is the
> protocol for you to add the tag yourself when the real version is posted? 

You can add my signed-off-by to your new patches, they shouldn't change
much with the exception of:

> I intend splitting one of the patches into two as suggested by Will.

And that's fine to keep my s-o-b for.

thanks for asking,

greg k-h

Re: [kernel-hardening] [RFC V2 1/6] lib: vsprintf: additional kernel pointer filtering options

[Steven Rostedt]

On Wed, 4 Oct 2017 10:55:42 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Sun, Oct 01, 2017 at 11:06:45AM +1100, Tobin C. Harding wrote:
> > Add the kptr_restrict setting of 3 which results in both
> > %p and %pK values being replaced by zeros.
> > 
> > Add an additional %pP value inspired by the Grsecurity
> > option which explicitly whitelists pointers for output.
> > 
> > Amend scripts/checkpatch.pl to handle %pP.
> > 
> > This patch is based on work by William Roberts
> > <william.c.roberts@intel.com>
> > 
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>  
> 
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Greg,

I'm curious to why you are adding a "Signed-off-by"? That usually means
that you either help author it, or it is going through your tree. I've
never seen a "Signed-off-by" in passing. That's usually a "Acked-by" or
"Reviewed-by".

In other words, I was told that a "Signed-off-by" should never be added
by anyone but the one who writes it. The original author adds it to the
patch they send, and the maintainer adds it when they pull in that
patch. But sending out a "Signed-off-by" like this, to be added, would
require someone else writing it for you. Which I was told was a no-no.

-- Steve

Re: [kernel-hardening] [RFC V2 1/6] lib: vsprintf: additional kernel pointer filtering options

[Greg KH]

On Wed, Oct 04, 2017 at 09:08:57AM -0400, Steven Rostedt wrote:
> On Wed, 4 Oct 2017 10:55:42 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Sun, Oct 01, 2017 at 11:06:45AM +1100, Tobin C. Harding wrote:
> > > Add the kptr_restrict setting of 3 which results in both
> > > %p and %pK values being replaced by zeros.
> > > 
> > > Add an additional %pP value inspired by the Grsecurity
> > > option which explicitly whitelists pointers for output.
> > > 
> > > Amend scripts/checkpatch.pl to handle %pP.
> > > 
> > > This patch is based on work by William Roberts
> > > <william.c.roberts@intel.com>
> > > 
> > > Signed-off-by: Tobin C. Harding <me@tobin.cc>  
> > 
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Greg,
> 
> I'm curious to why you are adding a "Signed-off-by"? That usually means
> that you either help author it, or it is going through your tree. I've
> never seen a "Signed-off-by" in passing. That's usually a "Acked-by" or
> "Reviewed-by".

I did help author it :)

Hope that helps,

greg k-h

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

[Steven Rostedt]

On Wed, 4 Oct 2017 14:42:33 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> > Is correct protocol for me to add your Signed-off-by tag to each patch from this RFC? Or is the
> > protocol for you to add the tag yourself when the real version is posted?   
> 
> You can add my signed-off-by to your new patches,

I was always told that one should never add someone else's
signed-off-by, because that's not what it means. 

I was told that this would be an Acked-by or Reviewed-by.

>From Documentation/process/submitting-patches.rst:

====
12) When to use Acked-by: and Cc:
---------------------------------

The Signed-off-by: tag indicates that the signer was involved in the
development of the patch, or that he/she was in the patch's delivery path.

If a person was not directly involved in the preparation or handling of a
patch but wishes to signify and record their approval of it then they can
ask to have an Acked-by: line added to the patch's changelog.

Acked-by: is often used by the maintainer of the affected code when that
maintainer neither contributed to nor forwarded the patch.

Acked-by: is not as formal as Signed-off-by:.  It is a record that the acker
has at least reviewed the patch and has indicated acceptance.  Hence patch
mergers will sometimes manually convert an acker's "yep, looks good to me"
into an Acked-by: (but note that it is usually better to ask for an
explicit ack).
====

-- Steve

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

[Steven Rostedt]

On Wed, 4 Oct 2017 14:42:33 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> > Is correct protocol for me to add your Signed-off-by tag to each patch from this RFC? Or is the
> > protocol for you to add the tag yourself when the real version is posted?   
> 
> You can add my signed-off-by to your new patches,

I was always told that one should never add someone else's
signed-off-by, because that's not what it means. 

I was told that this would be an Acked-by or Reviewed-by.

From Documentation/process/submitting-patches.rst:

====
12) When to use Acked-by: and Cc:
---------------------------------

The Signed-off-by: tag indicates that the signer was involved in the
development of the patch, or that he/she was in the patch's delivery path.

If a person was not directly involved in the preparation or handling of a
patch but wishes to signify and record their approval of it then they can
ask to have an Acked-by: line added to the patch's changelog.

Acked-by: is often used by the maintainer of the affected code when that
maintainer neither contributed to nor forwarded the patch.

Acked-by: is not as formal as Signed-off-by:.  It is a record that the acker
has at least reviewed the patch and has indicated acceptance.  Hence patch
mergers will sometimes manually convert an acker's "yep, looks good to me"
into an Acked-by: (but note that it is usually better to ask for an
explicit ack).
====

-- Steve

Re: [kernel-hardening] [RFC V2 1/6] lib: vsprintf: additional kernel pointer filtering options

[Steven Rostedt]

On Wed, 4 Oct 2017 15:26:50 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:


> > I'm curious to why you are adding a "Signed-off-by"? That usually means
> > that you either help author it, or it is going through your tree. I've
> > never seen a "Signed-off-by" in passing. That's usually a "Acked-by" or
> > "Reviewed-by".  
> 
> I did help author it :)
> 
> Hope that helps,

Ah! Than than makes a huge difference. I just didn't see anything in
the change log that suggested that you did.

Thanks,

-- Steve

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

[Steven Rostedt]

As Greg stated that he helped author the patch, you can ignore this
email. Sorry for the noise.

-- Steve

On Wed, 4 Oct 2017 09:28:15 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 4 Oct 2017 14:42:33 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > > Is correct protocol for me to add your Signed-off-by tag to each patch from this RFC? Or is the
> > > protocol for you to add the tag yourself when the real version is posted?     
> > 
> > You can add my signed-off-by to your new patches,  
> 
> I was always told that one should never add someone else's
> signed-off-by, because that's not what it means. 
> 
> I was told that this would be an Acked-by or Reviewed-by.
> 
> From Documentation/process/submitting-patches.rst:
> 
> ====
> 12) When to use Acked-by: and Cc:
> ---------------------------------
> 
> The Signed-off-by: tag indicates that the signer was involved in the
> development of the patch, or that he/she was in the patch's delivery path.
> 
> If a person was not directly involved in the preparation or handling of a
> patch but wishes to signify and record their approval of it then they can
> ask to have an Acked-by: line added to the patch's changelog.
> 
> Acked-by: is often used by the maintainer of the affected code when that
> maintainer neither contributed to nor forwarded the patch.
> 
> Acked-by: is not as formal as Signed-off-by:.  It is a record that the acker
> has at least reviewed the patch and has indicated acceptance.  Hence patch
> mergers will sometimes manually convert an acker's "yep, looks good to me"
> into an Acked-by: (but note that it is usually better to ask for an
> explicit ack).
> ====
> 
> -- Steve

Re: [kernel-hardening] [RFC V2 1/6] lib: vsprintf: additional kernel pointer filtering options

[Greg KH]

On Wed, Oct 04, 2017 at 09:29:08AM -0400, Steven Rostedt wrote:
> On Wed, 4 Oct 2017 15:26:50 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> 
> > > I'm curious to why you are adding a "Signed-off-by"? That usually means
> > > that you either help author it, or it is going through your tree. I've
> > > never seen a "Signed-off-by" in passing. That's usually a "Acked-by" or
> > > "Reviewed-by".  
> > 
> > I did help author it :)
> > 
> > Hope that helps,
> 
> Ah! Than than makes a huge difference. I just didn't see anything in
> the change log that suggested that you did.

It's hard to show that, I know others on the cc: helped author it as
well, it's a complex patchset that has gone through many different
authors over many months.  Hopefully Tobin will be the one to finally
finish it up and get it merged.

thanks,

greg k-h

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

[Linus Torvalds]

On Sat, Sep 30, 2017 at 5:06 PM, Tobin C. Harding <me@tobin.cc> wrote:
>   lib: vsprintf: default kptr_restrict to the maximum value

So I'm not convinced about this one.

It removes kernel pointers even for root, which is annoying for things
like perf.

And the only physical pointers we should print out during boot etc are
things we *need*.

So kptr_restrict is wrong for that, bercause either we potentially
need those values for debugging ("why does my kernel not boot"), or
they shouldn't be printed at all.

And I think _that_ is the real issue. If there are places that leak,
we should look at those, rather than just say "kptr_restrict".

                 Linus

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

[Linus Torvalds]

On Sat, Sep 30, 2017 at 5:06 PM, Tobin C. Harding <me@tobin.cc> wrote:
>   lib: vsprintf: default kptr_restrict to the maximum value

So I'm not convinced about this one.

It removes kernel pointers even for root, which is annoying for things
like perf.

And the only physical pointers we should print out during boot etc are
things we *need*.

So kptr_restrict is wrong for that, bercause either we potentially
need those values for debugging ("why does my kernel not boot"), or
they shouldn't be printed at all.

And I think _that_ is the real issue. If there are places that leak,
we should look at those, rather than just say "kptr_restrict".

                 Linus

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

[Roberts, William C]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Wednesday, October 4, 2017 1:59 AM
> To: Tobin C. Harding <me@tobin.cc>
> Cc: Petr Mladek <pmladek@suse.com>; Joe Perches <joe@perches.com>; Ian
> Campbell <ijc@hellion.org.uk>; Sergey Senozhatsky
> <sergey.senozhatsky@gmail.com>; kernel-hardening@lists.openwall.com; linux-
> kernel@vger.kernel.org; Catalin Marinas <catalin.marinas@arm.com>; Will
> Deacon <will.deacon@arm.com>; Steven Rostedt <rostedt@goodmis.org>;
> Roberts, William C <william.c.roberts@intel.com>; Chris Fries
> <cfries@google.com>; Dave Weinstein <olorin@google.com>
> Subject: Re: [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter
> options
> 
> On Sun, Oct 01, 2017 at 11:06:44AM +1100, Tobin C. Harding wrote:
> > Version 2 of Greg's patch series with changes made as suggested by comments
> to V1.
> >
> > Applies on top of Linus' current development tree
> >
> > a8c964eacb21288b2dbfa9d80cee5968a3b8fb21
> >
> > V1 cover letter:
> >
> > Here's a short patch series from Chris Fries and Dave Weinstein that
> > implements some new restrictions when printing out kernel pointers, as
> > well as the ability to whitelist kernel pointers where needed.
> >
> > These patches are based on work from William Roberts, and also are
> > inspired by grsecurity's %pP to specifically whitelist a kernel
> > pointer, where it is always needed, like the last patch in the series
> > shows, in the UIO drivers (UIO requires that you know the address,
> > it's a hardware address, nothing wrong with seeing that...)
> >
> > I haven't done much to this patch series, only forward porting it from
> > an older kernel release (4.4) and a few minor tweaks. [snip]
> 
> Nice!  Thanks for doing this work, looks great to me.  Care to resend the next
> version as a "real" one (i.e. no RFC)?

It looks like the only gripe from others was on the debug output change, so
I'd drop that change out of the series. Otherwise, LGTM.

> 
> thanks,
> 
> greg k-h

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

[Roberts, William C]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Wednesday, October 4, 2017 1:59 AM
> To: Tobin C. Harding <me@tobin.cc>
> Cc: Petr Mladek <pmladek@suse.com>; Joe Perches <joe@perches.com>; Ian
> Campbell <ijc@hellion.org.uk>; Sergey Senozhatsky
> <sergey.senozhatsky@gmail.com>; kernel-hardening@lists.openwall.com; linux-
> kernel@vger.kernel.org; Catalin Marinas <catalin.marinas@arm.com>; Will
> Deacon <will.deacon@arm.com>; Steven Rostedt <rostedt@goodmis.org>;
> Roberts, William C <william.c.roberts@intel.com>; Chris Fries
> <cfries@google.com>; Dave Weinstein <olorin@google.com>
> Subject: Re: [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter
> options
> 
> On Sun, Oct 01, 2017 at 11:06:44AM +1100, Tobin C. Harding wrote:
> > Version 2 of Greg's patch series with changes made as suggested by comments
> to V1.
> >
> > Applies on top of Linus' current development tree
> >
> > a8c964eacb21288b2dbfa9d80cee5968a3b8fb21
> >
> > V1 cover letter:
> >
> > Here's a short patch series from Chris Fries and Dave Weinstein that
> > implements some new restrictions when printing out kernel pointers, as
> > well as the ability to whitelist kernel pointers where needed.
> >
> > These patches are based on work from William Roberts, and also are
> > inspired by grsecurity's %pP to specifically whitelist a kernel
> > pointer, where it is always needed, like the last patch in the series
> > shows, in the UIO drivers (UIO requires that you know the address,
> > it's a hardware address, nothing wrong with seeing that...)
> >
> > I haven't done much to this patch series, only forward porting it from
> > an older kernel release (4.4) and a few minor tweaks. [snip]
> 
> Nice!  Thanks for doing this work, looks great to me.  Care to resend the next
> version as a "real" one (i.e. no RFC)?

It looks like the only gripe from others was on the debug output change, so
I'd drop that change out of the series. Otherwise, LGTM.

> 
> thanks,
> 
> greg k-h

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

[Boris Lukashev]

On Wed, Oct 4, 2017 at 11:41 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sat, Sep 30, 2017 at 5:06 PM, Tobin C. Harding <me@tobin.cc> wrote:
>>   lib: vsprintf: default kptr_restrict to the maximum value
>
> So I'm not convinced about this one.
>
> It removes kernel pointers even for root, which is annoying for things
> like perf.
>

The debugging and perf use cases seem well suited for using a
boot-time parameter to disable the restriction such as to allow
sysadmins to boot in a manner permitting them to see these values,
while running in production without the (potential) leaks.

> And the only physical pointers we should print out during boot etc are
> things we *need*.
>
> So kptr_restrict is wrong for that, bercause either we potentially
> need those values for debugging ("why does my kernel not boot"), or
> they shouldn't be printed at all.
>
> And I think _that_ is the real issue. If there are places that leak,
> we should look at those, rather than just say "kptr_restrict".

When adding modules from outside the mainline tree (zfs, aufs, scst,
etc), we would not be able to audit the source, and risk leaking
sensitive pointers from those components if we dont filter them out
this way or in a similar programmatic manner. There's also the issue
of containers' access to kernel output, making the "root concern"
somewhat more complicated. Large distributions are known to use code
which hasn't, and in some cases never can, pass through the
upstreaming process in your tree (ZFS being the primary case i'm
thinking of for licensing reasons), and it would be a shame to leave
their users with a reduced security posture for letting them utilize
things which can't get mainlined.

>
>                  Linus

Are there production-context requirements for exposing this
information, or is the thought that this really only screws up "trying
to fix or understand" the execution environment, with those being
debugging/tuning functions not usually performed on critical systems
in operation?
If its the latter, then for long running systems with relevant 0days
we're not yet privy to, restriction on the attackers ability to map
physmem can raise the bar for attack complexity significantly,
offering real-world benefit to defenders while mitigation is
implemented to correct the underlying concern.
We do a fair share of work in medical environments, where applying a
kernel update may involve FDA approval or other acrobatics through
flaming hoops (specifically for implanted med devices and their base
stations), so defensive measures providing blanket coverage for a
class of attacker activity (recon of memory layout in this case) are
very welcome to those consumers.

-- 
Boris Lukashev
Systems Architect
Semper Victus

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

[Boris Lukashev]

On Wed, Oct 4, 2017 at 11:41 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sat, Sep 30, 2017 at 5:06 PM, Tobin C. Harding <me@tobin.cc> wrote:
>>   lib: vsprintf: default kptr_restrict to the maximum value
>
> So I'm not convinced about this one.
>
> It removes kernel pointers even for root, which is annoying for things
> like perf.
>

The debugging and perf use cases seem well suited for using a
boot-time parameter to disable the restriction such as to allow
sysadmins to boot in a manner permitting them to see these values,
while running in production without the (potential) leaks.

> And the only physical pointers we should print out during boot etc are
> things we *need*.
>
> So kptr_restrict is wrong for that, bercause either we potentially
> need those values for debugging ("why does my kernel not boot"), or
> they shouldn't be printed at all.
>
> And I think _that_ is the real issue. If there are places that leak,
> we should look at those, rather than just say "kptr_restrict".

When adding modules from outside the mainline tree (zfs, aufs, scst,
etc), we would not be able to audit the source, and risk leaking
sensitive pointers from those components if we dont filter them out
this way or in a similar programmatic manner. There's also the issue
of containers' access to kernel output, making the "root concern"
somewhat more complicated. Large distributions are known to use code
which hasn't, and in some cases never can, pass through the
upstreaming process in your tree (ZFS being the primary case i'm
thinking of for licensing reasons), and it would be a shame to leave
their users with a reduced security posture for letting them utilize
things which can't get mainlined.

>
>                  Linus

Are there production-context requirements for exposing this
information, or is the thought that this really only screws up "trying
to fix or understand" the execution environment, with those being
debugging/tuning functions not usually performed on critical systems
in operation?
If its the latter, then for long running systems with relevant 0days
we're not yet privy to, restriction on the attackers ability to map
physmem can raise the bar for attack complexity significantly,
offering real-world benefit to defenders while mitigation is
implemented to correct the underlying concern.
We do a fair share of work in medical environments, where applying a
kernel update may involve FDA approval or other acrobatics through
flaming hoops (specifically for implanted med devices and their base
stations), so defensive measures providing blanket coverage for a
class of attacker activity (recon of memory layout in this case) are
very welcome to those consumers.

-- 
Boris Lukashev
Systems Architect
Semper Victus

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

[Linus Torvalds]

On Wed, Oct 4, 2017 at 9:22 AM, Boris Lukashev
<blukashev@sempervictus.com> wrote:
>
> When adding modules from outside the mainline tree (zfs, aufs, scst,
> etc), we would not be able to audit the source, and risk leaking
> sensitive pointers from those components if we dont filter them out
> this way or in a similar programmatic manner.

I call *COMPLETE* bullshit on that argument.

Non-mainlined source code is insecure, and printing some random
address is the *least* of the problems in it.

And the way to make it secure has absolutely nothing to do with printk strings.

Ask somebody about Android camera drivers some day.

Go away. Don't use this specious idiotic argument, all it does is to
make all your other arguments look stupid.

That said, they didn't need much help: ttalking about FDA and medical
equipment as an argument for some particular default value is another
sign that your arguments are UTTER SHIT.

If this is seriously the quality of excuses for this patch-series, I
never ever want to see those patches again.

                Linus

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

[Linus Torvalds]

On Wed, Oct 4, 2017 at 9:22 AM, Boris Lukashev
<blukashev@sempervictus.com> wrote:
>
> When adding modules from outside the mainline tree (zfs, aufs, scst,
> etc), we would not be able to audit the source, and risk leaking
> sensitive pointers from those components if we dont filter them out
> this way or in a similar programmatic manner.

I call *COMPLETE* bullshit on that argument.

Non-mainlined source code is insecure, and printing some random
address is the *least* of the problems in it.

And the way to make it secure has absolutely nothing to do with printk strings.

Ask somebody about Android camera drivers some day.

Go away. Don't use this specious idiotic argument, all it does is to
make all your other arguments look stupid.

That said, they didn't need much help: ttalking about FDA and medical
equipment as an argument for some particular default value is another
sign that your arguments are UTTER SHIT.

If this is seriously the quality of excuses for this patch-series, I
never ever want to see those patches again.

                Linus

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

[Ian Campbell]

On Sun, 2017-10-01 at 11:06 +1100, Tobin C. Harding wrote:
> Suggestion by Ian Campbell to add comments on the threat model being mitigated
> by use of %pa vs %paP etc is not implemented because I do not know the threat
> model (I'm only the janitor). Happy to add them if someone writes them.

Thanks for the CC on this repost, but no need for it for V3 onwards.

Thanks,
Ian.

Re: [kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to the maximum value

[Kees Cook]

On Sat, Sep 30, 2017 at 5:06 PM, Tobin C. Harding <me@tobin.cc> wrote:
> Set the initial value of kptr_restrict to the maximum
> setting rather than the minimum setting, to ensure that
> early boot logging is not leaking information.
>
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> ---
>  lib/vsprintf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 0271223..e009325 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -396,7 +396,7 @@ struct printf_spec {
>  #define FIELD_WIDTH_MAX ((1 << 23) - 1)
>  #define PRECISION_MAX ((1 << 15) - 1)
>
> -int kptr_restrict __read_mostly;
> +int kptr_restrict __read_mostly = 4; /* maximum setting */
>
>  /*
>   * return non-zero if we should cleanse pointers for %p and %pK specifiers.

I share Linus's concern that making this the unconditional default
will break some people. The intention here (as stated in the
changelog) is to cover anything leaked during early boot before sysctl
settings can change this value. That shouldn't break perf (which
should happen after sysctl settings), but it _may_ break debugging of
early boot. I would hope that nothing would be needed there, but if we
want to make this more configurable, we may want to consider a Kconfig
for the default. Perhaps:

-int kptr_restrict __read_mostly;
+int kptr_restrict __read_mostly = CONFIG_KPTR_RESTRICT_DEFAULT;

...

+config KPTR_RESTRICT_DEFAULT
+ bool "Avoid leaking kernel pointers to userspace"
+ default 0
+ range 0 4
+ help
+   This specifies the initial value of the kptr_restrict sysctl, which
+   controls the level of kernel pointers removed from display
+   to userspace. 0 = off, 1 = %pK not visible to non-root, 2 = %pK
+   not visible for any user, 3 = %p also not visible, 4 = physical and
+   resource addresses also not visible.


I'd argue that a default of "1" would be a sensible starting place,
but that can be a separate patch, IMO.

-Kees

-- 
Kees Cook
Pixel Security

Re: [kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to the maximum value

[Kees Cook]

On Sat, Sep 30, 2017 at 5:06 PM, Tobin C. Harding <me@tobin.cc> wrote:
> Set the initial value of kptr_restrict to the maximum
> setting rather than the minimum setting, to ensure that
> early boot logging is not leaking information.
>
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> ---
>  lib/vsprintf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 0271223..e009325 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -396,7 +396,7 @@ struct printf_spec {
>  #define FIELD_WIDTH_MAX ((1 << 23) - 1)
>  #define PRECISION_MAX ((1 << 15) - 1)
>
> -int kptr_restrict __read_mostly;
> +int kptr_restrict __read_mostly = 4; /* maximum setting */
>
>  /*
>   * return non-zero if we should cleanse pointers for %p and %pK specifiers.

I share Linus's concern that making this the unconditional default
will break some people. The intention here (as stated in the
changelog) is to cover anything leaked during early boot before sysctl
settings can change this value. That shouldn't break perf (which
should happen after sysctl settings), but it _may_ break debugging of
early boot. I would hope that nothing would be needed there, but if we
want to make this more configurable, we may want to consider a Kconfig
for the default. Perhaps:

-int kptr_restrict __read_mostly;
+int kptr_restrict __read_mostly = CONFIG_KPTR_RESTRICT_DEFAULT;

...

+config KPTR_RESTRICT_DEFAULT
+ bool "Avoid leaking kernel pointers to userspace"
+ default 0
+ range 0 4
+ help
+   This specifies the initial value of the kptr_restrict sysctl, which
+   controls the level of kernel pointers removed from display
+   to userspace. 0 = off, 1 = %pK not visible to non-root, 2 = %pK
+   not visible for any user, 3 = %p also not visible, 4 = physical and
+   resource addresses also not visible.


I'd argue that a default of "1" would be a sensible starting place,
but that can be a separate patch, IMO.

-Kees

-- 
Kees Cook
Pixel Security

RE: [kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to the maximum value

[Roberts, William C]

> -----Original Message-----
> From: keescook@google.com [mailto:keescook@google.com] On Behalf Of Kees
> Cook
> Sent: Wednesday, October 4, 2017 9:43 AM
> To: Tobin C. Harding <me@tobin.cc>
> Cc: Greg KH <gregkh@linuxfoundation.org>; Petr Mladek <pmladek@suse.com>;
> Joe Perches <joe@perches.com>; Ian Campbell <ijc@hellion.org.uk>; Sergey
> Senozhatsky <sergey.senozhatsky@gmail.com>; kernel-
> hardening@lists.openwall.com; LKML <linux-kernel@vger.kernel.org>; Catalin
> Marinas <catalin.marinas@arm.com>; Will Deacon <will.deacon@arm.com>;
> Steven Rostedt <rostedt@goodmis.org>; Roberts, William C
> <william.c.roberts@intel.com>; Chris Fries <cfries@google.com>; Dave Weinstein
> <olorin@google.com>; Linus Torvalds <torvalds@linux-foundation.org>
> Subject: Re: [kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to
> the maximum value
> 
> On Sat, Sep 30, 2017 at 5:06 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > Set the initial value of kptr_restrict to the maximum setting rather
> > than the minimum setting, to ensure that early boot logging is not
> > leaking information.
> >
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > ---
> >  lib/vsprintf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 0271223..e009325
> > 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -396,7 +396,7 @@ struct printf_spec {  #define FIELD_WIDTH_MAX ((1
> > << 23) - 1)  #define PRECISION_MAX ((1 << 15) - 1)
> >
> > -int kptr_restrict __read_mostly;
> > +int kptr_restrict __read_mostly = 4; /* maximum setting */
> >
> >  /*
> >   * return non-zero if we should cleanse pointers for %p and %pK specifiers.
> 
> I share Linus's concern that making this the unconditional default will break some
> people. The intention here (as stated in the
> changelog) is to cover anything leaked during early boot before sysctl settings can
> change this value. That shouldn't break perf (which should happen after sysctl
> settings), but it _may_ break debugging of early boot. I would hope that nothing
> would be needed there, but if we want to make this more configurable, we may
> want to consider a Kconfig for the default. Perhaps:
> 
> -int kptr_restrict __read_mostly;
> +int kptr_restrict __read_mostly = CONFIG_KPTR_RESTRICT_DEFAULT;
> 
> ...
> 
> +config KPTR_RESTRICT_DEFAULT
> + bool "Avoid leaking kernel pointers to userspace"
> + default 0
> + range 0 4
> + help
> +   This specifies the initial value of the kptr_restrict sysctl, which
> +   controls the level of kernel pointers removed from display
> +   to userspace. 0 = off, 1 = %pK not visible to non-root, 2 = %pK
> +   not visible for any user, 3 = %p also not visible, 4 = physical and
> +   resource addresses also not visible.
> 
> 
> I'd argue that a default of "1" would be a sensible starting place, but that can be a
> separate patch, IMO.
> 

I might be crazy, as often the case, but a  command line option might be useful here
so you can boot the same kernel with Kptr < 4 if you really need to get at any information
hidden by a configure time value of 4.

> -Kees
> 
> --
> Kees Cook
> Pixel Security

RE: [kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to the maximum value

[Roberts, William C]

> -----Original Message-----
> From: keescook@google.com [mailto:keescook@google.com] On Behalf Of Kees
> Cook
> Sent: Wednesday, October 4, 2017 9:43 AM
> To: Tobin C. Harding <me@tobin.cc>
> Cc: Greg KH <gregkh@linuxfoundation.org>; Petr Mladek <pmladek@suse.com>;
> Joe Perches <joe@perches.com>; Ian Campbell <ijc@hellion.org.uk>; Sergey
> Senozhatsky <sergey.senozhatsky@gmail.com>; kernel-
> hardening@lists.openwall.com; LKML <linux-kernel@vger.kernel.org>; Catalin
> Marinas <catalin.marinas@arm.com>; Will Deacon <will.deacon@arm.com>;
> Steven Rostedt <rostedt@goodmis.org>; Roberts, William C
> <william.c.roberts@intel.com>; Chris Fries <cfries@google.com>; Dave Weinstein
> <olorin@google.com>; Linus Torvalds <torvalds@linux-foundation.org>
> Subject: Re: [kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to
> the maximum value
> 
> On Sat, Sep 30, 2017 at 5:06 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > Set the initial value of kptr_restrict to the maximum setting rather
> > than the minimum setting, to ensure that early boot logging is not
> > leaking information.
> >
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > ---
> >  lib/vsprintf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 0271223..e009325
> > 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -396,7 +396,7 @@ struct printf_spec {  #define FIELD_WIDTH_MAX ((1
> > << 23) - 1)  #define PRECISION_MAX ((1 << 15) - 1)
> >
> > -int kptr_restrict __read_mostly;
> > +int kptr_restrict __read_mostly = 4; /* maximum setting */
> >
> >  /*
> >   * return non-zero if we should cleanse pointers for %p and %pK specifiers.
> 
> I share Linus's concern that making this the unconditional default will break some
> people. The intention here (as stated in the
> changelog) is to cover anything leaked during early boot before sysctl settings can
> change this value. That shouldn't break perf (which should happen after sysctl
> settings), but it _may_ break debugging of early boot. I would hope that nothing
> would be needed there, but if we want to make this more configurable, we may
> want to consider a Kconfig for the default. Perhaps:
> 
> -int kptr_restrict __read_mostly;
> +int kptr_restrict __read_mostly = CONFIG_KPTR_RESTRICT_DEFAULT;
> 
> ...
> 
> +config KPTR_RESTRICT_DEFAULT
> + bool "Avoid leaking kernel pointers to userspace"
> + default 0
> + range 0 4
> + help
> +   This specifies the initial value of the kptr_restrict sysctl, which
> +   controls the level of kernel pointers removed from display
> +   to userspace. 0 = off, 1 = %pK not visible to non-root, 2 = %pK
> +   not visible for any user, 3 = %p also not visible, 4 = physical and
> +   resource addresses also not visible.
> 
> 
> I'd argue that a default of "1" would be a sensible starting place, but that can be a
> separate patch, IMO.
> 

I might be crazy, as often the case, but a  command line option might be useful here
so you can boot the same kernel with Kptr < 4 if you really need to get at any information
hidden by a configure time value of 4.

> -Kees
> 
> --
> Kees Cook
> Pixel Security

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

[Steven Rostedt]

On Wed, 4 Oct 2017 09:29:16 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> If this is seriously the quality of excuses for this patch-series, I
> never ever want to see those patches again.

Note, the one you are replying to isn't the author of the patches nor
was he even Cc'd in the patch series.

I wouldn't judge the quality of these patches on some random reply from
someone.

I have no skin in this game, but I don't want the developers of a
series to be judged by random emails in the thread.

-- Steve

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

[Steven Rostedt]

On Wed, 4 Oct 2017 09:29:16 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> If this is seriously the quality of excuses for this patch-series, I
> never ever want to see those patches again.

Note, the one you are replying to isn't the author of the patches nor
was he even Cc'd in the patch series.

I wouldn't judge the quality of these patches on some random reply from
someone.

I have no skin in this game, but I don't want the developers of a
series to be judged by random emails in the thread.

-- Steve

Re: [kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to the maximum value

[Linus Torvalds]

On Wed, Oct 4, 2017 at 9:42 AM, Kees Cook <keescook@chromium.org> wrote:
>
> I'd argue that a default of "1" would be a sensible starting place,
> but that can be a separate patch, IMO.

I agree that '1' is a much saner default for _some_ uses, in that it
still gives root access to /proc file data etc.

However, the sad fact is that kptr_restrict just has bad semantics for
that case too, in that you do want to give root access to /proc files,
but the whole "is the current thread root" is a horrible horrible
test.

Partly it's horrible for the reasons mentioned in the source code (ie
the whole IRQ context thing etc), but that's actually the smallest
reason it's not great: the more fundamental issue is that even for
/proc files, it should use the cred for the file opener, not the
current user.

And for anything *but* /proc files, it's almost always the wrong thing
(ie random printk's aren't generally really associated with any user).

It might almost by mistake do the right thing (ie some kernel printk
that can be triggered by a normal user doing something odd), so it's
not like it always does the wrong thing, but it really is almost
entirely random.

So I seriously dislike kptr_restrict. The whole thing is mis-designed
for very very fundamental reasons.

And no, I also refuse to believe that the fix is "just make it have a
value of 4, and just hide all pointers". Because that will just mean
that people won't be using '%p' at all for debug messages etc, they'll
just use %x or whatever.

So I honestly doubt the value of kptr_restrict. Any *sane* policy
pretty much has to be in the caller, and by thinking about what you
print out. IOW, things like proc_pid_wchan().

                    Linus

Re: [kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to the maximum value

[Linus Torvalds]

On Wed, Oct 4, 2017 at 9:42 AM, Kees Cook <keescook@chromium.org> wrote:
>
> I'd argue that a default of "1" would be a sensible starting place,
> but that can be a separate patch, IMO.

I agree that '1' is a much saner default for _some_ uses, in that it
still gives root access to /proc file data etc.

However, the sad fact is that kptr_restrict just has bad semantics for
that case too, in that you do want to give root access to /proc files,
but the whole "is the current thread root" is a horrible horrible
test.

Partly it's horrible for the reasons mentioned in the source code (ie
the whole IRQ context thing etc), but that's actually the smallest
reason it's not great: the more fundamental issue is that even for
/proc files, it should use the cred for the file opener, not the
current user.

And for anything *but* /proc files, it's almost always the wrong thing
(ie random printk's aren't generally really associated with any user).

It might almost by mistake do the right thing (ie some kernel printk
that can be triggered by a normal user doing something odd), so it's
not like it always does the wrong thing, but it really is almost
entirely random.

So I seriously dislike kptr_restrict. The whole thing is mis-designed
for very very fundamental reasons.

And no, I also refuse to believe that the fix is "just make it have a
value of 4, and just hide all pointers". Because that will just mean
that people won't be using '%p' at all for debug messages etc, they'll
just use %x or whatever.

So I honestly doubt the value of kptr_restrict. Any *sane* policy
pretty much has to be in the caller, and by thinking about what you
print out. IOW, things like proc_pid_wchan().

                    Linus

Re: [kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to the maximum value

[Linus Torvalds]

On Wed, Oct 4, 2017 at 10:08 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So I honestly doubt the value of kptr_restrict. Any *sane* policy
> pretty much has to be in the caller, and by thinking about what you
> print out. IOW, things like proc_pid_wchan().

Looking at /proc/kallsyms is actually a prime example of this.

IOW, the old "open /proc/kallsyms as a normal user, then make it stdin
for some suid-root program that can be fooled to output it probably
works on it.

So kptr_restrict ends up being entirely the wrong thing to do there.

The only value in kptr_restrict ends up being as a complete hack,
where you say "I trust nobody" and make %p almost entirely useless.

And as mentioned, that will just make people use %x instead, or
randomly sprinkle the new "I didn't really mean this" modifiers like
the already discussed pr_debug() case.

So even when kptr_restrict "works", it ends up just fighting itself.
And most of the time it just doesn't work.

                Linus

Re: [kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to the maximum value

[Linus Torvalds]

On Wed, Oct 4, 2017 at 10:08 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So I honestly doubt the value of kptr_restrict. Any *sane* policy
> pretty much has to be in the caller, and by thinking about what you
> print out. IOW, things like proc_pid_wchan().

Looking at /proc/kallsyms is actually a prime example of this.

IOW, the old "open /proc/kallsyms as a normal user, then make it stdin
for some suid-root program that can be fooled to output it probably
works on it.

So kptr_restrict ends up being entirely the wrong thing to do there.

The only value in kptr_restrict ends up being as a complete hack,
where you say "I trust nobody" and make %p almost entirely useless.

And as mentioned, that will just make people use %x instead, or
randomly sprinkle the new "I didn't really mean this" modifiers like
the already discussed pr_debug() case.

So even when kptr_restrict "works", it ends up just fighting itself.
And most of the time it just doesn't work.

                Linus

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

[Jordan Glover]

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

If we knew where those leaks are hiding they will be fixed already. The only thing we knew is that bugs/leaks are there. It's always better to just fix all the code but it isn't realistic. That's what 25 years of kernel development teaches us. Proactive defense is imperfect, obscure, sometimes ugly but it's often best option we have which buy us more time for fixing actual bugs/leaks. Being honest to our users we can't pretend that kernel code is bug/leakfree. They deserve something better.

Yours sincerely,

Jordan

> -------- Original Message --------
> Subject: Re: [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options
> Local Time: October 4, 2017 3:41 PM
> UTC Time: October 4, 2017 3:41 PM
> From: torvalds@linux-foundation.org
> To: Tobin C. Harding <me@tobin.cc>
> Greg KH <gregkh@linuxfoundation.org>, Petr Mladek <pmladek@suse.com>, Joe Perches <joe@perches.com>, Ian Campbell <ijc@hellion.org.uk>, Sergey Senozhatsky <sergey.senozhatsky@gmail.com>, kernel-hardening@lists.openwall.com <kernel-hardening@lists.openwall.com>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will.deacon@arm.com>, Steven Rostedt <rostedt@goodmis.org>, William Roberts <william.c.roberts@intel.com>, Chris Fries <cfries@google.com>, Dave Weinstein <olorin@google.com>
>
> On Sat, Sep 30, 2017 at 5:06 PM, Tobin C. Harding <me@tobin.cc> wrote:
>> lib: vsprintf: default kptr_restrict to the maximum value
>
> So I"m not convinced about this one.
>
> It removes kernel pointers even for root, which is annoying for things
> like perf.
>
> And the only physical pointers we should print out during boot etc are
> things we *need*.
>
> So kptr_restrict is wrong for that, bercause either we potentially
> need those values for debugging ("why does my kernel not boot"), or
> they shouldn"t be printed at all.
>
> And I think _that_ is the real issue. If there are places that leak,
> we should look at those, rather than just say "kptr_restrict".
>
> Linus

[-- Attachment #2: Type: text/html, Size: 2813 bytes --]

Re: [kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to the maximum value

[Jann Horn]

On Wed, Oct 4, 2017 at 7:28 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Oct 4, 2017 at 10:08 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> So I honestly doubt the value of kptr_restrict. Any *sane* policy
>> pretty much has to be in the caller, and by thinking about what you
>> print out. IOW, things like proc_pid_wchan().
>
> Looking at /proc/kallsyms is actually a prime example of this.
>
> IOW, the old "open /proc/kallsyms as a normal user, then make it stdin
> for some suid-root program that can be fooled to output it probably
> works on it.

Actually, /proc/kallsyms uses %pK, which hacks around this issue
by checking for `euid != uid` in addition to the capability check - so this
isn't exploitable through a typical setuid program.

Re: [kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to the maximum value

[Jann Horn]

On Wed, Oct 4, 2017 at 7:28 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Oct 4, 2017 at 10:08 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> So I honestly doubt the value of kptr_restrict. Any *sane* policy
>> pretty much has to be in the caller, and by thinking about what you
>> print out. IOW, things like proc_pid_wchan().
>
> Looking at /proc/kallsyms is actually a prime example of this.
>
> IOW, the old "open /proc/kallsyms as a normal user, then make it stdin
> for some suid-root program that can be fooled to output it probably
> works on it.

Actually, /proc/kallsyms uses %pK, which hacks around this issue
by checking for `euid != uid` in addition to the capability check - so this
isn't exploitable through a typical setuid program.

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

[Linus Torvalds]

On Wed, Oct 4, 2017 at 11:58 AM, Jordan Glover
<Golden_Miller83@protonmail.ch> wrote:
> If we knew where those leaks are hiding they will be fixed already. The only
> thing we knew is that bugs/leaks are there. It's always better to just fix
> all the code but it isn't realistic.

Honestly, what's the difference between setting kptr_restrict to 4 and
just using a sed-script (or maybe some coccinelle) to remove all
existing plain %p users?

One just hides the issue and will make people work around it (likely
on a global level by just undoing it).

The other would *also* make people work around it for when they notice
breakage, but would actually force people to do it on a case-by-case
basis (and thus hopefully _properly_) rather than just setting
kptr_restrict back to 0.

Btw, this is *not* a theoretical argument.

WE HAVE BEEN HERE, DONE THIS!

kptr_restrict goes back to 6+ years ago, and was actually initially
set to a restrictive value. It got undone, exactly because it caused
problems. It's too big of a hammer, and it's too *broken* of a hammer.

And exactly because kptr_restrict was pretty much an "all or nothing"
thing, absolutely *NOTHING* has improved in the 6+ years since it was
introduced.

We have had improvements in our pointer printing that were _not_
related to kptr_restrict, though, See for example commit bb5e5ce545f2
("x86/dumpstack: Remove kernel text addresses from stack dump").

Those have actually been _real_ fixes for leaking things, unlike kptr_restrict.

This is why I maintain that kptr_restrict is bad. It's a badly thought
out interface. It's wrong.

We know it is crap, exactly because we've already been there. The
whole notion of a global switch is seriously mis-designed.

So I really do think that it would be better to just write a script to
get rid of all raw %p users, and then put the ones that are needed
(hopefully very few) back.

It wouldn't require odd new magic sequences to override "I actually
_do_ want %p".

              Linus

Re: [kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to the maximum value

[Linus Torvalds]

On Wed, Oct 4, 2017 at 12:13 PM, Jann Horn <jannh@google.com> wrote:
>
> Actually, /proc/kallsyms uses %pK, which hacks around this issue
> by checking for `euid != uid` in addition to the capability check - so this
> isn't exploitable through a typical setuid program.

Fair enough, you'd have to be a pretty broken suid program to have set
uid to euid before reading some untrusted file descriptor.

I could still imagine happening (hey, the X server used to sendmsg
file descriptors back and forth), but hopefully it's not really
realistic.

                  Linus

Re: [kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to the maximum value

[Linus Torvalds]

On Wed, Oct 4, 2017 at 12:13 PM, Jann Horn <jannh@google.com> wrote:
>
> Actually, /proc/kallsyms uses %pK, which hacks around this issue
> by checking for `euid != uid` in addition to the capability check - so this
> isn't exploitable through a typical setuid program.

Fair enough, you'd have to be a pretty broken suid program to have set
uid to euid before reading some untrusted file descriptor.

I could still imagine happening (hey, the X server used to sendmsg
file descriptors back and forth), but hopefully it's not really
realistic.

                  Linus

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

[Roberts, William C]

> -----Original Message-----
> From: linus971@gmail.com [mailto:linus971@gmail.com] On Behalf Of Linus
> Torvalds
> Sent: Wednesday, October 4, 2017 12:19 PM
> To: Jordan Glover <Golden_Miller83@protonmail.ch>
> Cc: Tobin C. Harding <me@tobin.cc>; Greg KH <gregkh@linuxfoundation.org>;
> Petr Mladek <pmladek@suse.com>; Joe Perches <joe@perches.com>; Ian
> Campbell <ijc@hellion.org.uk>; Sergey Senozhatsky
> <sergey.senozhatsky@gmail.com>; kernel-hardening@lists.openwall.com;
> Catalin Marinas <catalin.marinas@arm.com>; Will Deacon
> <will.deacon@arm.com>; Steven Rostedt <rostedt@goodmis.org>; Roberts,
> William C <william.c.roberts@intel.com>; Chris Fries <cfries@google.com>; Dave
> Weinstein <olorin@google.com>
> Subject: Re: [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter
> options
> 
> On Wed, Oct 4, 2017 at 11:58 AM, Jordan Glover
> <Golden_Miller83@protonmail.ch> wrote:
> > If we knew where those leaks are hiding they will be fixed already.
> > The only thing we knew is that bugs/leaks are there. It's always
> > better to just fix all the code but it isn't realistic.
> 
> Honestly, what's the difference between setting kptr_restrict to 4 and just using
> a sed-script (or maybe some coccinelle) to remove all existing plain %p users?

I can already see the hate filled irate response I'll get to this statement, thankfully I will
be away not caring.

The kernel proper is in a place where it can attempt to defend itself against stupidity, either in
tree or out of tree. Will it stop everything? Obviously not, like you said %x or disabling.

I agree with you 100% kptr restrict is odd, and I don't think anyone should have had to opt in to be
cleansed via kptr_restrict value via %pK. Opt-in never works. One nice thing now, is that checkpatch
has checking of %p usages and warns.
 
As far as broken things, I can't comment on desktop systems where I think it's harder to make that claim.
I see value in embedded systems where I am shipping the whole image, So I know when/what will
break.

If this was in-tree, Android would be setting this to 4 immediately FWIW.

> 
> One just hides the issue and will make people work around it (likely on a global
> level by just undoing it).
> 
> The other would *also* make people work around it for when they notice
> breakage, but would actually force people to do it on a case-by-case basis (and
> thus hopefully _properly_) rather than just setting kptr_restrict back to 0.
> 
> Btw, this is *not* a theoretical argument.
> 
> WE HAVE BEEN HERE, DONE THIS!
> 
> kptr_restrict goes back to 6+ years ago, and was actually initially set to a
> restrictive value. It got undone, exactly because it caused problems. It's too big of
> a hammer, and it's too *broken* of a hammer.
> 
> And exactly because kptr_restrict was pretty much an "all or nothing"
> thing, absolutely *NOTHING* has improved in the 6+ years since it was
> introduced.
> 
> We have had improvements in our pointer printing that were _not_ related to
> kptr_restrict, though, See for example commit bb5e5ce545f2
> ("x86/dumpstack: Remove kernel text addresses from stack dump").
> 
> Those have actually been _real_ fixes for leaking things, unlike kptr_restrict.
> 
> This is why I maintain that kptr_restrict is bad. It's a badly thought out interface.
> It's wrong.
> 
> We know it is crap, exactly because we've already been there. The whole notion
> of a global switch is seriously mis-designed.
> 
> So I really do think that it would be better to just write a script to get rid of all raw
> %p users, and then put the ones that are needed (hopefully very few) back.
> 
> It wouldn't require odd new magic sequences to override "I actually _do_ want
> %p".
> 
>               Linus

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

[Daniel Micay]

On Wed, 2017-10-04 at 21:58 +0000, Roberts, William C wrote:
> > -----Original Message-----
> > From: linus971@gmail.com [mailto:linus971@gmail.com] On Behalf Of
> > Linus
> > Torvalds
> > Sent: Wednesday, October 4, 2017 12:19 PM
> > To: Jordan Glover <Golden_Miller83@protonmail.ch>
> > Cc: Tobin C. Harding <me@tobin.cc>; Greg KH <gregkh@linuxfoundation.
> > org>;
> > Petr Mladek <pmladek@suse.com>; Joe Perches <joe@perches.com>; Ian
> > Campbell <ijc@hellion.org.uk>; Sergey Senozhatsky
> > <sergey.senozhatsky@gmail.com>; kernel-hardening@lists.openwall.com;
> > Catalin Marinas <catalin.marinas@arm.com>; Will Deacon
> > <will.deacon@arm.com>; Steven Rostedt <rostedt@goodmis.org>;
> > Roberts,
> > William C <william.c.roberts@intel.com>; Chris Fries <cfries@google.
> > com>; Dave
> > Weinstein <olorin@google.com>
> > Subject: Re: [kernel-hardening] [RFC V2 0/6] add more kernel pointer
> > filter
> > options
> > 
> > On Wed, Oct 4, 2017 at 11:58 AM, Jordan Glover
> > <Golden_Miller83@protonmail.ch> wrote:
> > > If we knew where those leaks are hiding they will be fixed
> > > already.
> > > The only thing we knew is that bugs/leaks are there. It's always
> > > better to just fix all the code but it isn't realistic.
> > 
> > Honestly, what's the difference between setting kptr_restrict to 4
> > and just using
> > a sed-script (or maybe some coccinelle) to remove all existing plain
> > %p users?
> 
> I can already see the hate filled irate response I'll get to this
> statement, thankfully I will
> be away not caring.
> 
> The kernel proper is in a place where it can attempt to defend itself
> against stupidity, either in
> tree or out of tree. Will it stop everything? Obviously not, like you
> said %x or disabling.
> 
> I agree with you 100% kptr restrict is odd, and I don't think anyone
> should have had to opt in to be
> cleansed via kptr_restrict value via %pK. Opt-in never works. One nice
> thing now, is that checkpatch
> has checking of %p usages and warns.
>  
> As far as broken things, I can't comment on desktop systems where I
> think it's harder to make that claim.
> I see value in embedded systems where I am shipping the whole image,
> So I know when/what will
> break.
> 
> If this was in-tree, Android would be setting this to 4 immediately
> FWIW.

It's already shipping on Pixel phones in production as of Android 8.0,
but they didn't pull it into the common kernel LTS branches yet:

https://android.googlesource.com/kernel/msm.git/+/00e1c16fbac282b99a769b939dd215cbdc775ecb%5E%21/#F0

Google backports many KSPP changes to their LTS kernels and they aren't
limiting that to changes that are already upstream. It can all just be
dropped without impacting app compatibility so it doesn't matter if the
finalized upstream approach ends up being different.

It's a similar situation to perf_event_paranoid=3 which is mandatory for
all Android devices since the August 2016 security patch despite being
rejected upstream. There's a test checking for it and integration into
the profiling tools which know how to turn it back down via the USB
debugging shell (ADB). Debian had perf_event_paranoid=3 even earlier so
it's on the vast majority of Linux devices already.

They just end up being permanent downstream patches if they're rejected
without providing an alternative on par with it.

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

[Linus Torvalds]

On Wed, Oct 4, 2017 at 2:58 PM, Roberts, William C
<william.c.roberts@intel.com> wrote:
>
> I agree with you 100% kptr restrict is odd, and I don't think anyone should have had to opt in to be
> cleansed via kptr_restrict value via %pK. Opt-in never works. One nice thing now, is that checkpatch
> has checking of %p usages and warns.

Yeah, the checkpatch thing may help for future patches.

> As far as broken things, I can't comment on desktop systems where I think it's harder to make that claim.
> I see value in embedded systems where I am shipping the whole image, So I know when/what will
> break.
>
> If this was in-tree, Android would be setting this to 4 immediately FWIW.

Does android set it to 2 right now?

But even if it does, my point is that we've had this thing for 6+
years, and it really hasn't helped fix our code much at all.

In fact, I think the opposite is true. I think it *hurts*. It hurts
exactly because it's a hack, and it makes people not bother to fix the
real problems.

I think we'd be better off just fixing code.

One thing you can do today is just look through your 'dmesg' for what
looks like kernel addresses (if virtual addresses is what you're
interested in - obviously physical addresses will look different). On
x86-64, for example, something like this:

    dmesg | egrep 'ffff[0-9a-f]{12}'

might be interesting to look at. It shows (for me) that we show the
percpu address mapping, for example. That certainly sounds interesting
to a potential attacker, and I suspect Tejun isn't really interested
in that information any more. Added to Cc.

It also shows

    software IO TLB [mem PA-PB] (64MB) mapped at [VA-VB]

for example.  And THIS IS IMPORTANT! The physical address is shown
with "%#010llx".

See what I'm saying? The kptr_restrict games are just that: games and
security theater. If you actually care about things like physical
addresses, you don't say "let's hide %pa". Because that's not how the
real world works. I found one case that would entirely have missed
with the very first trivial grep I did.

So I'm claiming that anybody who says "it's too hard to fix it for
real, let's just paper over it in vsprintf.c" is fundamentally going
to miss things like this. I agree that it might be painful to try to
figure out where the problems are, but somebody like google probably
has a lot of dmesg output, and it should not be that hard to do the
above kinds of "let's just see what leaks, and FIX it".

                   Linus

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

[Linus Torvalds]

On Wed, Oct 4, 2017 at 4:52 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> It also shows
>
>     software IO TLB [mem PA-PB] (64MB) mapped at [VA-VB]
>
> for example.  And THIS IS IMPORTANT! The physical address is shown
> with "%#010llx".

Oh, and I added Tejun for the percpu printout, but didn't add Bjorn
for this one.

Like the percpu one, I suspect that this pr_info() isn't really
important enough to stay, and while it doesn't sound nearly as
interesting as the percpu virtual address to an attacker, it probably
isn't important enough to leak kernel (and hardware) data for.

Bjorn?

I suspect we could try to make people aware of these kinds of things.
The nice zero-day robots etc probably aren't wondeful for this (since
they'll have fairly limited hardware), but maybe we could have a
script that makes it easy to just say

 "does dmesg seem to contain interesting numbers that really shouldn't
be leaked?"

That "egrep 'ffff[0-9a-f]{12}" is certainly not very good - not only
is it 64-bit specific, but it triggers, for example, on a "mask:
0xffffffffffffffff" from the clocksource code. But Something
*slightly* smarter could be something we could probably encourage
people to run occasionally.

... and if nothing else, maybe it makes developers more aware of the
"I shouldn't be leaking kernel data in dmesg" issue in general.

More so than just special-casing %p handling in odd ways and ignoring
all the other ways we can leak.

For example: x86 removed the raw stack dump last year (commit
0ee1dd9f5e7e: "x86/dumpstack: Remove raw stack dump"), because we
decided that triggering a BUG_ON() obviously does need to expose some
kernel data, but the stack wasn't worth it (and definitely tends to
contain nasty information). ARM has not (see "dump_mem()". Again,
that's not using %p. It uses to use %lx.

So is %p really the problem?

               Linus

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

[Daniel Micay]

On Wed, 2017-10-04 at 16:52 -0700, Linus Torvalds wrote:
> On Wed, Oct 4, 2017 at 2:58 PM, Roberts, William C
> <william.c.roberts@intel.com> wrote:
> > 
> > I agree with you 100% kptr restrict is odd, and I don't think anyone
> > should have had to opt in to be
> > cleansed via kptr_restrict value via %pK. Opt-in never works. One
> > nice thing now, is that checkpatch
> > has checking of %p usages and warns.
> 
> Yeah, the checkpatch thing may help for future patches.
> 
> > As far as broken things, I can't comment on desktop systems where I
> > think it's harder to make that claim.
> > I see value in embedded systems where I am shipping the whole image,
> > So I know when/what will
> > break.
> > 
> > If this was in-tree, Android would be setting this to 4 immediately
> > FWIW.
> 
> Does android set it to 2 right now?

Yes, as the universal baseline.

On Google Pixels it's set to this 4 level since August (Android 8.0)
which indicates they plan on moving to that universally.

They only allow dmesg access for core system services so I think their
concern is with formatted strings leaking it elsewhere, not to dmesg.

These are the only services they allow to read dmesg:

private/system_server.te:  allow system_server kernel:system syslog_read;
public/dumpstate.te:allow dumpstate kernel:system syslog_read;
public/init.te:allow init kernel:system syslog_read;
public/logd.te:allow logd kernel:system syslog_read;
public/recovery.te:  allow recovery kernel:system syslog_read;

logd doesn't read it in production builds, but even when it does in
engineering builds it only gives out access to privileged apps with
READ_LOGS which isn't something a third party app can obtain.

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

[Kees Cook]

On Wed, Oct 4, 2017 at 5:29 PM, Daniel Micay <danielmicay@gmail.com> wrote:
> On Wed, 2017-10-04 at 16:52 -0700, Linus Torvalds wrote:
>> On Wed, Oct 4, 2017 at 2:58 PM, Roberts, William C
>> <william.c.roberts@intel.com> wrote:
>> >
>> > I agree with you 100% kptr restrict is odd, and I don't think anyone
>> > should have had to opt in to be
>> > cleansed via kptr_restrict value via %pK. Opt-in never works. One
>> > nice thing now, is that checkpatch
>> > has checking of %p usages and warns.
>>
>> Yeah, the checkpatch thing may help for future patches.
>>
>> > As far as broken things, I can't comment on desktop systems where I
>> > think it's harder to make that claim.
>> > I see value in embedded systems where I am shipping the whole image,
>> > So I know when/what will
>> > break.
>> >
>> > If this was in-tree, Android would be setting this to 4 immediately
>> > FWIW.
>>
>> Does android set it to 2 right now?
>
> Yes, as the universal baseline.
>
> On Google Pixels it's set to this 4 level since August (Android 8.0)
> which indicates they plan on moving to that universally.

Yup, which is what triggered working to upstream a solution. (As
mentioned in the changelogs, this series has gone through a number of
hands including Intel and Google folks.)

> They only allow dmesg access for core system services so I think their
> concern is with formatted strings leaking it elsewhere, not to dmesg.

One of the paths is via seq_file and it's many outputs in procfs,
debugfs, etc etc. The %x issue exists there too, but it's relatively
rare compared to %p and family.

Just looking at the %p case, an alternative might be to follow
grsecurity's method here, which is to examine where v*printf is
writing. They perform the censorship in the case of seq_file buffers,
or memory destined for userspace.

-Kees

-- 
Kees Cook
Pixel Security

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

[Tobin C. Harding]

On Wed, Oct 04, 2017 at 04:52:58PM -0700, Linus Torvalds wrote:
> On Wed, Oct 4, 2017 at 2:58 PM, Roberts, William C
> <william.c.roberts@intel.com> wrote:
> >
> > I agree with you 100% kptr restrict is odd, and I don't think anyone should have had to opt in to be
> > cleansed via kptr_restrict value via %pK. Opt-in never works. One nice thing now, is that checkpatch
> > has checking of %p usages and warns.
> 
> Yeah, the checkpatch thing may help for future patches.
> 
> > As far as broken things, I can't comment on desktop systems where I think it's harder to make that claim.
> > I see value in embedded systems where I am shipping the whole image, So I know when/what will
> > break.
> >
> > If this was in-tree, Android would be setting this to 4 immediately FWIW.
> 
> Does android set it to 2 right now?
> 
> But even if it does, my point is that we've had this thing for 6+
> years, and it really hasn't helped fix our code much at all.
> 
> In fact, I think the opposite is true. I think it *hurts*. It hurts
> exactly because it's a hack, and it makes people not bother to fix the
> real problems.
> 
> I think we'd be better off just fixing code.
> 
> One thing you can do today is just look through your 'dmesg' for what
> looks like kernel addresses (if virtual addresses is what you're
> interested in - obviously physical addresses will look different). On
> x86-64, for example, something like this:
> 
>     dmesg | egrep 'ffff[0-9a-f]{12}'
> 
> might be interesting to look at. It shows (for me) that we show the
> percpu address mapping, for example. That certainly sounds interesting
> to a potential attacker, and I suspect Tejun isn't really interested
> in that information any more. Added to Cc.
> 
> It also shows
> 
>     software IO TLB [mem PA-PB] (64MB) mapped at [VA-VB]
> 
> for example.  And THIS IS IMPORTANT! The physical address is shown
> with "%#010llx".
> 
> See what I'm saying? The kptr_restrict games are just that: games and
> security theater. If you actually care about things like physical
> addresses, you don't say "let's hide %pa". Because that's not how the
> real world works. I found one case that would entirely have missed
> with the very first trivial grep I did.
> 
> So I'm claiming that anybody who says "it's too hard to fix it for
> real, let's just paper over it in vsprintf.c" is fundamentally going
> to miss things like this. I agree that it might be painful to try to
> figure out where the problems are, but somebody like google probably
> has a lot of dmesg output, and it should not be that hard to do the
> above kinds of "let's just see what leaks, and FIX it".

This sounds like just the job for an upcoming kernel hacker, with a lot of time
and not much experience, to do something laborious that no one else wants to do
and learn a bunch about the kernel.

So what if we drop this patch set and

1. Find and fix every place in the kernel that leaks addresses.
2. Verify that checkpatch.pl warns for all potential future leakage.
2. Add a script to check dmesg output for future hardening.

I'm super keen to work. I would have to get everyone to dump their ideas/demands 
onto me if we are to get this fixed fully.

thanks,
Tobin.

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

[Linus Torvalds]

On Wed, Oct 4, 2017 at 7:19 PM, Tobin C. Harding <me@tobin.cc> wrote:
>
> This sounds like just the job for an upcoming kernel hacker, with a lot of time
> and not much experience, to do something laborious that no one else wants to do
> and learn a bunch about the kernel.

Heh.

We _have_ been doing it, but we definitely have only done it on a
random case-by-cased basis as something comes up.

So what would be great is to have something actively looking for these
things - and by "something" I mean mostly scripting.

And yes, as Kees and Daniel mentioned, it's definitely not just dmesg.
In fact, the primary things tend to be /proc and /sys, not dmesg
itself.

Another example of this would be commit 31b0b385f69d ("nf_conntrack:
avoid kernel pointer value leak in slab name"), where the fact that
the slab name had a pointer in it leaked it in the _filenames_ in
/sys, because we export slab statistics under /sys/kernel/slab/. And
each file was readable only by root, but the file *names* were
readable by everybody.

(That separate slab thing then got removed entirely later, so that
slab no longer exists at all, but it's an odd example of how the leaks
can be in the meta-data, not in the file contents themselves.

But again, I think that kind of leak would have been fairly obvious if
we just had some scripting that looked for interfaces that the kernel
exposed, and looked for that hex pattern.

In fact, that was how I noticved that one: not a grep, but an entirely
unrelated "find /sys..." that made me go "why are there kernel
addresses in there.."

So we've found them occasionally simply by mistake - and it would be
great if we found them because we actively went _looking_ for them.

> So what if we drop this patch set and
>
> 1. Find and fix every place in the kernel that leaks addresses.
> 2. Verify that checkpatch.pl warns for all potential future leakage.
> 2. Add a script to check dmesg output for future hardening.

Very ambitious (particularly that "*every* place"), but I certainly
think it would be the better end result, yes.

And I do think we would be really well off if we aimed for simply
getting rid of all the variations of %p that output hex addresses
entirely.

Don't get me wrong: I think the various _fancy_ versions of "%pXYZ"
are great, ie things like "%pS" to show the symbol name etc.

But plain %p has definitely been a problem, and I don't think %pa is
great either. So aiming to get rid of them entirely is probably a good
idea.

So I am not opposed to hobbling %p per se. It's literally just the
'kptr_restrict' model I detest. I'd rather get rid of %p (and %pa)
_entirely_ and make valid users have to work a bit extra for it,
because I think the 6+ years of kptr_restrict has shown that model of
"let people who care just enable it again" to simply not work.

                   Linus

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

[Kees Cook]

On Wed, Oct 4, 2017 at 8:10 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> But plain %p has definitely been a problem, and I don't think %pa is
> great either. So aiming to get rid of them entirely is probably a good
> idea.

As you've hinted, doing this will make %x use go up, of course, so I
remain convinced that we need some kind of at-runtime evaluation of
the arguments coming into v*sprintf(). If we removed the raw %p format
string, we'd want to stop %x from being used on memory addresses too.

-Kees

-- 
Kees Cook
Pixel Security

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

[Bjorn Helgaas]

On Wed, Oct 4, 2017 at 7:09 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Oct 4, 2017 at 4:52 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> It also shows
>>
>>     software IO TLB [mem PA-PB] (64MB) mapped at [VA-VB]
>>
>> for example.  And THIS IS IMPORTANT! The physical address is shown
>> with "%#010llx".
>
> Oh, and I added Tejun for the percpu printout, but didn't add Bjorn
> for this one.
>
> Like the percpu one, I suspect that this pr_info() isn't really
> important enough to stay, and while it doesn't sound nearly as
> interesting as the percpu virtual address to an attacker, it probably
> isn't important enough to leak kernel (and hardware) data for.
>
> Bjorn?

My only interest in the "software IO TLB" message was to make the
style match other similar things; I personally don't care whether this
message exists at all.

I haven't seen the whole series, but I do hope %pR remains useful.  We
use it often to analyze PCI BAR assignment issues.

Bjorn

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

[Roberts, William C]

> -----Original Message-----
> From: linus971@gmail.com [mailto:linus971@gmail.com] On Behalf Of Linus
> Torvalds
> Sent: Wednesday, October 4, 2017 8:11 PM
> To: Tobin C. Harding <me@tobin.cc>
> Cc: Roberts, William C <william.c.roberts@intel.com>; Tejun Heo
> <tj@kernel.org>; Jordan Glover <Golden_Miller83@protonmail.ch>; Greg KH
> <gregkh@linuxfoundation.org>; Petr Mladek <pmladek@suse.com>; Joe
> Perches <joe@perches.com>; Ian Campbell <ijc@hellion.org.uk>; Sergey
> Senozhatsky <sergey.senozhatsky@gmail.com>; kernel-
> hardening@lists.openwall.com; Catalin Marinas <catalin.marinas@arm.com>; Will
> Deacon <will.deacon@arm.com>; Steven Rostedt <rostedt@goodmis.org>; Chris
> Fries <cfries@google.com>; Dave Weinstein <olorin@google.com>
> Subject: Re: [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter
> options
> 
> On Wed, Oct 4, 2017 at 7:19 PM, Tobin C. Harding <me@tobin.cc> wrote:
> >
> > This sounds like just the job for an upcoming kernel hacker, with a
> > lot of time and not much experience, to do something laborious that no
> > one else wants to do and learn a bunch about the kernel.
> 
> Heh.
> 
> We _have_ been doing it, but we definitely have only done it on a random case-
> by-cased basis as something comes up.
> 
> So what would be great is to have something actively looking for these things -
> and by "something" I mean mostly scripting.
> 
> And yes, as Kees and Daniel mentioned, it's definitely not just dmesg.
> In fact, the primary things tend to be /proc and /sys, not dmesg itself.
> 
> Another example of this would be commit 31b0b385f69d ("nf_conntrack:
> avoid kernel pointer value leak in slab name"), where the fact that the slab name
> had a pointer in it leaked it in the _filenames_ in /sys, because we export slab
> statistics under /sys/kernel/slab/. And each file was readable only by root, but
> the file *names* were readable by everybody.
> 
> (That separate slab thing then got removed entirely later, so that slab no longer
> exists at all, but it's an odd example of how the leaks can be in the meta-data, not
> in the file contents themselves.
> 
> But again, I think that kind of leak would have been fairly obvious if we just had
> some scripting that looked for interfaces that the kernel exposed, and looked for
> that hex pattern.
> 
> In fact, that was how I noticved that one: not a grep, but an entirely unrelated
> "find /sys..." that made me go "why are there kernel addresses in there.."
> 
> So we've found them occasionally simply by mistake - and it would be great if we
> found them because we actively went _looking_ for them.
> 
> > So what if we drop this patch set and
> >
> > 1. Find and fix every place in the kernel that leaks addresses.
> > 2. Verify that checkpatch.pl warns for all potential future leakage.
> > 2. Add a script to check dmesg output for future hardening.
> 
> Very ambitious (particularly that "*every* place"), but I certainly think it would be
> the better end result, yes.
> 
> And I do think we would be really well off if we aimed for simply getting rid of all
> the variations of %p that output hex addresses entirely.
> 
> Don't get me wrong: I think the various _fancy_ versions of "%pXYZ"
> are great, ie things like "%pS" to show the symbol name etc.
> 
> But plain %p has definitely been a problem, and I don't think %pa is great either.
> So aiming to get rid of them entirely is probably a good idea.
> 
> So I am not opposed to hobbling %p per se. It's literally just the 'kptr_restrict'
> model I detest. I'd rather get rid of %p (and %pa) _entirely_ and make valid users
> have to work a bit extra for it, because I think the 6+ years of kptr_restrict has
> shown that model of "let people who care just enable it again" to simply not
> work.

I think one of the reasons it didn't work was that it was opt-in via %pK. It should have
always been filter unless one opts-out, at that point, you shot yourself and I could
care less about those. Killing %p just means there will be a lot of %lu users (as an example)
which means they would be harder to audit. I'm sure, somewhere, someone is doing that
already.

> 
>                    Linus

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

[Linus Torvalds]

On Thu, Oct 5, 2017 at 8:12 AM, Roberts, William C
<william.c.roberts@intel.com> wrote:
>
> I think one of the reasons it didn't work was that it was opt-in via %pK.

I agree that the opt-in doesn't work, but I really don't think a
config option really works either, and in the case of '%p',
unconditionally doing it everywhere wasn't really an option.

The "check if root' only works in synchronous contexts (and not well
there either, since it's often not about the current task, but about
the current file descriptor etc).

The "always print 0" model obviously does work, but then you really
have to have a strategy for moving away from %p users entirely.

And I'm willing to do that, but then I think we'd need to

 (a) get rid of kptr_restrict entirely, because as long as it exists
the "fix" for any problem is always going to be "just boot with this
disabled".

     And no, that "just boot with it disabled" is not an acceptable
model. It's what we've been doing exactly because it's not acceptable.

     We already don't get great bug reports. If we now start getting
bug reports and tell users "recreate this with 'kptr_enabled' on the
kernel command line" because some debug message didn't give proper
data, that will just result in us getting even less bug reports.

 (b) have some plan in place for people who end up just using %x instead.

Part of "b" might be to not print out zeroes, but something more useful.

Because *most* %p users are for debugging, I think. There tends to be
at least a couple of places:

 - debug messages that want to indicate which object we're talking
about. The actual pointer is not important, but the pointer to the
device descriptor or whatever is used as a "unique ID" for the case
where there might be multiple.

 - meaningful hardware addresses (ie the pointer is some mmio pointer).

At least the first case could possibly continue to use a '%p' that
simply hashes the actual pointer value with some hash or something
like that. It's not the pointer itself that is important, it's just
the "identity" of the socket or device or whatever.

Of course, "just hash it" isn't good enough if we're just hiding the
(very limited) kernel address space randomization, but mixing in a
per-boot random value might be sufficient.

So if instead of just printing zeroes, we'd still print something that
is a useful _identity_, maybe a lot of existing users of '%p' would
still be ok with it.

Which would limit that (b) issue.

What do people think? Literally something like

  diff --git a/lib/vsprintf.c b/lib/vsprintf.c
  index 86c3385b9eb3..a995227a1918 100644
  --- a/lib/vsprintf.c
  +++ b/lib/vsprintf.c
  @@ -1708,7 +1708,8 @@ static noinline_for_stack
   char *pointer(const char *fmt, char *buf, char *end, void *ptr,
                 struct printf_spec spec)
   {
  -        const int default_width = 2 * sizeof(void *);
  +        const int default_width = 8;
  +        unsigned int hashval;

           if (!ptr && *fmt != 'K') {
                   /*
  @@ -1865,7 +1866,12 @@ char *pointer(const char *fmt, char *buf,
char *end, void *ptr,
           }
           spec.base = 16;

  -        return number(buf, end, (unsigned long) ptr, spec);
  +        hashval = hash_three_words(
  +                (unsigned long)ptr,
  +                (unsigned long)ptr >> 16 >> 16,
  +                boot_time_random_int);
  +
  +        return number(buf, end, hashval, spec);
   }

   /*

which just means that a pointer that falls through to the hex version
will always just be shown as a 32-bit hash. It will hash to the same
thing for any particular boot, so you'll recognize values you already
know, but it should be pretty hard to use it for an attack that needs
to figure out the pointer.

Yes, if you want the "real" pointer, you'd need to switch to '%x', but
that is at least _potentially_ a much smaller set of cases now.

Hmm?

I have *not* tried the above out in practice. But something like that
seems at least potentially realistic to me as a way forward.

Comments?

                     Linus

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

[Dave Weinstein]

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

> Does android set it to 2 right now?

Yes. And as of Android 8.0, it will attempt to set it to 4, and step back
to 2 if the higher restriction values are not supported (
https://android.googlesource.com/platform/system/core/+/44f7e4f42190fdb5309b818d5acc0ff6b0f87249%5E%21/#F0
)

>     We already don't get great bug reports. If we now start getting

>bug reports and tell users "recreate this with 'kptr_enabled' on the

>kernel command line" because some debug message didn't give proper

>data, that will just result in us getting even less bug reports.

This is why the original Android patch set whitelists the stack traces --
it was a deliberate choice to make sure that kernel bug reports were
generally meaningful by default.

>Honestly, what's the difference between setting kptr_restrict to 4 and

>just using a sed-script (or maybe some coccinelle) to remove all

>existing plain %p users?

The primary benefit from my perspective is that the userland component can
establish a general expectation of the level of information disclosure that
depends on a single feature being implemented, rather than an arbitrarily
large number of point fixes across multiple kernel versions.

--Dave

-- 
Dave Weinstein
Android SDL

[-- Attachment #2: Type: text/html, Size: 4212 bytes --]

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

[Djalal Harouni]

Hi Kees,

On Thu, Oct 5, 2017 at 1:35 AM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Oct 4, 2017 at 5:29 PM, Daniel Micay <danielmicay@gmail.com> wrote:
>> On Wed, 2017-10-04 at 16:52 -0700, Linus Torvalds wrote:
>>> On Wed, Oct 4, 2017 at 2:58 PM, Roberts, William C
>>> <william.c.roberts@intel.com> wrote:
>>> >
>>> > I agree with you 100% kptr restrict is odd, and I don't think anyone
>>> > should have had to opt in to be
>>> > cleansed via kptr_restrict value via %pK. Opt-in never works. One
>>> > nice thing now, is that checkpatch
>>> > has checking of %p usages and warns.
>>>
>>> Yeah, the checkpatch thing may help for future patches.
>>>
>>> > As far as broken things, I can't comment on desktop systems where I
>>> > think it's harder to make that claim.
>>> > I see value in embedded systems where I am shipping the whole image,
>>> > So I know when/what will
>>> > break.
>>> >
>>> > If this was in-tree, Android would be setting this to 4 immediately
>>> > FWIW.
>>>
>>> Does android set it to 2 right now?
>>
>> Yes, as the universal baseline.
>>
>> On Google Pixels it's set to this 4 level since August (Android 8.0)
>> which indicates they plan on moving to that universally.
>
> Yup, which is what triggered working to upstream a solution. (As
> mentioned in the changelogs, this series has gone through a number of
> hands including Intel and Google folks.)
>
>> They only allow dmesg access for core system services so I think their
>> concern is with formatted strings leaking it elsewhere, not to dmesg.
>
> One of the paths is via seq_file and it's many outputs in procfs,
> debugfs, etc etc. The %x issue exists there too, but it's relatively
> rare compared to %p and family.

I think the kptr_restrict solution is not enough since you will have
to continue to track all call sites.

Anyway for procfs as you know Kees, we have patches to modernize it
based on Andy Lutomirski suggestions.
There is this branch [1] that allows only to access pids in
/proc/<pids>/ and block the rest that is kernel data.

It can be used by any sandbox solution to allow glibc and others work
on /proc/self/ access and block the rest, or for other embedded
systems sandboxes.
I updated the patches to post them but there is a bug with some other
kernel changes + dracut that I am still tracking.

[1] https://github.com/legionus/linux/commits/pidfs-v4

-- 
tixxdz

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

[Theodore Ts'o]

On Thu, Oct 05, 2017 at 09:19:17AM -0700, Linus Torvalds wrote:
>  - meaningful hardware addresses (ie the pointer is some mmio pointer).
> 
> At least the first case could possibly continue to use a '%p' that
> simply hashes the actual pointer value with some hash or something
> like that. It's not the pointer itself that is important, it's just
> the "identity" of the socket or device or whatever.
> 
> Of course, "just hash it" isn't good enough if we're just hiding the
> (very limited) kernel address space randomization, but mixing in a
> per-boot random value might be sufficient.

Maybe this is overkill, but what about *encrypting* the pointer using
fixed, per-boot random key?  It would have to be a block crypto
algorithm, like Tea/XXTea/XXTEA or Speck.  That has all of the
benefits of hashing, plus if the key is available (only to privileged
users, of course), then it would be possible to get the pointer for
debugging purposes.

						- Ted

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

[Linus Torvalds]

On Sat, Oct 7, 2017 at 4:44 PM, Theodore Ts'o <tytso@mit.edu> wrote:
>
> Maybe this is overkill, but what about *encrypting* the pointer using
> fixed, per-boot random key?  It would have to be a block crypto
> algorithm, like Tea/XXTea/XXTEA or Speck.  That has all of the
> benefits of hashing, plus if the key is available (only to privileged
> users, of course), then it would be possible to get the pointer for
> debugging purposes.

I doubt it's going to be used enough to be worth it.

So I'd rather just make it a hash that gives you that identity for
people who want it, and then people who actually need the address
would need to decide whether they _really_ need it in the first place,
or whether they would use %lx instead together with stricty security
rules.

I did test the patch I sent out (using jhash - so not a cryptographic
hash, and with a "random value" that was just a constant) just to see
that it doesn't break anything obvious. As expected, it did break
kernel profiling (which uses /proc/kallsyms), and it obviously made
the kernel pointers I'd already found in dmesg be just garbage, but it
didn't seem insurmountable. More like a "with a couple of fixup cases,
it might all work fine".

So I'd be willing to try it for the next merge window, and see if it
causes unexpected problems..

                Linus

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

[Roberts, William C]

> -----Original Message-----
> From: Tobin C. Harding [mailto:me@tobin.cc]
> Sent: Wednesday, October 4, 2017 7:19 PM
> To: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Roberts, William C <william.c.roberts@intel.com>; Tejun Heo
> <tj@kernel.org>; Jordan Glover <Golden_Miller83@protonmail.ch>; Greg KH
> <gregkh@linuxfoundation.org>; Petr Mladek <pmladek@suse.com>; Joe
> Perches <joe@perches.com>; Ian Campbell <ijc@hellion.org.uk>; Sergey
> Senozhatsky <sergey.senozhatsky@gmail.com>; kernel-
> hardening@lists.openwall.com; Catalin Marinas <catalin.marinas@arm.com>; Will
> Deacon <will.deacon@arm.com>; Steven Rostedt <rostedt@goodmis.org>; Chris
> Fries <cfries@google.com>; Dave Weinstein <olorin@google.com>
> Subject: Re: [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter
> options
> 
> On Wed, Oct 04, 2017 at 04:52:58PM -0700, Linus Torvalds wrote:
> > On Wed, Oct 4, 2017 at 2:58 PM, Roberts, William C
> > <william.c.roberts@intel.com> wrote:
> > >
> > > I agree with you 100% kptr restrict is odd, and I don't think anyone
> > > should have had to opt in to be cleansed via kptr_restrict value via
> > > %pK. Opt-in never works. One nice thing now, is that checkpatch has checking
> of %p usages and warns.
> >
> > Yeah, the checkpatch thing may help for future patches.
> >
> > > As far as broken things, I can't comment on desktop systems where I think it's
> harder to make that claim.
> > > I see value in embedded systems where I am shipping the whole image,
> > > So I know when/what will break.
> > >
> > > If this was in-tree, Android would be setting this to 4 immediately FWIW.
> >
> > Does android set it to 2 right now?
> >
> > But even if it does, my point is that we've had this thing for 6+
> > years, and it really hasn't helped fix our code much at all.
> >
> > In fact, I think the opposite is true. I think it *hurts*. It hurts
> > exactly because it's a hack, and it makes people not bother to fix the
> > real problems.
> >
> > I think we'd be better off just fixing code.
> >
> > One thing you can do today is just look through your 'dmesg' for what
> > looks like kernel addresses (if virtual addresses is what you're
> > interested in - obviously physical addresses will look different). On
> > x86-64, for example, something like this:
> >
> >     dmesg | egrep 'ffff[0-9a-f]{12}'
> >
> > might be interesting to look at. It shows (for me) that we show the
> > percpu address mapping, for example. That certainly sounds interesting
> > to a potential attacker, and I suspect Tejun isn't really interested
> > in that information any more. Added to Cc.
> >
> > It also shows
> >
> >     software IO TLB [mem PA-PB] (64MB) mapped at [VA-VB]
> >
> > for example.  And THIS IS IMPORTANT! The physical address is shown
> > with "%#010llx".
> >
> > See what I'm saying? The kptr_restrict games are just that: games and
> > security theater. If you actually care about things like physical
> > addresses, you don't say "let's hide %pa". Because that's not how the
> > real world works. I found one case that would entirely have missed
> > with the very first trivial grep I did.
> >
> > So I'm claiming that anybody who says "it's too hard to fix it for
> > real, let's just paper over it in vsprintf.c" is fundamentally going
> > to miss things like this. I agree that it might be painful to try to
> > figure out where the problems are, but somebody like google probably
> > has a lot of dmesg output, and it should not be that hard to do the
> > above kinds of "let's just see what leaks, and FIX it".
> 
> This sounds like just the job for an upcoming kernel hacker, with a lot of time and
> not much experience, to do something laborious that no one else wants to do
> and learn a bunch about the kernel.
> 
> So what if we drop this patch set and
> 
> 1. Find and fix every place in the kernel that leaks addresses.
> 2. Verify that checkpatch.pl warns for all potential future leakage.

FYI: This was done in commit 0b523769ebb by Joe Perches. Added to CC line.

> 2. Add a script to check dmesg output for future hardening.
> 
> I'm super keen to work. I would have to get everyone to dump their
> ideas/demands onto me if we are to get this fixed fully.
> 
> thanks,
> Tobin.

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

[Roberts, William C]

> -----Original Message-----
> From: linus971@gmail.com [mailto:linus971@gmail.com] On Behalf Of Linus
> Torvalds
> Sent: Saturday, October 7, 2017 5:08 PM
> To: Theodore Ts'o <tytso@mit.edu>
> Cc: Roberts, William C <william.c.roberts@intel.com>; Tobin C. Harding
> <me@tobin.cc>; Tejun Heo <tj@kernel.org>; Jordan Glover
> <Golden_Miller83@protonmail.ch>; Greg KH <gregkh@linuxfoundation.org>;
> Petr Mladek <pmladek@suse.com>; Joe Perches <joe@perches.com>; Ian
> Campbell <ijc@hellion.org.uk>; Sergey Senozhatsky
> <sergey.senozhatsky@gmail.com>; kernel-hardening@lists.openwall.com;
> Catalin Marinas <catalin.marinas@arm.com>; Will Deacon
> <will.deacon@arm.com>; Steven Rostedt <rostedt@goodmis.org>; Chris Fries
> <cfries@google.com>; Dave Weinstein <olorin@google.com>
> Subject: Re: [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter
> options
> 
> On Sat, Oct 7, 2017 at 4:44 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> >
> > Maybe this is overkill, but what about *encrypting* the pointer using
> > fixed, per-boot random key?  It would have to be a block crypto
> > algorithm, like Tea/XXTea/XXTEA or Speck.  That has all of the
> > benefits of hashing, plus if the key is available (only to privileged
> > users, of course), then it would be possible to get the pointer for
> > debugging purposes.
> 
> I doubt it's going to be used enough to be worth it.
> 
> So I'd rather just make it a hash that gives you that identity for people who want
> it, and then people who actually need the address would need to decide
> whether they _really_ need it in the first place, or whether they would use %lx
> instead together with stricty security rules.
> 
> I did test the patch I sent out (using jhash - so not a cryptographic hash, and with
> a "random value" that was just a constant) just to see that it doesn't break
> anything obvious. As expected, it did break kernel profiling (which uses
> /proc/kallsyms), and it obviously made the kernel pointers I'd already found in
> dmesg be just garbage, but it didn't seem insurmountable. More like a "with a
> couple of fixup cases, it might all work fine".
> 
> So I'd be willing to try it for the next merge window, and see if it causes
> unexpected problems..

I don't particularly hate this idea, and I actually discussed this before with others (not on the list)
before approaching it using kptr_restrict. The argument that we had against this approach was
that it would push people away from using %p and make it even that much harder to audit. But,
I'm not sure if that's really that valid or strong of an argument. I would say go for that approach
with an timeline/agreement to kill kptr_restrict.

Another possible solution discussed would be to enable an option to encrypt dmesg. But I
steered away from that.

I settled on using kptr_restrict for the ability to enable/disable. One of the use cases
we had was that a driver is found to be doing this silly thing, and an admin/user wants
to turn it off in dmesg until its fixed.

> 
>                 Linus

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

[Linus Torvalds]

On Fri, Oct 13, 2017 at 9:32 AM, Roberts, William C
<william.c.roberts@intel.com> wrote:
>
> I don't particularly hate this idea, and I actually discussed this before with others (not on the list)
> before approaching it using kptr_restrict. The argument that we had against this approach was
> that it would push people away from using %p and make it even that much harder to audit.

So I think kptr_restrict was worth trying. I'm not saying it was a bad
idea when it was introduced.

It's just that it's six years later, we now have the knowledge that
opt-in doesn't work for this (either), and that we should just admit
that it didn't work very well.

And even as a "failure", the global flag may well work fine for soem
uses (ie Android), largely because hardly anybody actually _develops_
using Android. So it may not have been a failure in that sense, and
for all I know the Android use-case really sees it as a huge success
(and extending on it there would have made sense).

> Another possible solution discussed would be to enable an option to encrypt dmesg. But I
> steered away from that.

Yeah, that would make it really hard for bug reporters.  And as
discussed, it's not _just_ about dmesg, since %p shows up elsewhere
too.

That said, it *might* make sense to restrict dmesg, and have actual
hard limits on message levels. Many (most?) of the %p users that
intentionally expose kernel and hardware addresses are about debug
messages. Often they are disabled entirely anyway, but it might make
sense to just limit the dmesg output to "information and above" to
regular users.

But practically speaking, you end up traditionally having the system
logs visible other ways (ie /var/log/messages or journalctl) that
would make that kind of limiting pointless.

So I think we're practically speaking limited to just trying to avoid
printing sensitive information. And for all the historical baggage
reasons, I do think we effectively just need to make it do so by
default, with an opt-out for people who have good reason to do so,
because '%pK' obviously didn't really end up working.

(We _do_ have some %pK in the driver code that has grown up over the
last 6+ years, but they are a small minority, and most of them are
actually fundamentally broken with kptr_restrict=1, and only work with
the values of 0 or 2+)

> I settled on using kptr_restrict for the ability to enable/disable. One of the use cases
> we had was that a driver is found to be doing this silly thing, and an admin/user wants
> to turn it off in dmesg until its fixed.

Hopefully just making %p print out a hash will fix the silly things.

              Linus

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

[Kees Cook]

On Fri, Oct 13, 2017 at 11:11 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Oct 13, 2017 at 9:32 AM, Roberts, William C
> <william.c.roberts@intel.com> wrote:
>>
>> I don't particularly hate this idea, and I actually discussed this before with others (not on the list)
>> before approaching it using kptr_restrict. The argument that we had against this approach was
>> that it would push people away from using %p and make it even that much harder to audit.
>
> So I think kptr_restrict was worth trying. I'm not saying it was a bad
> idea when it was introduced.
>
> It's just that it's six years later, we now have the knowledge that
> opt-in doesn't work for this (either), and that we should just admit
> that it didn't work very well.
>
> And even as a "failure", the global flag may well work fine for soem
> uses (ie Android), largely because hardly anybody actually _develops_
> using Android. So it may not have been a failure in that sense, and
> for all I know the Android use-case really sees it as a huge success
> (and extending on it there would have made sense).

That's precisely where this comes from: the original patches from
Tobin (and Greg) were trying to make this extension, based on the work
done in the Android kernels (which was based on work by William, etc).

>> I settled on using kptr_restrict for the ability to enable/disable. One of the use cases
>> we had was that a driver is found to be doing this silly thing, and an admin/user wants
>> to turn it off in dmesg until its fixed.
>
> Hopefully just making %p print out a hash will fix the silly things.

I'm unable to tell if you're universally opposed to a global flag, or
if you're acknowledging that it has use in certain environments.

I get the sense that unconditionally censoring (either through
omission or a hash) of %p will likely drive people to using %x, and
that'll be really hard to audit. (I suppose it'd be possible to build
a compiler plugin that would look for %x uses where the format
argument was cast from a pointer type, but it seems like it'd be best
to avoid even needing such things.)

Having a global flag for censorship of %p means the developer case can
be left unchanged (meaning less migration to %x). Or were you
suggesting a global flag that controls the censorship level (e.g.
something like "none", "hash", "all zero")?

-Kees

-- 
Kees Cook
Pixel Security

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

[Linus Torvalds]

On Fri, Oct 13, 2017 at 12:34 PM, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Oct 13, 2017 at 11:11 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> And even as a "failure", the global flag may well work fine for soem
>> uses (ie Android), largely because hardly anybody actually _develops_
>> using Android. So it may not have been a failure in that sense, and
>> for all I know the Android use-case really sees it as a huge success
>> (and extending on it there would have made sense).
>
> That's precisely where this comes from: the original patches from
> Tobin (and Greg) were trying to make this extension, based on the work
> done in the Android kernels (which was based on work by William, etc).

Yes, but what I want so-called "security" people and Andoid people to
acknowledge is that it was

 (a) a hack

 (b) not actually real security

 (c) didn't actually help long-term development at all, and in fact
likely held real improvements *back* because it was "whatever, we
don't have a problem".

It's security theater. Particularly with the opt-in, it was never
anything more. There are _very_ few %pK users in the kernel, and those
that are there could generally have been just removed entirely, or
have been made to check the flag _locally_ instead of globally, and
been made better in the process.

The new extension that raised kptr_restrict to 4 was just more of the
same. It was in no way real security. It's exactly the same as the TSA
"advisory system" color coding.

Do you seriously believe that color coded threat levels were a good
thing? The whole "kptr_restrict=4" is exactly the same thing as the
TSA "threat level red". It's a joke. It doesn't actually _fix_
anything.

Guys, it took me five seconds to come up with that

    dmesg | egrep '[0-9a-f]{16}'

pattern, and find several printouts that weren't %pK, and that
wouldn't have been caught by the new "strict" color coding EITHER.

Seriously. It's BS.

It's BS exactly the same way profiling by the TSA is BS. Sure, if you
stop  "brown single male" passengers, you probably will stop
something. But you don't really _fix_ anything.

The "%p" pattern is just the kernel equivalent of "brown single male".
It's not a real security fix, it never has been, and it never will be.
There are probably _more_ places that export physical addresses with
%x than with %pa.

So what I suggested was that we actually look at what the kernel
exports, and we make it easy to just shut _real_ problems down.

The whole "let's make %p print a hashed address" is not the real
security part. That's just the "make it easy to do something secure by
_default_" part. The real security is actually scanning for bad
things, not profiling the escape sequences.

Is "scan for potential actual; leaks" some kind of absolute security?
No. But at least it's not just silly games.

>> Hopefully just making %p print out a hash will fix the silly things.
>
> I'm unable to tell if you're universally opposed to a global flag, or
> if you're acknowledging that it has use in certain environments.

I'm acknowledging that it's a complete hack that may have made sense
in a particular setting, but it's not one that actually fixes any real
security issues, and in particular, it's one that has likely held
people back from doing *real* security.

And as a maintainer of the kernel, to me that "long term actual
development" is a whole lot more important than "silly hack that might
close a random escape or two".

So yes, I'm actually opposed to the global flag, because we seriously
have 6+ years of reality staring us in the face: it fixed exactly
nothing. It fixed /proc/kallsyms,m but did so _badly_. It fixed a
handful of other places. But it didn't really and truly add any real
security.

Are people really in denial about this? Really?

              Linus

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

[Kees Cook]

On Fri, Oct 13, 2017 at 1:22 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> I'm acknowledging that it's a complete hack that may have made sense
> in a particular setting, but it's not one that actually fixes any real
> security issues, and in particular, it's one that has likely held
> people back from doing *real* security.
>
> And as a maintainer of the kernel, to me that "long term actual
> development" is a whole lot more important than "silly hack that might
> close a random escape or two".
>
> So yes, I'm actually opposed to the global flag, because we seriously
> have 6+ years of reality staring us in the face: it fixed exactly
> nothing. It fixed /proc/kallsyms,m but did so _badly_. It fixed a
> handful of other places. But it didn't really and truly add any real
> security.
>
> Are people really in denial about this? Really?

I totally agree that %pK isn't actually useful. (I think of %pK and a
global flag controlling %p behavior as separate things.) The original
patch I was talking about fully acknowledges this (%pK not actually
working) and was about changing %p behavior. Some other solution was
needed that didn't drive developers into using %x for everything
instead.

> The whole "let's make %p print a hashed address" is not the real
> security part. That's just the "make it easy to do something secure by
> _default_" part. The real security is actually scanning for bad
> things, not profiling the escape sequences.

So yes, it's trivial to scan for stuff in dmesg, but it's much less
trivial to find the cases where some /proc entry only has %p reports
under certain runtime conditions. (e.g. print the allocation address
of a pending wifi association timer or something: visible only during
a brief window, and something not obvious to a casual scan of /proc
entry contents, but an attacker might be able to influence and peek
at.)

I'm also fine with eliminating the use of %p in source, but that's a
serious game of whack-a-mole, and I don't think it scales. So, yeah,
while I do like the %p-hash idea, I remain concerned that we'll just
have this whole discussion again in a few years, trying to figure out
what to do about the now heavily-used %x.

Is the correct path to:
- unconditionally convert %p to reporting a 32-bit hash
- actively start removing as much %p use as possible
- do something to discourage %x on pointers (checkpatch.pl?)

Can we do something more about %x?

Do we want to remove %pK also?

-Kees

-- 
Kees Cook
Pixel Security

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

[Linus Torvalds]

On Fri, Oct 13, 2017 at 1:47 PM, Kees Cook <keescook@chromium.org> wrote:
>
> Is the correct path to:
> - unconditionally convert %p to reporting a 32-bit hash
> - actively start removing as much %p use as possible
> - do something to discourage %x on pointers (checkpatch.pl?)

I don't know if we even need to remove %p if we just unconditionally hash it.

I _think_ that what will happen if we just start hashing %p is that
99% of existing %p cases will just stay around, because many of them
are likely effectively stale. Maybe they were useful for driver
development, but they probably aren't any more.

The 1% that will cause people to look at the code may well be painful,
though. We are potentially talking more than just a handful here.

We *may* actually want to introduce %pX as a way to opt _out_ of the
hashing, so that  %p users that really do want an address can keep it
- while making it  really easy to grep for (and then maybe
kptr_restrict=4 would say "even %pX gets hashed anyway").

As to %x and pointers, I'm not sure how to do that with checkpatch.
And I don't even think it's about pointers. If people care about the
physical addresses leaking (and the %pa format kind of indicates
people do), the most common physical addresses likely are %x printouts
before those addresses were made into pointers at all. Those physical
addresses often came from somewhere else where they were just integers
(eg PCI BAR values etc).

So realistically, that's where the "let's try to come up with scripts
to find those things" comes in.

... and just perhaps generally talk about this "don't leak pointers or
physical addresses to random user space" so that people are more aware
of it in general.

That, btw, might be a side effect of %p being hashed in the first
place. People will go "WTF?" for a while, and just _explaining_ the %p
hashing might make people more aware of this issue in the first place.

Or maybe I'm just unrealistically optimistic, and what will _actually_
happen if we start hashing %p output is just a lot and lot of
complaining and whining and tons of breakage. ;(

> Can we do something more about %x?
>
> Do we want to remove %pK also?

I'm not sure we need to. But there may be %pK users that simply think
that getting a hashed address is better than getting just zeroes, and
that would thus prefer going back to %p.

           Linus

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

[Theodore Ts'o]

On Fri, Oct 13, 2017 at 01:47:07PM -0700, Kees Cook wrote:
> Can we do something more about %x?

Can we have sparse or some other static checking tool complain when a
pointer is cast to an integer type in a printf/sprintf context?

	   	      	      	   - Ted