[RFC 00/06] printk: add more new kernel pointer filter options.

[RFC 00/06] printk: add more new kernel pointer filter options.

[Greg KH]

Here's a short patch series from Chris Fries and Dave Weinstein that
implement 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 is
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.  It applies
cleanly on top of 4.11 as well as Linus's current development tree
(10502 patches into the 4.12-rc1 merge window).  I'm posting it now for
comments if anyone sees anything wrong with this approach, or thinks the
things that are being whitelisted should not be?

thanks,

greg k-h

 Documentation/printk-formats.txt |   15 +++-
 Documentation/sysctl/kernel.txt  |   11 +++
 arch/arm64/kernel/traps.c        |    4 -
 drivers/uio/uio.c                |    4 -
 include/linux/kallsyms.h         |    2
 kernel/printk/printk.c           |    2
 kernel/sysctl.c                  |    6 -
 lib/vsprintf.c                   |  128 ++++++++++++++++++++++++++++-----------
 8 files changed, 123 insertions(+), 49 deletions(-)

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

[Greg KH]

From: Dave Weinstein <olorin@google.com>

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.

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

Cc: William Roberts <william.c.roberts@intel.com>
Cc: Chris Fries <cfries@google.com>
Signed-off-by: Dave Weinstein <olorin@google.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 Documentation/printk-formats.txt |  5 +++
 Documentation/sysctl/kernel.txt  |  3 ++
 kernel/sysctl.c                  |  3 +-
 lib/vsprintf.c                   | 81 ++++++++++++++++++++++++++--------------
 4 files changed, 63 insertions(+), 29 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 5962949944fd..8994c65aa3b0 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -64,6 +64,11 @@ Kernel Pointers:
 	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:
 
 	%pr	[mem 0x60000000-0x6fffffff flags 0x2200] or
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index bac23c198360..c9f5da409868 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -392,6 +392,9 @@ 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.
+
 ==============================================================
 
 l2cr: (PPC only)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 8c8714fcb53c..1bfdd262c66a 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
@@ -830,7 +831,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 e3bf4e0f10b5..f4e11dade1ab 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -395,6 +395,16 @@ struct printf_spec {
 #define FIELD_WIDTH_MAX ((1 << 23) - 1)
 #define PRECISION_MAX ((1 << 15) - 1)
 
+int kptr_restrict __read_mostly;
+
+/*
+ * Always cleanse %p and %pK specifiers
+ */
+static inline int kptr_restrict_always_cleanse_pointers(void)
+{
+	return kptr_restrict >= 3;
+}
+
 static noinline_for_stack
 char *number(char *buf, char *end, unsigned long long num,
 	     struct printf_spec spec)
@@ -1470,8 +1480,6 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)
 	return format_flags(buf, end, flags, names);
 }
 
-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
@@ -1540,6 +1548,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):
@@ -1569,6 +1578,9 @@ int kptr_restrict __read_mostly;
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a
  * pointer to the real address.
+ *
+ * Note: 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,
@@ -1576,7 +1588,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_always_cleanse_pointers()) {
 		/*
 		 * Print (null) with the same width as a pointer so it makes
 		 * tabular output look nice.
@@ -1657,10 +1669,43 @@ 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;
+	default:
+		/*
+		 * plain %p, no extension, check if we should always cleanse and
+		 * treat like %pK.
+		 */
+		if (!kptr_restrict_always_cleanse_pointers())
+			break;
+		/* fallthrough */
 	case 'K':
 		switch (kptr_restrict) {
 		case 0:
-			/* Always print %pK values */
+			/* Always print %p values */
 			break;
 		case 1: {
 			const struct cred *cred;
@@ -1679,7 +1724,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			 * Only print the real pointer value if the current
 			 * process has CAP_SYSLOG and is running with the
 			 * same credentials it started with. This is because
-			 * access to files is checked at open() time, but %pK
+			 * access to files is checked at open() time, but %p
 			 * checks permission at read() time. We don't want to
 			 * leak pointer values if a binary opens a file using
 			 * %pK and then elevates privileges before reading it.
@@ -1691,33 +1736,13 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 				ptr = NULL;
 			break;
 		}
-		case 2:
+		case 2: /* restrict only %pK */
+		case 3: /* restrict all non-extensioned %p and %pK */
 		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);
 	}
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {
@@ -1726,7 +1751,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);
 }
 
 /*
-- 
2.12.2

[RFC 2/6] lib: vsprintf: whitelist stack traces

[Greg KH]

From: Dave Weinstein <olorin@google.com>

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

Cc: William Roberts <william.c.roberts@intel.com>
Cc: Chris Fries <cfries@google.com>
Signed-off-by: Dave Weinstein <olorin@google.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 arch/arm64/kernel/traps.c | 4 ++--
 include/linux/kallsyms.h  | 2 +-
 kernel/printk/printk.c    | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index e52be6aa44ee..b127be39011a 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -146,7 +146,7 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 	unsigned long irq_stack_ptr;
 	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;
@@ -252,7 +252,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/include/linux/kallsyms.h b/include/linux/kallsyms.h
index 6883e197acb9..e4a205debe09 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -122,7 +122,7 @@ static inline void print_symbol(const char *fmt, unsigned long addr)
 
 static inline void print_ip_sym(unsigned long ip)
 {
-	printk("[<%p>] %pS\n", (void *) ip, (void *) ip);
+	printk("[<%pP>] %pS\n", (void *) ip, (void *) ip);
 }
 
 #endif /*_LINUX_KALLSYMS_H*/
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 2984fb0f0257..d32e84e3ab6b 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3110,7 +3110,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.12.2

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

