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

[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

[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

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

[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

[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