diff mbox

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

Message ID 1506816410-10230-2-git-send-email-me@tobin.cc (mailing list archive)
State New, archived
Headers show

Commit Message

Tobin Harding Oct. 1, 2017, 12:06 a.m. UTC
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(-)

Comments

Greg KH Oct. 4, 2017, 8:55 a.m. UTC | #1
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>
Steven Rostedt Oct. 4, 2017, 1:08 p.m. UTC | #2
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
Greg KH Oct. 4, 2017, 1:26 p.m. UTC | #3
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
Steven Rostedt Oct. 4, 2017, 1:29 p.m. UTC | #4
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
Greg KH Oct. 4, 2017, 1:54 p.m. UTC | #5
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
diff mbox

Patch

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;
 				}