[Greg KH]

From: Dave Weinstein <olorin@google.com>

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

Cc: William Roberts <william.c.roberts@intel.com>
Cc: Chris Fries <cfries@google.com>
Signed-off-by: Dave Weinstein <olorin@google.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 Documentation/sysctl/kernel.txt |  8 +++++++-
 kernel/sysctl.c                 |  3 +--
 lib/vsprintf.c                  | 33 ++++++++++++++++++++++++++++++---
 3 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index c9f5da409868..df069ec42e4a 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -393,7 +393,13 @@ 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.
+%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.
 
 ==============================================================
 
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 1bfdd262c66a..acf7e6cb00b4 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
@@ -831,7 +830,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 f4e11dade1ab..75a49795fcae 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -405,6 +405,22 @@ static inline int kptr_restrict_always_cleanse_pointers(void)
 	return kptr_restrict >= 3;
 }
 
+/*
+ * Always cleanse physical addresses (%pa* specifiers)
+ */
+static inline int kptr_restrict_cleanse_addresses(void)
+{
+	return kptr_restrict >= 4;
+}
+
+/*
+ * Always cleanse resource addresses (%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)
@@ -757,6 +773,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++ = '[';
@@ -784,10 +801,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) {
@@ -1390,7 +1408,9 @@ char *address_val(char *buf, char *end, const void *addr, const char *fmt)
 		break;
 	}
 
-	return special_hex_number(buf, end, num, size);
+	return special_hex_number(buf, end,
+		      kptr_restrict_cleanse_addresses() ? 0UL : num,
+		      size);
 }
 
 static noinline_for_stack
@@ -1581,6 +1601,12 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)
  *
  * 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,
@@ -1738,6 +1764,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		}
 		case 2: /* restrict only %pK */
 		case 3: /* restrict all non-extensioned %p and %pK */
+		case 4: /* restrict all non-extensioned %p, %pK, %pa*, %p[rR] */
 		default:
 			ptr = NULL;
 			break;
-- 
2.12.2

[RFC 4/6] lib: vsprintf: default kptr_restrict to the maximum value

[Greg KH]

From: Dave Weinstein <olorin@google.com>

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.

Cc: William Roberts <william.c.roberts@intel.com>
Cc: Chris Fries <cfries@google.com>
Signed-off-by: Dave Weinstein <olorin@google.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 lib/vsprintf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 75a49795fcae..404d477d4bd2 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -395,7 +395,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;
 
 /*
  * Always cleanse %p and %pK specifiers
-- 
2.12.2

[RFC 5/6] lib: vsprintf: Add "%paP", "%padP" options

[Greg KH]

From: Chris Fries <cfries@google.com>

Add %paP and %padP for physical address that need to always be shown
regardless of kptr restrictions.

Cc: William Roberts <william.c.roberts@intel.com>
Cc: Dave Weinstein <olorin@google.com>
Signed-off-by: Chris Fries <cfries@google.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 Documentation/printk-formats.txt | 10 ++++++----
 lib/vsprintf.c                   | 12 +++++++++---
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 8994c65aa3b0..7ee51269096f 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -82,18 +82,20 @@ Struct Resources:
 
 Physical addresses types phys_addr_t:
 
-	%pa[p]	0x01234567 or 0x0123456789abcdef
+	%pa[p][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/lib/vsprintf.c b/lib/vsprintf.c
index 404d477d4bd2..37f9d615e622 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1394,23 +1394,29 @@ 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 thru */
 	default:
 		num = *(const phys_addr_t *)addr;
 		size = sizeof(phys_addr_t);
 		break;
 	}
 
