Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1636815 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFC 00/06] printk: add more new kernel pointer filter options. Greg KH <gregkh@linuxfoundation.org> - 2017-05-06 06:10 +0200
    [RFC 6/6] drivers: uio: Un-restrict sysfs pointers for UIO Greg KH <gregkh@linuxfoundation.org> - 2017-05-06 06:10 +0200
    [RFC 5/6] lib: vsprintf: Add "%paP", "%padP" options Greg KH <gregkh@linuxfoundation.org> - 2017-05-06 06:10 +0200
      Re: [RFC 5/6] lib: vsprintf: Add "%paP", "%padP" options Joe Perches <joe@perches.com> - 2017-05-06 06:50 +0200
        Re: [RFC 5/6] lib: vsprintf: Add "%paP", "%padP" options Greg KH <gregkh@linuxfoundation.org> - 2017-05-06 07:10 +0200
    [RFC 4/6] lib: vsprintf: default kptr_restrict to the maximum value Greg KH <gregkh@linuxfoundation.org> - 2017-05-06 06:10 +0200
    [RFC 1/6] lib: vsprintf: additional kernel pointer filtering options Greg KH <gregkh@linuxfoundation.org> - 2017-05-06 06:10 +0200
    [RFC 3/6] lib: vsprintf: physical address kernel pointer filtering  options Greg KH <gregkh@linuxfoundation.org> - 2017-05-06 06:10 +0200
      Re: [kernel-hardening] [RFC 3/6] lib: vsprintf: physical address  kernel pointer filtering options Ian Campbell <ijc@hellion.org.uk> - 2017-05-06 12:50 +0200
    [RFC 2/6] lib: vsprintf: whitelist stack traces Greg KH <gregkh@linuxfoundation.org> - 2017-05-06 06:10 +0200
    Re: [RFC 00/06] printk: add more new kernel pointer filter options. Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-05-11 03:40 +0200

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

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(-)

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1639143

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web