Re: [patch 1/1] net: convert %p usage to %pK

[Eric Dumazet <eric.dumazet@gmail.com>]

Le mardi 24 mai 2011 à 01:13 -0400, David Miller a écrit :
> From: akpm@linux-foundation.org
> Date: Mon, 23 May 2011 15:17:35 -0700
> 
> > From: Dan Rosenberg <drosenberg@vsecurity.com>
> > 
> > The %pK format specifier is designed to hide exposed kernel pointers,
> > specifically via /proc interfaces.  Exposing these pointers provides an
> > easy target for kernel write vulnerabilities, since they reveal the
> > locations of writable structures containing easily triggerable function
> > pointers.  The behavior of %pK depends on the kptr_restrict sysctl.
> > 
> > If kptr_restrict is set to 0, no deviation from the standard %p behavior
> > occurs.  If kptr_restrict is set to 1, the default, if the current user
> > (intended to be a reader via seq_printf(), etc.) does not have CAP_SYSLOG
> > (currently in the LSM tree), kernel pointers using %pK are printed as 0's.
> >  If kptr_restrict is set to 2, kernel pointers using %pK are printed as
> > 0's regardless of privileges.  Replacing with 0's was chosen over the
> > default "(null)", which cannot be parsed by userland %p, which expects
> > "(nil)".
> > 
> > The supporting code for kptr_restrict and %pK are currently in the -mm
> > tree.  This patch converts users of %p in net/ to %pK.  Cases of printing
> > pointers to the syslog are not covered, since this would eliminate useful
> > information for postmortem debugging and the reading of the syslog is
> > already optionally protected by the dmesg_restrict sysctl.
> > 
> > Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> 
> Applied.

We probably need to extend this to inet_diag as well.

"ss -e" currently expose kernel pointers, like /proc files used to do
before this patch.

Thanks

[PATCH] inet_diag: hide socket pointers

Provide a mayber_hide_ptr() helper and use it in inet_diag to not
disclose kernel pointers to user, with kptr_restrict logic :

kptr_restrict = 0 : kernel pointers are not mangled
kptr_restrict = 1 : if the current user does not have CAP_SYSLOG,
kernel pointers are replaced by 0
kptr_restrict = 2 : kernel pointers are replaced by 0

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 include/linux/printk.h |    1 +
 lib/vsprintf.c         |   15 +++++++++++----
 net/ipv4/inet_diag.c   |    6 ++++--
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index ee048e7..47c0cef 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -111,6 +111,7 @@ extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
 extern int printk_delay_msec;
 extern int dmesg_restrict;
 extern int kptr_restrict;
+void *maybe_hide_ptr(void *ptr);
 
 void log_buf_kexec_setup(void);
 #else
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 1d659d7..20d3576 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -799,6 +799,16 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 
 int kptr_restrict __read_mostly;
 
+void *maybe_hide_ptr(void *ptr)
+{
+	if (!((kptr_restrict == 0) ||
+	      (kptr_restrict == 1 &&
+	       has_capability_noaudit(current, CAP_SYSLOG))))
+		ptr = NULL;
+	return ptr;
+}
+EXPORT_SYMBOL(maybe_hide_ptr);
+
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
@@ -911,10 +921,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 				spec.field_width = 2 * sizeof(void *);
 			return string(buf, end, "pK-error", spec);
 		}
-		if (!((kptr_restrict == 0) ||
-		      (kptr_restrict == 1 &&
-		       has_capability_noaudit(current, CAP_SYSLOG))))
-			ptr = NULL;
+		ptr = maybe_hide_ptr(ptr);
 		break;
 	}
 	spec.flags |= SMALL;
diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 6ffe94c..b5646a3 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -84,6 +84,7 @@ static int inet_csk_diag_fill(struct sock *sk,
 	struct inet_diag_meminfo  *minfo = NULL;
 	unsigned char	 *b = skb_tail_pointer(skb);
 	const struct inet_diag_handler *handler;
+	u64 ptr;
 
 	handler = inet_diag_table[unlh->nlmsg_type];
 	BUG_ON(handler == NULL);
@@ -114,8 +115,9 @@ static int inet_csk_diag_fill(struct sock *sk,
 	r->idiag_retrans = 0;
 
 	r->id.idiag_if = sk->sk_bound_dev_if;
-	r->id.idiag_cookie[0] = (u32)(unsigned long)sk;
-	r->id.idiag_cookie[1] = (u32)(((unsigned long)sk >> 31) >> 1);
+	ptr = (u64)maybe_hide_ptr(sk);
+	r->id.idiag_cookie[0] = (u32)ptr;
+	r->id.idiag_cookie[1] = (u32)(ptr >> 32);
 
 	r->id.idiag_sport = inet->inet_sport;
 	r->id.idiag_dport = inet->inet_dport;