-	return special_hex_number(buf, end,
-		      kptr_restrict_cleanse_addresses() ? 0UL : num,
-		      size);
+	/* 'P' on the tail means don't restrict the pointer. */
+	cleanse = cleanse && (fmt[decleanse_idx] != 'P');
+
+	return special_hex_number(buf, end, cleanse ? 0UL : num, size);
 }
 
 static noinline_for_stack
-- 
2.12.2

[RFC 6/6] drivers: uio: Un-restrict sysfs pointers for UIO

[Greg KH]

From: Chris Fries <cfries@google.com>

The addr and size on the UIO devices are required by userspace to function
properly.  Let's unrestrict these by adding the 'P' modifier to %p and %pa.

Cc: William Roberts <william.c.roberts@intel.com>
Cc: Dave Weinstein <olorin@google.com>
Signed-off-by: Chris Fries <cfries@google.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 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 60ce7fd54e89..d9eae52519f4 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.12.2

Re: [RFC 5/6] lib: vsprintf: Add "%paP", "%padP" options

[Joe Perches]

On Fri, 2017-05-05 at 21:07 -0700, Greg KH wrote:
> From: Chris Fries <cfries@google.com>
> 
> Add %paP and %padP for physical address that need to always be shown
> regardless of kptr restrictions.

The commit message could be improved.

I had to look at the actual code to see if %papP was supported.

> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
[]
> @@ -82,18 +82,20 @@ Struct Resources:
>  
>  Physical addresses types phys_addr_t:
>  
> -	%pa[p]	0x01234567 or 0x0123456789abcdef
> +	%pa[p][P] 0x01234567 or 0x0123456789abcdef

Well, that's good.

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> @@ -1394,23 +1394,29 @@ 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();

bool

Re: [RFC 5/6] lib: vsprintf: Add "%paP", "%padP" options

[Greg KH]

On Fri, May 05, 2017 at 09:42:40PM -0700, Joe Perches wrote:
> On Fri, 2017-05-05 at 21:07 -0700, Greg KH wrote:
> > From: Chris Fries <cfries@google.com>
> > 
> > Add %paP and %padP for physical address that need to always be shown
> > regardless of kptr restrictions.
> 
> The commit message could be improved.

Good point, I'll work on that, thanks.

> I had to look at the actual code to see if %papP was supported.
> 
> > diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> []
> > @@ -82,18 +82,20 @@ Struct Resources:
> >  
> >  Physical addresses types phys_addr_t:
> >  
> > -	%pa[p]	0x01234567 or 0x0123456789abcdef
> > +	%pa[p][P] 0x01234567 or 0x0123456789abcdef
> 
> Well, that's good.
> 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
> > @@ -1394,23 +1394,29 @@ 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();
> 
> bool

I'll change this, thanks.

greg k-h

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

[Ian Campbell]

On Fri, 2017-05-05 at 21:07 -0700, Greg KH wrote:
> From: Dave Weinstein <olorin@google.com>
> 
> Add the kptr_restrict setting of 4 which results in %pa and
> %p[rR] values being replaced by zeros.

Given that '%pa' is:
 * - 'a[pd]' For address types [p] phys_addr_t, [d] dma_addr_t and derivatives
 *           (default assumed to be phys_addr_t, passed by reference)

what is the thread model which hiding physical addresses from attackers
protects against? I can see why virtual addresses would be obviously
dangerous but physical addresses seem less obvious and I didn't see it
spelled out in any of the commit messages or added comments in the
thread.

I think a comment somewhere would be useful for people who are trying
to decide if they should use %pa vs %paP etc.

Ian.

Re: [RFC 00/06] printk: add more new kernel pointer filter options.

[Sergey Senozhatsky]

Hello Greg,

On (05/05/17 21:06), Greg KH wrote:
> Here's a short patch series from Chris Fries and Dave Weinstein that
> implement 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 is
> 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.  It applies
> cleanly on top of 4.11 as well as Linus's current development tree
> (10502 patches into the 4.12-rc1 merge window).  I'm posting it now for
> comments if anyone sees anything wrong with this approach

overall, I don't see anything wrong.

> or thinks the things that are being whitelisted should not be?

can't say for sure, sorry.

	-ss

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

[Petr Mladek]

On Fri 2017-05-05 21:06:56, Greg KH wrote:
> From: Dave Weinstein <olorin@google.com>
> 
> 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.
> 
> This patch is based on work by William Roberts
> <william.c.roberts@intel.com>
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index e3bf4e0f10b5..f4e11dade1ab 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -395,6 +395,16 @@ struct printf_spec {
>  #define FIELD_WIDTH_MAX ((1 << 23) - 1)
>  #define PRECISION_MAX ((1 << 15) - 1)
>  
> +int kptr_restrict __read_mostly;
> +
> +/*
> + * Always cleanse %p and %pK specifiers
> + */
> +static inline int kptr_restrict_always_cleanse_pointers(void)

The name of the function is very long and still confusing.
It uses the word "always" but there are many types of pointers
that are not cleared with this condition, for example %pP, %pa.

Do we need this helper function at all? It is used
a weird way, see below.

> +{
> +	return kptr_restrict >= 3;
> +}
> +
>  static noinline_for_stack
>  char *number(char *buf, char *end, unsigned long long num,
>  	     struct printf_spec spec)
> @@ -1576,7 +1588,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_always_cleanse_pointers()) {
>  		/*
>  		 * Print (null) with the same width as a pointer so it makes
>  		 * tabular output look nice.
> @@ -1657,10 +1669,43 @@ 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;
> +	default:
> +		/*
> +		 * plain %p, no extension, check if we should always cleanse and
> +		 * treat like %pK.
> +		 */
> +		if (!kptr_restrict_always_cleanse_pointers())
> +			break;
> +		/* fallthrough */

Using default: in the middle of other cases is pretty confusing
and a call for troubles.

>  	case 'K':
>  		switch (kptr_restrict) {
>  		case 0:
> -			/* Always print %pK values */
> +			/* Always print %p values */

And this is one example. We are never here for %p because we
called break in the "default:" case above for kptr_restrict == 0.


>  			break;
>  		case 1: {
>  			const struct cred *cred;
> @@ -1679,7 +1724,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  			 * Only print the real pointer value if the current
>  			 * process has CAP_SYSLOG and is running with the
>  			 * same credentials it started with. This is because
> -			 * access to files is checked at open() time, but %pK
> +			 * access to files is checked at open() time,
>  		but %p

Same here. This change makes the feeling that we check CAP_SYSLOG even
for plain %p but we actually do not do it.

>  			 * checks permission at read() time. We don't want to
>  			 * leak pointer values if a binary opens a file using
>  			 * %pK and then elevates privileges before reading it.
> @@ -1691,33 +1736,13 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  				ptr = NULL;
>  			break;
>  		}
> -		case 2:
> +		case 2: /* restrict only %pK */
> +		case 3: /* restrict all non-extensioned %p and %pK */
>  		default:
> -			/* Always print 0's for %pK */
>  			ptr = NULL;
>  			break;
>  		}
>  		break;


I would personally write this piece of code a more straightforward way,
for example:

	switch (*fmt) {
[...]
	case 'P':
		/* Always print an explicitly whitelisted kernel pointer. */
		break;
	case 'K':
		/* Always print %pK values when there are no restrictions. */
		if (!kptr_restrict)
			break;

		/* Cleanse %pK values for non-privileged users. */
		if (kptr_restrict == 1) {
			const struct cred *cred;

			/*
			 * kptr_restrict==1 cannot be used in IRQ context
			 * because its test for CAP_SYSLOG would be meaningless.
			 */
			if (in_irq() || in_serving_softirq() || in_nmi()) {
				if (spec.field_width == -1)
					spec.field_width = default_width;
				return string(buf, end, "pK-error", spec);
			}

			/*
			 * Only print the real pointer value if the current
			 * process has CAP_SYSLOG and is running with the
			 * same credentials it started with. This is because
			 * access to files is checked at open() time, but %p
			 * checks permission at read() time. We don't want to
			 * leak pointer values if a binary opens a file using
			 * %pK and then elevates privileges before reading it.
			 */
			cred = current_cred();
			if (!has_capability_noaudit(current, CAP_SYSLOG) ||
			    !uid_eq(cred->euid, cred->uid) ||
			    !gid_eq(cred->egid, cred->gid))
				ptr = NULL;
			break;
		}

		/* Always cleanse %pK values in higher restrition levels. */
		ptr = NULL;
		break;

	default:
		/*
		 * Also plain pointers (%p) are always cleansed in higher
		 * restriction levels.
		 */
		if (kptr_restrict >= 3)
			ptr = NULL;
	}

Best Regards,
Petr

Re: [RFC 5/6] lib: vsprintf: Add "%paP", "%padP" options

[Petr Mladek]

On Fri 2017-05-05 21:07:47, Greg KH wrote:
> From: Chris Fries <cfries@google.com>
> 
> Add %paP and %padP for physical address that need to always be shown
> regardless of kptr restrictions.
> 
> Cc: William Roberts <william.c.roberts@intel.com>
> Cc: Dave Weinstein <olorin@google.com>
> Signed-off-by: Chris Fries <cfries@google.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  Documentation/printk-formats.txt | 10 ++++++----
>  lib/vsprintf.c                   | 12 +++++++++---
>  2 files changed, 15 insertions(+), 7 deletions(-)

This patch should update also the section about
kptr_restrict in Documentation/sysctl/kernel.txt.
It should mention that the trailing P allows to see
pointers also for %pa, and %p[rR] formats when
the level is 4.

Best Regards,
Petr

RE: [RFC 00/06] printk: add more new kernel pointer filter options.

[Roberts, William C]

> -----Original Message-----
> From: Sergey Senozhatsky [mailto:sergey.senozhatsky.work@gmail.com]
> Sent: Wednesday, May 10, 2017 6:38 PM
> To: Greg KH <gregkh@linuxfoundation.org>
> Cc: kernel-hardening@lists.openwall.com; Petr Mladek <pmladek@suse.com>;
> Sergey Senozhatsky <sergey.senozhatsky@gmail.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: [RFC 00/06] printk: add more new kernel pointer filter options.
> 
> Hello Greg,
> 
> On (05/05/17 21:06), Greg KH wrote:
> > Here's a short patch series from Chris Fries and Dave Weinstein that
> > implement 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 is
> > 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.  It applies
> > cleanly on top of 4.11 as well as Linus's current development tree
> > (10502 patches into the 4.12-rc1 merge window).  I'm posting it now
> > for comments if anyone sees anything wrong with this approach
> 
> overall, I don't see anything wrong.
> 
> > or thinks the things that are being whitelisted should not be?
> 
> can't say for sure, sorry.
> 
> 	-ss

I almost missed this, none of the mail was delivered to my inbox...

Anyways, I am glad to see this revived and I don't have any
Comments besides thanks.

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

[Greg KH]

On Tue, May 16, 2017 at 01:58:11PM +0200, Petr Mladek wrote:
> On Fri 2017-05-05 21:06:56, Greg KH wrote:
> > From: Dave Weinstein <olorin@google.com>
> > 
> > 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.
> > 
> > This patch is based on work by William Roberts
> > <william.c.roberts@intel.com>
> > 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index e3bf4e0f10b5..f4e11dade1ab 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -395,6 +395,16 @@ struct printf_spec {
> >  #define FIELD_WIDTH_MAX ((1 << 23) - 1)
> >  #define PRECISION_MAX ((1 << 15) - 1)
> >  
> > +int kptr_restrict __read_mostly;
> > +
> > +/*
> > + * Always cleanse %p and %pK specifiers
> > + */
> > +static inline int kptr_restrict_always_cleanse_pointers(void)
> 
> The name of the function is very long and still confusing.
> It uses the word "always" but there are many types of pointers
> that are not cleared with this condition, for example %pP, %pa.
> 
> Do we need this helper function at all? It is used
> a weird way, see below.

Thanks for the comments, I'll revise this for the next version, thanks
so much for the review, much appreciated.

thanks,

greg k-h

Re: [RFC 5/6] lib: vsprintf: Add "%paP", "%padP" options

[Greg KH]

On Tue, May 16, 2017 at 04:41:04PM +0200, Petr Mladek wrote:
> On Fri 2017-05-05 21:07:47, Greg KH wrote:
> > From: Chris Fries <cfries@google.com>
> > 
> > Add %paP and %padP for physical address that need to always be shown
> > regardless of kptr restrictions.
> > 
> > Cc: William Roberts <william.c.roberts@intel.com>
> > Cc: Dave Weinstein <olorin@google.com>
> > Signed-off-by: Chris Fries <cfries@google.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  Documentation/printk-formats.txt | 10 ++++++----
> >  lib/vsprintf.c                   | 12 +++++++++---
> >  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> This patch should update also the section about
> kptr_restrict in Documentation/sysctl/kernel.txt.
> It should mention that the trailing P allows to see
> pointers also for %pa, and %p[rR] formats when
> the level is 4.

Ah, good point, will do!

greg k-h

Re: [RFC 00/06] printk: add more new kernel pointer filter options.

[Greg KH]

On Tue, May 16, 2017 at 09:36:37PM +0000, Roberts, William C wrote:
> 
> 
> > -----Original Message-----
> > From: Sergey Senozhatsky [mailto:sergey.senozhatsky.work@gmail.com]
> > Sent: Wednesday, May 10, 2017 6:38 PM
> > To: Greg KH <gregkh@linuxfoundation.org>
> > Cc: kernel-hardening@lists.openwall.com; Petr Mladek <pmladek@suse.com>;
> > Sergey Senozhatsky <sergey.senozhatsky@gmail.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: [RFC 00/06] printk: add more new kernel pointer filter options.
> > 
> > Hello Greg,
> > 
> > On (05/05/17 21:06), Greg KH wrote:
> > > Here's a short patch series from Chris Fries and Dave Weinstein that
> > > implement 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 is
> > > 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.  It applies
> > > cleanly on top of 4.11 as well as Linus's current development tree
> > > (10502 patches into the 4.12-rc1 merge window).  I'm posting it now
> > > for comments if anyone sees anything wrong with this approach
> > 
> > overall, I don't see anything wrong.
> > 
> > > or thinks the things that are being whitelisted should not be?
> > 
> > can't say for sure, sorry.
> > 
> > 	-ss
> 
> I almost missed this, none of the mail was delivered to my inbox...

Why not?  Did I get the address wrong?

> Anyways, I am glad to see this revived and I don't have any
> Comments besides thanks.

Acks for the patches are always appreciated :)

I'll revise this in the next few weeks and send out a new series.

thanks,

greg k-h

RE: [RFC 00/06] printk: add more new kernel pointer filter options.

[Roberts, William C]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, May 18, 2017 7:13 AM
> To: Roberts, William C <william.c.roberts@intel.com>
> Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>; kernel-
> hardening@lists.openwall.com; Petr Mladek <pmladek@suse.com>; Sergey
> Senozhatsky <sergey.senozhatsky@gmail.com>; linux-kernel@vger.kernel.org;
> 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: [RFC 00/06] printk: add more new kernel pointer filter options.
> 
> On Tue, May 16, 2017 at 09:36:37PM +0000, Roberts, William C wrote:
> >
> >
> > > -----Original Message-----
> > > From: Sergey Senozhatsky [mailto:sergey.senozhatsky.work@gmail.com]
> > > Sent: Wednesday, May 10, 2017 6:38 PM
> > > To: Greg KH <gregkh@linuxfoundation.org>
> > > Cc: kernel-hardening@lists.openwall.com; Petr Mladek
> > > <pmladek@suse.com>; Sergey Senozhatsky
> > > <sergey.senozhatsky@gmail.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: [RFC 00/06] printk: add more new kernel pointer filter options.
> > >
> > > Hello Greg,
> > >
> > > On (05/05/17 21:06), Greg KH wrote:
> > > > Here's a short patch series from Chris Fries and Dave Weinstein
> > > > that implement 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 is
> > > > 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.  It
> > > > applies cleanly on top of 4.11 as well as Linus's current
> > > > development tree
> > > > (10502 patches into the 4.12-rc1 merge window).  I'm posting it
> > > > now for comments if anyone sees anything wrong with this approach
> > >
> > > overall, I don't see anything wrong.
> > >
> > > > or thinks the things that are being whitelisted should not be?
> > >
> > > can't say for sure, sorry.
> > >
> > > 	-ss
> >
> > I almost missed this, none of the mail was delivered to my inbox...
> 
> Why not?  Did I get the address wrong?

I don't think so. I've had weird issues with my Intel email address and mailing
lists before. On the selinux mailing list they kept getting bounces when sending
Me email, but it's only that list. I'm just going to blame it on something within
our corporate network.

> 
> > Anyways, I am glad to see this revived and I don't have any Comments
> > besides thanks.
> 
> Acks for the patches are always appreciated :)
> 
> I'll revise this in the next few weeks and send out a new series.

I see some comments on clarifying the docs that seem spot on.
I'll look at the next series and I will test them, if they look good
to me, I'll ack away :-)

> 
> thanks,
> 
> greg k-h